From ecb0ae5d0d607a53da2037f7fc618c38e23b81db Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 20 Apr 2020 15:05:10 -0700 Subject: [PATCH] (REF) dev/core#1460 - CRM_Utils_Hook::invoke() - Use DispatchPolicy. Simplify. Add test. Overview -------- Broadly speaking, this patch aims to allow the list of supported hooks to change *during* the upgrade process. To wit: * While executing low-level DB updates, do not fire many hooks. * Ater executing low-level DB updates, fire whatever hooks are needed for the system to flush its caches. Before ------ When running upgrades, all phases of the upgrade must use the same list of permitted hooks. The list of ugprade hooks is encoded inside of `CRM_Utils_Hook::invoke` (and `::pre`/`::post`). The hook whitelist is stored inside of `CRM_Utils_Hook::invoke` as `$upgradeFriendlyHooks`. After ----- When running upgrades, the main phase (incremental updates) and the final phase (doFinish/rebuild) can each use a different list of permitted hooks. The hook whitelist is stored inside of `CRM_Upgrade_DispatchPolicy`. Technical Details ----------------- The updates for `invoke()` have a couple of incidental changes. These simplify the conditionals/code-paths and also ensure that the dispatch policy is consistently enforced. * The hidden option `CIVICRM_FORCE_LEGACY_HOOK` is no longer supported. We've been using this invocation style now for years - I haven't heard of anyone needing `CIVICRM_FORCE_LEGACY_HOOK` in that, and Google doesn't find anyone discussing it. * Circa `civicrm-core@4.7.x`, the first param changed from `int $count` to `string[] $names` to provide compatibility with Symfony-style listeners. However, the `int` approach was still supported for backward compatibility with extensions. This patch still provides backward-compatibility, but it subtly changes the behavior for legacy callers. Before, legacy callers would bypass the Symfony dispatcher completely. Now, they go through the Symfony dispatcher, but they use placeholder names (`arg1`, `arg2`, etc). * The support for both canonical `string[] $names` and deprecated `int $count` is now covered by a unit-test. --- CRM/Upgrade/Form.php | 10 ++++ CRM/Utils/Hook.php | 62 +++++++++++-------------- Civi/Core/Container.php | 4 ++ tests/phpunit/E2E/Core/HookTest.php | 72 +++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 36 deletions(-) create mode 100644 tests/phpunit/E2E/Core/HookTest.php diff --git a/CRM/Upgrade/Form.php b/CRM/Upgrade/Form.php index 72ceb75005..3c65c5e31f 100644 --- a/CRM/Upgrade/Form.php +++ b/CRM/Upgrade/Form.php @@ -769,11 +769,21 @@ SET version = '$version' } public static function doFinish() { + Civi::dispatcher()->setDispatchPolicy(\CRM_Upgrade_DispatchPolicy::get('upgrade.finish')); + $restore = \CRM_Utils_AutoClean::with(function() { + Civi::dispatcher()->setDispatchPolicy(\CRM_Upgrade_DispatchPolicy::get('upgrade.main')); + }); + $upgrade = new CRM_Upgrade_Form(); list($ignore, $latestVer) = $upgrade->getUpgradeVersions(); // Seems extraneous in context, but we'll preserve old behavior $upgrade->setVersion($latestVer); + // TODO: Consider running a general system flush instead of piecemeal flushes. + // Historically, that would not have worked well because hooks are restricted + // in upgrade-mode. But the dispatch-policy for 'upgrade.finish' is probably + // more suitable. + // Clear cached metadata. Civi::service('settings_manager')->flush(); diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index d634c0c7a1..289c1c8c3a 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -152,35 +152,35 @@ abstract class CRM_Utils_Hook { &$arg1, &$arg2, &$arg3, &$arg4, &$arg5, &$arg6, $fnSuffix ) { - // Per https://github.com/civicrm/civicrm-core/pull/13551 we have had ongoing significant problems where hooks from modules are - // invoked during upgrade but not those from extensions. The issues are both that an incorrect module list & settings are cached and that - // some hooks REALLY need to run during upgrade - the loss of triggers during upgrade causes significant loss of data - // whereas loss of logTable hooks means that log tables are created for tables specifically excluded - e.g due to large - // quantities of low-importance data or the table not having an id field, which could cause a fatal error. - // Instead of not calling any hooks we only call those we know to be frequently important - if a particular extension wanted - // to avoid this they could do an early return on CRM_Core_Config::singleton()->isUpgradeMode - // Futther discussion is happening at https://lab.civicrm.org/dev/core/issues/1460 - $upgradeFriendlyHooks = ['civicrm_alterSettingsFolders', 'civicrm_alterSettingsMetaData', 'civicrm_triggerInfo', 'civicrm_alterLogTables', 'civicrm_container', 'civicrm_permission', 'civicrm_managed', 'civicrm_config']; - if (CRM_Core_Config::singleton()->isUpgradeMode() && !in_array($fnSuffix, $upgradeFriendlyHooks)) { - return; - } - if (is_array($names) && !defined('CIVICRM_FORCE_LEGACY_HOOK') && \Civi\Core\Container::isContainerBooted()) { - $event = \Civi\Core\Event\GenericHookEvent::createOrdered( - $names, - array(&$arg1, &$arg2, &$arg3, &$arg4, &$arg5, &$arg6) - ); - \Civi::dispatcher()->dispatch('hook_' . $fnSuffix, $event); - return $event->getReturnValues(); - } - else { - // We need to ensure tht we will still run known bootstrap related hooks even if the container is not booted. - $prebootContainerHooks = array_merge($upgradeFriendlyHooks, ['civicrm_entityTypes', 'civicrm_config']); - if (!\Civi\Core\Container::isContainerBooted() && !in_array($fnSuffix, $prebootContainerHooks)) { + if (!\Civi\Core\Container::isContainerBooted()) { + $prebootHooks = ['civicrm_container', 'civicrm_entityTypes']; + // 'civicrm_config' ? + if (in_array($fnSuffix, $prebootHooks)) { + $count = is_array($names) ? count($names) : $names; + return $this->invokeViaUF($count, $arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $fnSuffix); + } + else { + // TODO: Emit a warning, eg + // error_log("Warning: hook_$fnSuffix fired prematurely. Dropped."); return; } - $count = is_array($names) ? count($names) : $names; - return $this->invokeViaUF($count, $arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $fnSuffix); } + + if (!is_array($names)) { + // We were called with the old contract wherein $names is actually an int. + // Symfony dispatcher requires some kind of name. + // TODO: Emit a warning, eg + // error_log("Warning: hook_$fnSuffix does not give names for its parameters. It will present odd names to any Symfony event listeners."); + $compatNames = ['arg1', 'arg2', 'arg3', 'arg4', 'arg5', 'arg6']; + $names = array_slice($compatNames, 0, (int) $names); + } + + $event = \Civi\Core\Event\GenericHookEvent::createOrdered( + $names, + array(&$arg1, &$arg2, &$arg3, &$arg4, &$arg5, &$arg6) + ); + \Civi::dispatcher()->dispatch('hook_' . $fnSuffix, $event); + return $event->getReturnValues(); } /** @@ -355,11 +355,6 @@ abstract class CRM_Utils_Hook { * the return value is ignored */ public static function pre($op, $objectName, $id, &$params) { - // Dev/core#1449 DO not dispatch hook_civicrm_pre if we are in an upgrade as this cases the upgrade to fail - // Futher discussion is happening at https://lab.civicrm.org/dev/core/issues/1460 - if (CRM_Core_Config::singleton()->isUpgradeMode()) { - return; - } $event = new \Civi\Core\Event\PreEvent($op, $objectName, $id, $params); \Civi::dispatcher()->dispatch('hook_civicrm_pre', $event); return $event->getReturnValues(); @@ -382,11 +377,6 @@ abstract class CRM_Utils_Hook { * an error message which aborts the operation */ public static function post($op, $objectName, $objectId, &$objectRef = NULL) { - // Dev/core#1449 DO not dispatch hook_civicrm_post if we are in an upgrade as this cases the upgrade to fail - // Futher discussion is happening at https://lab.civicrm.org/dev/core/issues/1460 - if (CRM_Core_Config::singleton()->isUpgradeMode()) { - return; - } $event = new \Civi\Core\Event\PostEvent($op, $objectName, $objectId, $objectRef); \Civi::dispatcher()->dispatch('hook_civicrm_post', $event); return $event->getReturnValues(); diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index 6666ce61fc..f0bc4a2bf0 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -330,6 +330,10 @@ class Container { */ public function createEventDispatcher($container) { $dispatcher = new CiviEventDispatcher($container); + if (\CRM_Core_Config::isUpgradeMode()) { + $dispatcher->setDispatchPolicy(\CRM_Upgrade_DispatchPolicy::get('upgrade.main')); + } + $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\InstallationCanary', 'check']); $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\DatabaseInitializer', 'initialize']); $dispatcher->addListener(SystemInstallEvent::EVENT_NAME, ['\Civi\Core\LocalizationInitializer', 'initialize']); diff --git a/tests/phpunit/E2E/Core/HookTest.php b/tests/phpunit/E2E/Core/HookTest.php new file mode 100644 index 0000000000..ab9466dd51 --- /dev/null +++ b/tests/phpunit/E2E/Core/HookTest.php @@ -0,0 +1,72 @@ +addListener('hook_civicrm_e2eHookExample', function ($e) use (&$calls) { + $calls++; + $e->a['foo'] = 'a.name'; + $e->b->bar = 'b.name'; + }); + $a = []; + $b = new \stdClass(); + $this->hookStub(['a', 'b'], $a, $b); + $this->assertEquals(1, $calls); + $this->assertEquals('a.name', $a['foo']); + $this->assertEquals('b.name', $b->bar); + } + + /** + * This test ensures that CRM_Utils_Hook::invoke() dispatches via Symfony. + * + * This should be *in*addition* to dispatching through the UF event system, + * although the mechanics depend on the UF, so that part has to be tested per-UF. + * + * This uses the deprecated form, `CRM_Utils_Hook::invoke(int $count...)` + */ + public function testSymfonyListener_int() { + $calls = 0; + \Civi::dispatcher() + ->addListener('hook_civicrm_e2eHookExample', function ($e) use (&$calls) { + $calls++; + $e->arg1['foo'] = 'a.num'; + $e->arg2->bar = 'b.num'; + }); + $a = []; + $b = new \stdClass(); + $this->hookStub(2, $a, $b); + $this->assertEquals(1, $calls); + $this->assertEquals('a.num', $a['foo']); + $this->assertEquals('b.num', $b->bar); + } + + /** + * @param mixed $names + * @param array $a + * @param \stdClass $b + * @return mixed + */ + private function hookStub($names, &$a, $b) { + return \CRM_Utils_Hook::singleton() + ->invoke($names, $a, $b, \CRM_Utils_Hook::$_nullObject, \CRM_Utils_Hook::$_nullObject, \CRM_Utils_Hook::$_nullObject, \CRM_Utils_Hook::$_nullObject, + 'civicrm_e2eHookExample'); + } + +} -- 2.25.1