Fix loading with alternate packages path (system-level) #16407
Merged
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
|
(Standard links)
|
|
jenkins, test this please |
| @@ -124,7 +124,13 @@ public function register($prepend = FALSE) { | |||
| $this->initHtmlPurifier($prepend); | |||
|
|
|||
| $this->_registered = TRUE; | |||
| $packages_path = implode(DIRECTORY_SEPARATOR, [$civicrm_base_path, 'packages']); | |||
| // The ClassLoader runs before the classes are available. Approximate Civi::paths()->get('[civicrm.packages]'). | |||
| if (isset($GLOBALS['civicrm_paths']['civicrm.packages']['path'])) { | |||
There was a problem hiding this comment.
Just adds an IF - no change for normal cases
| @@ -177,7 +183,12 @@ private function getHtmlPurifierPath() { | |||
| // we do this to prevent a autoloader errors with joomla / 3rd party packages | |||
| // Use absolute path, since we don't know the content of include_path yet. | |||
| // CRM-11304 | |||
| $file = dirname(__FILE__) . '/../../packages/IDS/vendors/htmlpurifier/HTMLPurifier/Bootstrap.php'; | |||
| if (isset($GLOBALS['civicrm_paths']['civicrm.packages']['path'])) { | |||
| @@ -433,7 +433,7 @@ public static function strtoboolstr($str) { | |||
| * the converted string | |||
| */ | |||
| public static function htmlToText($html) { | |||
| require_once 'packages/html2text/rcube_html2text.php'; | |||
| require_once 'html2text/rcube_html2text.php'; | |||
There was a problem hiding this comment.
we have tests on this I believe
| @@ -818,6 +818,19 @@ public function coreResourceList($region) { | |||
| // Allow hooks to modify this list | |||
| CRM_Utils_Hook::coreResourceList($items, $region); | |||
|
|
|||
| // Oof, existing listeners would expect $items to typically begin with 'bower_components/' or 'packages/' | |||
| // (using an implicit base of `[civicrm.root]`). We preserve the hook contract and cleanup $items post-hook. | |||
There was a problem hiding this comment.
This is the most complex part of this PR but my take on it is it would fail very broadly if it were going to. I'm pretty sure the new mailing screen would not load if this were broken (it does load)
There was a problem hiding this comment.
Quite right - if these things didn't work, then everything would be wrong in... pretty much every screen. It'd be missing jQuery, etc. A problem would be pretty obvious.
|
Code makes sense - I did a basic click around on the test site - feels low risk. Merging |
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
The configuration option
$civicrm_paths['civicrm.packages']allows one to specify the local path and remote URL of thepackagesfolder. This is not done often, but it is helpful for the composer/D8/git deployment styles. Alas, there are still sundry hard-coded references to thepackagesfolder.This fixes some (but not all) references to
packages. It generally focuses on classes that are used in universal/system-level processes (such as the class-loader).Before
CRM_Core_Resources,CRM_Core_Smarty,CRM_Core_IDS,CRM_Core_ClassLoader, andCRM_Utils_Stringhave path references which assume thatcivicrm-packages.gitlives directly under[civicrm.root]/packages.After
Each of these references has been updated to relax that expectation.
Comments
Generally, it's preferable to either:
packagesand just rely on the include-path (for*.phpfiles)packagesusing the "paths" subsystem (e.g.Civi::paths()->getPath('[civicrm.packages]/foobar').However, some files have peculiar issues which make that problematic -- so several of these items required alternative solutions.
This PR is an extracted subset of #16328, which was an exploratory branch aimed at supporting deployment of Civi git repos in D8 via composer. The changes are extracted to make the size of the review more manageable. It's probably best to use this smaller PR to consider topics like regression-risk and general code convention rather than trying to assess the fuller aims of #16328.