(dev/cloud-native#3) CRM_Utils_File - Deprecate baseFilePath() et al #14778
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The rationale is provided in the docblock.
Previous versions interpreted `$basePath=NULL` to mean "default to `self::baseFilePath()`". However, no code in the known `universe` relies on this interpretation, and the `baseFilePath()` function is problematic/deprecated. We could almost deprecate the entire function `absoluteDirectory()`; however, it's fine to keep around: the semantics are reasonable if one provides `$basePath`, and there is one active use-case in which `Civi\Core\Paths` calls `absoluteDirectory()` with a suitable `$basePath`.
Computation of a relative path requires some base. This implementation is problematic because it relies on an implicit base which was constructed problematically. Moreover, the function is not used anywhere in the known `universe`.
|
(Standard links)
|
|
This seems safe & sensible - I am a fan of more aggressive deprecation (throwing up the warnings) but this is quite early in your process I guess |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This deprecates a function which was enabling/complicit in problematic file-construction logic.
Before
CRM_Utils_File, the functionsbaseFilePath(),relativeDirectory($directory), andabsoluteDirectory($directory, $basePath = NULL)exist. They are not deprecated.After
CRM_Utils_File, the functionsbaseFilePath()andrelativeDirectory($directory)exist. They are deprecated, but they behave the same as before.CRM_Utils_File, the functionabsoluteDirectory($directory, $basePath)now requires the second parameter. This parameter is actually provided by all callers in the knownuniverse.Technical Details
The function
baseFilePath()represents a hack: the configuration incivicrm.settings.phpdid not provide a genuine way to figure out the path to the civicrm data folder (e.g./var/www/sites/default/files/civicrm), but it did provideCIVICRM_TEMPLATE_COMPILEDIR(e.g./var/www/sites/default/files/civicrm/templates_c). In absence of a proper reference to the data-folder, various bits of code usedbaseFilePath()to guess it.This hack means that paths are brittle: if one changes
CIVICRM_TEMPLATE_COMPILEDIR, it could have confusing repercussions on other folders which have nothing to do with template compilation.Since Civi 4.7, there has been a way to identify
/var/www/sites/default/files/civicrmspecifically (and even customize/override it) usingCivi::paths().However, this only deprecates
baseFilePath(). The function is useful in providing backward-compatibility with existing deployments/civicrm.settings.php-content.https://lab.civicrm.org/dev/cloud-native/issues/3