dev/core#1460, dev/core#1713 - Categorical fix for upgrade<=>hook issues #17126
Conversation
…Simplify. Add test. Overview -------- Broadly speaking, this patch aims to allow the list of supported hooks to change *during* the upgrade process. To wit: * While executing low-level DB updates, do not fire many hooks. * Ater executing low-level DB updates, fire whatever hooks are needed for the system to flush its caches. Before ------ When running upgrades, all phases of the upgrade must use the same list of permitted hooks. The list of ugprade hooks is encoded inside of `CRM_Utils_Hook::invoke` (and `::pre`/`::post`). The hook whitelist is stored inside of `CRM_Utils_Hook::invoke` as `$upgradeFriendlyHooks`. After ----- When running upgrades, the main phase (incremental updates) and the final phase (doFinish/rebuild) can each use a different list of permitted hooks. The hook whitelist is stored inside of `CRM_Upgrade_DispatchPolicy`. Technical Details ----------------- The updates for `invoke()` have a couple of incidental changes. These simplify the conditionals/code-paths and also ensure that the dispatch policy is consistently enforced. * The hidden option `CIVICRM_FORCE_LEGACY_HOOK` is no longer supported. We've been using this invocation style now for years - I haven't heard of anyone needing `CIVICRM_FORCE_LEGACY_HOOK` in that, and Google doesn't find anyone discussing it. * Circa `[email protected]`, the first param changed from `int $count` to `string[] $names` to provide compatibility with Symfony-style listeners. However, the `int` approach was still supported for backward compatibility with extensions. This patch still provides backward-compatibility, but it subtly changes the behavior for legacy callers. Before, legacy callers would bypass the Symfony dispatcher completely. Now, they go through the Symfony dispatcher, but they use placeholder names (`arg1`, `arg2`, etc). * The support for both canonical `string[] $names` and deprecated `int $count` is now covered by a unit-test.
…ish phases This is the culmination of the preceding commits: when running upgrades, we don't just apply one brittle whitelist throughtout the process. Instead: - Apply a very restrictive dispatch policy while executing incremental DB updates, preventing interference from unexpected callpaths. - Apply a very permissive dispatch policy while executing the final cleanup/reset, allowing more data-structures to rehydrate correctly.
…rebuild. Before ------ If/when an administrator opens the `civicrm/upgrade?reset=1` UI, then it rebuilds the menu (`CRM_Core_Menu::store()`). This runs in upgrade-mode, so the menu does not include any routes defined via hook. After ----- At the end of upgrade, after the general schema has reached canonical form, after most hooks are re-enabled for extensions, then it performs a full system flush. This incldues rebuilding the menu (and other caches). Comments -------- You might think that it serves some purpose to call `CRM_Core_Menu::store()` from `CRM_Upgrade_Page_Upgrade::runIntro()`. I can't figure one in the current regimen. Consider: * After loading a new source tree, do we need to rebuild the menu to access `civicrm/upgrade?reset=1`? Conceivably. But... if that's the case, then we wouldn't be able to visit `civicrm/upgrade?reset=1` at all. Moreover, the other upgrade routes (eg `civicrm/upgrade/queue*`) are hard-coded into the router specifically to resolve that chicken-egg issue. * Code in `CRM_Upgrade_Page_Upgrade::runIntro()` only runs if you use the web-based upgrader (not headless upgraders). So that line isn't a reliable part of upgrade logic. * When that old line runs, lots of hooks are disabled (incl `hook_xmlMenu` and `hook_alterMenu`), so it doesn't build a proper route table. * Consider subsystems like Afform and Data Processor - which allow admins to define new pages. These routes have to be stored somewhere and then loaded programmatically (eg `hook_alterMenu`). The logic which reads the list probably involves APIs, database tables, and/or settings -- i.e. things that are not be reliably functional with old schema.
|
(Standard links)
|
|
Extracted the first commit as a smaller PR that should be easier to review -- #17127. |
totten
changed the title
|
I like the concept, it feels like the right kind of solution. |
|
I merged #17127 - this seems good to me overall. My worries are around the setting hook because the results of that function call are cached - as long as 'wherever settings are cached' is cleared again with that hook enabled it should be fine. |
| $prebootContainerHooks = array_merge($upgradeFriendlyHooks, ['civicrm_entityTypes', 'civicrm_config']); | ||
| if (!\Civi\Core\Container::isContainerBooted() && !in_array($fnSuffix, $prebootContainerHooks)) { | ||
| if (!\Civi\Core\Container::isContainerBooted()) { | ||
| $prebootHooks = ['civicrm_container', 'civicrm_entityTypes']; |
There was a problem hiding this comment.
@totten I think we want civicrm_alterSettingsFolders and civicrm_alterSettingsMetaData as well given we want to boot with the settings as they should be modified right?
There was a problem hiding this comment.
Good question. I was a little bit surprised that this simplified as much as it did, but it checked out on each angle I could find:
-
In my testing (of the previous code)
h_c_alterSettingsFoldersandh_c_alterSettingsMetaDatadid not behave like pre-boot hooks. Specifically, I set a breakpoint on the declaration of$prebootContainerHooks = ..., cleared out thecivicrm_cacheandfiles/civicrm/templates_c, and openedcivicrm/upgrade?reset=1. The debugger showed thath_c_containerwas preboot, but that was it.(I also repeated the procedure on
civicrm/admin?reset=1. That showedh_c_entityTypesandh_c_containerboth being preboot.h_c_themespopped up as a red-herring. It's not preboot per se; it's just an oldinvoke(1,...)notation; fix in (REF) CRM_Utils_Hook - Remove deprecated formulations ofinvoke(int,…)#17124.) -
In my mind/expectations, the
h_c_alterSettingsFooshouldn't be fired pre-boot. To resolve some chicken/egg issues while refactoring circa 4.7, I had pulled out a handful of boot-critical/early-stage-bootstrap settings intoSettingsManager::getSystemDefaults(). This meant that the rest of the settings system could be deferred until late-stage bootstrap -- and then profit from a fuller set of services (extensions/hooks). -
Thinking in terms of this PR using two-phases during upgrade, my intuition is that:
- (a) During the planning/closed phase, I don't see how it matters if you have the settings from extensions -- even if you loaded them, they wouldn't be used, because all the other hooks are turned off. The settings are inert.
- (b) During the finish/open phase, when it runs the system-flush, it is important to fire these hooks -- so that caches hydrate properly and so that settings are available for use within other hooks.
There was a problem hiding this comment.
So we historically had a lot of significant breakage on upgrade around settings once we migrated from modules (which always ran hooks on upgrade) to extensions. However as long as when anything more a very limited subset of settings are loaded the hooks are called & definitely before any setting metadata is cached it should be ok
|
i think we've done a good job of putting this through it's hoops |
This updates some of the docblocks to reflect civicrm#17126
This updates some of the docblocks to reflect civicrm#17126
This updates some of the docblocks to reflect civicrm#17126
This guard was added by 912511a as part of a previous approach to managing hooks during upgrades. This general approach changed with civicrm#17126; so 17126 partially undid this... but it inadvertently had the effect of completely disabling `reconcile()` (because this guard was left).
This guard was added by 912511a as part of a previous approach to managing hooks during upgrades. This general approach changed with civicrm#17126; so 17126 partially undid this... but it inadvertently had the effect of completely disabling `reconcile()` (because this guard was left).
Overview
Over the past 10 or so releases, we've had a slow game of whack-a-mole involving the upgrader - e.g. in response to buggy interactions between the upgrader and extensions, we would gradually change the list of hooks that are allowed to execute within "upgrade mode" (eg 62f662b, 1b4ddd9, 912511a, b5cf569, a6ee76f). The game isn't done yet - eg dev/core#1713 is another similar issue. dev/core#1460 is an open issue asking how (generally) to address this topic.
This PR responds generally to dev/core#1460 and specifically to dev/core#1713. It aims at a solution which doesn't require as much maintenance for latent bugs. The essence of the change is:
civicrm/clearcacheorSystem.flushAPI orcv flush).During the rest of the discussion, it may help to consider that the upgrade logic is not monolithic - it comes in a few steps.
Before
While executing the upgrade process, all hooks are subject to a filtering policy - n.b. by default, all hooks are dropped/ignored, but there are exceptions: the whitelist (
$upgradeFriendlyHooks) specifies that certain hooks are allowed to run. This policy applies to all steps of the upgrade (planning, incrementing, and finalization).If you find that some data-structure is misbuilt because the necessary hook didn't fire during upgrade, then you must add it to
$upgradeFriendlyHooks. (And cross your fingers that it's not a super-generic hook -- likehook_civicrm_pre-- which might be problematic one way and necessary in another way.)After
While executing the upgrade process, there are two phases:
upgrade.main.upgrade.finish.Technical Details
Like
$upgradeFriendlyHooks, theupgrade.mainandupgrade.finishpolicies are also arrays, and they also list valid hooks. However, the signature has changed a bit (now:[string $eventNameRegex => string $action]). You can match events using a regex (e.g./^hook_civicrm_(pre|post)/) and give more nuanced actions, e.g.run: Allow any matching hooks to runwarn: Allow any matching hooks to run, but emit a warning that the hook was not expecteddrop: Quietly drop/ignore any matching hookswarn-drop: Emit a warning about the hook, and drop/ignore it.fail: Throw an exception if any matching hooks are invokedComments
(A) In broad terms, this should reduce the number of required exceptions -- instead of 1 rule with 10+ exceptions, there are 2 rules with ~2 exceptions.
(B) Incidentally, this PR also brings the upgrader into closer alignment with the familiar "system flush" operation. Intuitively, as a developer or admin, you'd expect that the upgrader would do a system flush -- and it sort of did. The upgrader's
doFinish()pantomimed parts of "system flush", but a full flush would be precarious because most hooks weren't running. With this PR's approach,doFinish()now allows most hooks to run, so you can do a full/proper "system flush".(C) This branch includes a few distinct commits, and they're... not small. 🙃 I can split off bits for smaller PRs. But I wanted to share the big picture first - because otherwise the individual commits won't seem too useful.
(D) I've done some testing of the use-case from https://lab.civicrm.org/dev/core/-/issues/1713 as well as running the upgrader with fairly old databases.