(dev/core#3660) Ensure that rebuildMenuAndCaches has current mixins+classloaders #23824
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
…lassloaders Overview -------- This fixes an issue in transitioning from `hook_managed` to `mixin/mgd-php@1`, wherein managed-entities are briefly deleted (but later recreated). Steps to Reproduce ------------------ 1. (Web) Install an extension with a revision that uses `hook_managed` 2. (CLI) Switch the extension to a revision that uses `mixin/mgd-php@1` 3. (CLI) View contents of `civicrm_managed` 4. (Web) Run `civicrm/menu/rebuild` 5. (CLI) View contents of `civicrm_managed` 6. (Web) Run `civicrm/menu/rebuild` 7. (CLI) View contents of `civicrm_managed` Before ------ While processing step 4 (`civicrm/menu/rebuild`), it fails to run the hooks for `mgd-php`. Consequently, the list of managed-entities is lost and will disappear at step 5. After ----- While processing step 4 (`civicrm/menu/rebuild`), it activates the hooks for `mgd-php`. Technical Details ------------------ When processing `civicrm/menu/rebuild`, there are a couple big substeps: * `Civi\Core\Container::boot()` - During this process, it loads extensions. As usual, this reads cached metadata, sets up classloaders, sets up mixins/hooks, etc. * `CRM_Core_Invoke::rebuildMenuAndCaches()` - During this process, it clears out caches and rebuilds several things (menus, managed-entities, etc). The problem is this: * The cache used during `boot()` has stale metadata (specifically, `civicrm_cache` has old values of `mixinScan` and `mixinBoot`). So it doesn't setup any new mixins/hooks. * Then `rebuildMenuAndCaches()` depends on the mixins/hooks that are already setup. While the function does clear persistent caches, it assumes that the PHP runtime environment is up-to-spec. But it's not -- becuase our hooks were based on the caches. The patch uses the same `refresh()` mechanism as the extension-administration subsystem (which has to reset the classloaders+mixins after enabling or disabling an extension).
|
(Standard links)
|
|
Note: I considered an alternative - track timestamps for all active
There is a hypothetical world where that would be worth entertaining, but (for now) the current PR seemed easier and more performant. |
|
Tested |
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
Jun 29, 2022
…refreshes Overview -------- This is a follow-up to civicrm#23824 (c24dd7d) which addresses a regressive edge-case. Steps to Reproduce ------------------ * Write an extension like `wmf-civicrm` which (a) calls `System.flush` (`rebuildMenuAndCaches()`) during `hook_install` -- and then (b) loads some class from the same extension. ```php function foo_civicrm_install() { civicrm_api3('System', 'flush', []); CRM_Foo_Helper::doStuff(); } ``` * Try to install the extension. Before ------ Crashes on loading the class `CRM_Foo_Helper` After ----- Loads the class `CRM_Foo_Helper`. Comments -------- (1) To see what's happening, consider `CRM_Extension_Manager_Module::onPreInstall()`. This registers the new classloader and then fires `hook_install` which eventually fires `rebuildMenuAndCaches()`. With c24dd7d, this resets the classloader again. But the extension isn't fully installed yet - so it forgets about the new extension. (2) Is it safe to have some (temporarily) sticky items? Ish. You might say: "Ah, but what if we need to remove an extension? Won't this static variable retain stale things?" Doesn't matter. In PHP, classloading is a one-way-street. (You cannot unload.) So you'll still have old classes in memory, regardless of whether the `ClassLoader` has some old metadata about where to find classes. (3) I'm on the fence about whether this patch is a good idea. Calling `System.flush` explicitly in this context seems like an invitation to trouble. OTOH, it worked before civicrm#23824, so it can be called a regression.
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 fixes an issue in transitioning from
hook_managedtomixin/mgd-php@1, wherein managed-entities are briefly deleted (but later recreated).See also: https://lab.civicrm.org/dev/core/-/issues/3660
ping @colemanw
Steps to Reproduce
hook_managedto load*.mgd.phpfilescivicrm_managedmixin/mgd-php@1to load*.mgd.phpfilescivicrm/menu/rebuildcivicrm_managedcivicrm/menu/rebuildcivicrm_managedBefore
While processing step 4 (
civicrm/menu/rebuild), it fails to run the hooks formgd-php.Consequently, managed-entities are (temporarily) lost/destroyed - and will be missing at step 5.
After
While processing step 4 (
civicrm/menu/rebuild), it activates the hooks formgd-php.Consequently, managed-entities are preserved.
Technical Details
When processing
civicrm/menu/rebuild, there are a couple big substeps:Civi\Core\Container::boot()- During this process, it loads extensions. As usual, this reads cached metadata, sets up classloaders, sets up mixins/hooks, etc.CRM_Core_Invoke::rebuildMenuAndCaches()- During this process, it clears out caches and rebuilds several things (menus, managed-entities, etc).The problem is this:
boot()has stale metadata (specifically,civicrm_cachehas old values ofmixinScanandmixinBoot). So it doesn't setup any new mixins/hooks.rebuildMenuAndCaches()depends on the mixins/hooks that are already setup. While the function does clear persistent caches, it assumes that the PHP runtime environment is up-to-spec. But it's not -- because our hooks were based on the old caches.This patch uses the same
refresh()mechanism as the extension-administration subsystem (which has to reset the classloaders+mixins after enabling or disabling an extension).