dev/core#2232 - Upgrade UI contaminates cache via l10n-js. Consolidate isUpgradeMode().
authorTim Otten <totten@civicrm.org>
Sat, 12 Dec 2020 03:33:23 +0000 (19:33 -0800)
committerTim Otten <totten@civicrm.org>
Sat, 12 Dec 2020 05:08:59 +0000 (21:08 -0800)
commit3ec0873bc42db8fb3eddf2b996f05af1f68acb87
tree3d21daec39ae8d3a2ad37cfeb82f9b32f99f48c4
parentdcc43a9be3ed8e35331d3a6c3c873468110106ce
dev/core#2232 - Upgrade UI contaminates cache via l10n-js. Consolidate isUpgradeMode().

This patch fixes an upgrade bug 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`: The functions agree -- it's a regular page, not an upgrade.  Therefore:
     * (a) It allows extensions to run fully.
     * (b) It enables `CachedCiviContainer` (read or write automatically).
     * (a+b) It puts good/full information in the cache.
2. `civicrm/upgrade`: The functions agree -- it's an upgrade. Therefore:
     * (a) It runs in paranoid mode (suspended extensions).
     * (b) It disables `CachedCiviContainer`.
     * (a+b) It puts no information in the cache.
3. `civicrm/ajax/l10n-js/en_US`: The functions *disagree*. In this case:
     * (a) It runs in paranoid mode (suspended extensions).
     * (b) It enables `CachedCiviContainer` (read or write automatically).
     * (a+b) 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).
CRM/Core/Config.php
CRM/Utils/System.php