Hooks/Dispatcher - Close loopholes that occur around "preboot" hooks

[totten]

Overview

In CRM_Utils_Hook and Civi::dispatcher(), there is a subtle distinction between normal hooks and "preboot" hooks. This eliminates the distinction, making it easier to reason about and modify the hook mechanism.

Before

• To listen to almost any hook, you may use function PREFIX_civicrm_HOOK($param...) or Civi::dispatcher().
• Except... if you wish to listen to the preboot hooks (hook_civicrm_container and hook_civicrm_entityTypes), then Civi::dispatcher() does not work.
• One cannot register any listeners via Civi::dispatcher() until fairly late during bootstrap.

After

• All hooks - including hook_civicrm_container and hook_civicrm_entityTypes should work with Civi::dispatcher().
• There is a built-in guard-rail - if some future change causes a hook to fire too early (eg in a way that breaks Civi::dispatcher()), then it will raise an exception. This is not specifically a unit-test - but it would raised by the unit-tests for any use-cases involving a premature hook.
• One can register listeners via Civi::dispatcher() fairly early during bootstrap.

Comments

In "Before/After", the third item speaks about "early" and "late" during bootstrap, and this is probably a bit abstract. My long-term aim is to move some of the civix boilerplate which will require other PR(s). For something immediate+concrete, this may be a better demonstration of the principle:

• Before, an extension which uses Civi::dispatcher() must call it from within some other hook:

  // FILE: mymod.php
  function mymod_civicrm_config() {
    ...
    Civi::dispatcher()->addListener('hook_civicrm_pre', ...);
    ..
  }

• After, an extension may directly use Civi::dispatcher() at the top-level of the PHP file:

  // FILE: mymod.php
  Civi::dispatcher()->addListener('hook_civicrm_pre', ...);

I haven't fully tested the lifecycle of an extension using this code-style (because it's a bit incidental/by-the-by), but it hopefully demonstrates the principle of the change.

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

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@totten so I'm just looking at this from the point of view of whether it should get the 'has-test', 'ok-without-test' o 'needs-test' flag. It's definitely not in the first category. I think I apply the second when it either

1. is very form-layer (wording changes, css, tpl but not postProcess handling)
2. is unreasonably hard to write a test for
3. we are dealing with aa regression under pressure & we need to respect the fact that someone has just 'dropped everything to solve a regression' and not pile more on that situation

This list probably can and should be expanded - & I'm hoping that if you make the case for 'ok -without-test' here it will help to clarify it

[totten]

@eileenmcnaughton Yeah, good comment. It is fairly hard to frame a unit-test for this. It took me a bit to think up a QA/defensive mechanism. The primary risk, IMHO, is that some innocuous-looking patch will have some obscure side-effect which inadvertently causes some hook to dispatch in the midst of bootstrap.

(The stuff in a7718ac is a pretty good example of otherwise innocuous code which inadvertently dispatches hook_civicrm_entityTypes.)

This patch offers a defense using setDispatchPolicy(). To see this defense in action, you can sprinkle in calls to \CRM_Utils_Hook::entityTypes($x); in various places (e.g. in createLockManager() or SettingsManager::__construct() or CRM_Utils_System_*::initialize()) - if anything tries to fire a hook before the boot system is good-and-ready for it, then you'll get an exception.

(Clarification: The setDispatchPolicy() notation is probably unfamiliar. The first policy ['/./' => 'not-ready'] matches the wildcard regex and throws a "Not ready" exception if any hooks are dispatched. The second policy is typically NULL, which allows all hooks -- notwithstanding the pre-existing edge-case for upgrades.)

The key thing here is that the defensive measure is always used during boot(). Consequently, every unit-test and every E2E test will employ this defensive measure. IMHO, the odds are quite high that some automated-test (if not all automated-tests) will raise a redflag on regression.

Arguably, the defense should be a little stronger - 42ccedc relaxes the dispatch-policy a little sooner than I'd like. It's just that the branch (if ($loadFromDB...) means that the More Correct patch will also be More Ugly. But I suppose it's worth it - I can push up a tweak to tighten it.

[totten]

ab79d6c extends the defense a little further and expands the comments.

[eileenmcnaughton]

OK - I think we are homing in on dealing with this ongoing whack-a-mole - merging