From: Tim Otten <totten@civicrm.org>
Date: Sat, 12 Dec 2020 03:33:23 +0000 (-0800)
Subject: dev/core#2232 - Upgrade UI contaminates cache via l10n-js. Consolidate isUpgradeMode().
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=3ec0873bc42db8fb3eddf2b996f05af1f68acb87;p=civicrm-core.git

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).
---

diff --git a/CRM/Core/Config.php b/CRM/Core/Config.php
index 1785fa9a93..62fa439755 100644
--- a/CRM/Core/Config.php
+++ b/CRM/Core/Config.php
@@ -409,6 +409,11 @@ class CRM_Core_Config extends CRM_Core_Config_MagicMerge {
       return TRUE;
     }
 
+    $upgradeInProcess = CRM_Core_Session::singleton()->get('isUpgradePending');
+    if ($upgradeInProcess) {
+      return TRUE;
+    }
+
     if (!$path) {
       // note: do not re-initialize config here, since this function is part of
       // config initialization itself
diff --git a/CRM/Utils/System.php b/CRM/Utils/System.php
index 47e72900f0..48abe7d393 100644
--- a/CRM/Utils/System.php
+++ b/CRM/Utils/System.php
@@ -1846,16 +1846,11 @@ class CRM_Utils_System {
    * Is in upgrade mode.
    *
    * @return bool
+   * @deprecated
+   * @see CRM_Core_Config::isUpgradeMode()
    */
   public static function isInUpgradeMode() {
-    $args = explode('/', CRM_Utils_Array::value('q', $_GET));
-    $upgradeInProcess = CRM_Core_Session::singleton()->get('isUpgradePending');
-    if ((isset($args[1]) && $args[1] == 'upgrade') || $upgradeInProcess) {
-      return TRUE;
-    }
-    else {
-      return FALSE;
-    }
+    return CRM_Core_Config::isUpgradeMode();
   }
 
   /**