dev/core#2232 - Upgrade UI contaminates cache via l10n-js. Consolidate isUpgradeMode().

[totten]

Overview

This patch fixes an upgrade bug (dev/core#2232) where CachedCiviContainer has stale data after an upgrade.

To do this, it removes an edge-case where two overlapping functions disagree.

Steps to Reproduce

1. Create a new/empty WP build
2. Install CiviCRM 5.30.1 from zipball
3. Install Mosaico 2.5
4. Download+extract new 5.32.1 zipball
5. Navigate to the GUI upgrade screen. Execute upgrade
6. Click to go back to the CiviCRM dashboard.
7. Receive error about the service 'mosaico_graphics'

Discussion

The problem is created while running the upgrade GUI (step 5). The GUI sends several HTTP requests (civicrm/upgrade, civicrm/upgrade/queue/ajax/runNext, etc). At the end, there is an HTTP request for civicrm/ajax/l10n-js/en_US. The l10n-js request creates a flawed copy of CachedCiviContainer which is responsible for subsequent errors.

This shouldn't happen - there are defensive mechanism which prevent it from happening (e.g. during civicrm/upgrade). What makes civicrm/ajax/l10n-js/en_US different? It turns on the implementation of isUpgradeMode() which activates the defensive mechanisms.

Before

There are two implementations of isUpgradeMode() (via CRM_Core_Config and CRM_Utils_System). They often agree, but not always.

Some defensive measures trigger on CRM_Core_Config::isUpgradeMode() and others trigger on CRM_Utils_System::isUpgradeMode(). They often trigger together, but not always.

Let's see how these can playout in a few HTTP requests:

1. civicrm/dashboard (or any normal page): The functions agree -- it's a regular page, not an upgrade. Therefore:
   • (a) Hook policy: It allows extensions to run fully.
   • (b) Cache policy: It enables CachedCiviContainer / CachedExtLoader (read or write automatically).
   • (a+b) Effect: It puts good/full information in the cache.
2. civicrm/upgrade: The functions agree -- it's an upgrade. Therefore:
   • (a) Hook policy: It runs in paranoid mode (suspended extensions).
   • (b) Cache policy: It disables CachedCiviContainer / CachedExtLoader.
   • (a+b) Effect: It puts no information in the cache.
3. civicrm/ajax/l10n-js/en_US: The functions disagree. In this case:
   • (a) Hook policy: It runs in paranoid mode (suspended extensions).
   • (b) Cache policy: It enables CachedCiviContainer / CachedExtLoader (read or write automatically).
   • (a+b) Effect: It puts bad/incomplete information in the cache.

After

There is one implementation of isUpgradeMode(). It may be called through either class (CRM_Core_Config or CRM_Utils_System), but the results will always agree.

This produces the same appropriate outcome for cases of agreement (1) (2), and it fixes the wonky/mismatched behavior in (3).

Comments

This patch builds on recent work in #19133 and #19141. A few observations:

• [REF] dev/core#2232 Re-Register Extension Class Loader after performi… #19133 added an extra clear/build at the end of the upgrade logic. One would've expected it to overwrite bad cache data. Why didn't it work? Because the problem wasn't in the upgrade logic. The problem was in the final call to civicrm/ajax/l10n-js/en_US in the upgrade UI. (This was pretty hard to find... took 45m of debugging and head-scratching...)
• dev/core#2232 Permit hook_civicrm_container and some other prebootish… #19141 did appear to fix the problem, and it demonstrated that (contrary to my own expectation) the restricted Hook Policy was influencing the content of CachedCiviContainer. However, I think this fix is better in two ways:
  • Preserves the posture of the main upgrade steps -- wherein naughty extensions can't throw a wrench into the awkward pre-upgrade stages.
  • Applies to any caches/hooks that might (now or the future) be implicated during AJAX calls. Don't need to maintain whitelist.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

I tested this and confirmed in that specific scenario it worked, I just wonder about extensions that provide their own permissions how that will work but seems ok I guess

[totten]

If there's a bug-report/steps-to-reproduce for some kind of permissions issue, then we can check.

General expectations:

• During the upgrade.main phase, there shouldn't be a need to build/use the list of permissions.
  • If there's some ambiguity about whether this expectation holds (e.g. if one suspects it doesn't hold but we don't have steps/time to reproduce), then we could set the policy for hook_civicrm_permission to warn or fail.
• When one hits the upgrade.finish phase (circa CRM_Upgrade_Form::doFinish()), then it enables all hooks and calls CRM_Core_Invoke::rebuildMenuAndCaches()=> cleanupPermissions().