DispatchPolicy - Actively report any upgrade problems with hook_civicrm_permission

[totten]

Overview

This is a preventive/diagnostic revision which would bring to light potential problems with firing cleanupPermissions()/hook_civicrm_permission at the wrong moment during an upgrade.

Before

If cleanupPermissions() (and its hook_civicrm_permission) are fired too early during an upgrade, then the cleanup runs quietly but omits important results (because the hook is dropped). On several UF's, the cleanupPermissions() revokes access for any omitted permissions. The sysadmin would have manually re-grant access.

After

If cleanupPermissions() (and its hook_civicrm_permission) are fired too early during an upgrade, then it raises an error.

Surely, it's better to report the error rather than silently drop the data.

Comments

• This revision is preventive/diagnostic/speculative. It's based on rumor that somebody had a problem with permissions in an upgrade. I don't actually have steps to reproduce a probelmatic case.

• To see this preventive mechanism in action, I provoked a trivial violation:

  • Setup a DB with 5.32
  • Checkout the code for 5.34.
  • Add a local hack https://gist.github.com/totten/e1448b343f94ff9c3d971402a1b3db3d
    which has the effect of running cleanupPermissions() prematurely.
  • Observe: cv upgrade:db fails (cv v0.3.5+)

• It's tempting to think of hook_civicrm_permission as returning a static list of strings -- in which case, there's no real harm to firing during the defensive phase. The problem is that hook_civicrm_permission can also be used for dynamic scenarios. (The power of hooks!) That's useful for any site-building extension (e.g. bespoke forms/views/APIs/dashlets) where you generate new permissions based on site configuration. That will depend on the
  correct-functioning of the extension+configuration... which cannot be ensured during the defensive upgrade-phase... and which creates a parallel choice between incorrect-results (data-loss) or fatal-error. This is the real reason why one shouldn't run cleanupPermissions() during the defensive phase. (Of course, we should run it... during the liberal phase...)

[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]

@totten just wondering if this should go against the RC given the other upgrade changes we have merged in there?

[totten]

@seamuslee001 Yeah, 5.33 is OK by me because it's related to some other RC fixes, but TBH this part is more a preventive thing than a critical fix, so it's grey-area.

I've rebased so that the PR can be switched freely to either 5.33 or master.

[seamuslee001]

I think 5.33 is the one given the other improvements that have happened there MOP