Fix hook_civicrm_permission upgrade failure. Defer system-flush to 'upgrade.finish' phase.

[seamuslee001]

Overview

port of PR #19345 to 5.34 / master

This addresses a recently reported upgrade failure. It moves a particular upgrade task (clearing all Drupal/Backdrop/etc caches) to address a previously latent discrepancy and prevent a cascade of premature interactions between modules.

Before

The upgrader executes Drupal/Backdrop cache-clear ($userSystem->flush()) in doIncrementalUpgradeFinish(), which is part of upgrade.main phase and which runs several times (after every incremental version-update).

During the upgrade.main phase, the pre-condition and invariant-condition is that the Civi schema is mismatched/unsuitable for executing most Civi logic. It is not until the end of the last incremental step that we reach the post-condition where schema is up-to-date and where most Civi logic can run safely.

Depending on the UF/modules/configuration, $userSystem->flush() has open-ended side-effects -- causing different third-party code to execute mid-upgrade. In the reported error, it kicks off a cascade that involves D7, Features, Panopoly, D7's permission subsystem, Civi's permission subsystem, and then any modules/extensions that tie into Civi's permission subsystem (at which point it balks per a9033ca).

The cascade caused by $userSystem->flush() is essentially chaotic (dependent on UF/modules/extensions/configurations). Of course, we don't care as long as the chaos stays over on the UF/CMS side... but if it circles back to the Civi side, then we're asking for trouble... because the invariant-condition of upgrade.main means that we cannot run most Civi logic reliably.

After

The upgrader executes Drupal/Backdrop cache-clear ($userSystem->flush()) in doFinish() as part of the upgrade.finish phase (ie after the schema is up-to-date). During the upgrade.finish phase, it is safe to run a wider range of Civi logic, so we are less sensitive to the chaotic cascade.

Comments

• Tangentially, this means that $userSystem->flush() will fire less often (e.g. 1x during upgrade.finish instead of 10x during upgrade.main). It'll be curious to see if this makes upgrades faster.
• The call to $userSystem->flush() was originally introduced circa v4.3 with https://issues.civicrm.org/jira/browse/CRM-11823 and d8a4acc. At the time, 4.3's drupal/civicrm.module had started to make critical use of D7's hook_theme_registry_alter (cacheable data), so upgrades needed to clear the D7 theme cache. That particular issue is now obsolete (MINIMUM_UPGRADABLE_VERSION is now 4.4), but it's interesting as to spot-check this patch with a real use-case. I think this checks out -- the incremental schema updates for Civi should not require rebuilding D7's theme-registry for every point-release (5.10.alpha1, 5.10.beta1, 5.10.0, 5.11.alpha1, ...). Rather, it makes more sense to rebuild D7's theme-registry one time.

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

[totten]

This is a forward-port. Merge-on-pass since it's already in the stable branch.