From aa0195698f8d5b4a62abf081a3b860814279f9b4 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 16 Aug 2022 14:46:05 -0700 Subject: [PATCH 1/3] (REF) Civi\Core\Container - Extract method for overriding the active container --- Civi/Core/Container.php | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index bf7c4672ca20..e0f205c0ce47 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -642,21 +642,30 @@ public static function boot($loadFromDB) { $runtime->includeCustomPath(); $c = new self(); - $container = $c->loadContainer(); - foreach ($bootServices as $name => $obj) { - $container->set($name, $obj); - } - \Civi::$statics[__CLASS__]['container'] = $container; - // Ensure all container-based serivces have a chance to add their listeners. - // Without this, it's a matter of happenstance (dependent upon particular page-request/configuration/etc). - $container->get('dispatcher'); - + static::useContainer($c->loadContainer()); } else { $bootServices['dispatcher.boot']->setDispatchPolicy(\CRM_Core_Config::isUpgradeMode() ? \CRM_Upgrade_DispatchPolicy::pick() : NULL); } } + /** + * Set the active container (over-writing the current container, if defined). + * + * @param \Symfony\Component\DependencyInjection\ContainerInterface $container + * @internal Intended for bootstrap and unit-testing. + */ + public static function useContainer($container): void { + $bootServices = \Civi::$statics[__CLASS__]['boot']; + foreach ($bootServices as $name => $obj) { + $container->set($name, $obj); + } + \Civi::$statics[__CLASS__]['container'] = $container; + // Ensure all container-based serivces have a chance to add their listeners. + // Without this, it's a matter of happenstance (dependent upon particular page-request/configuration/etc). + $container->get('dispatcher'); + } + /** * @param string $name * From b4e1d12ec64332fd128c876e62510d87d36d5e84 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 15 Aug 2022 21:53:34 -0700 Subject: [PATCH 2/3] CiviEventDispatcher - Fix pass-by-reference of hook-style arguments for service-based listeners Suppose you are firing `hook_civicrm_foo` to a serivce-based listener taht uses hook-style arguments. Conceptually, this is a call like: Civi::service('foo')->hook_civicrm_foo($arg1, &$arg2, $arg3); Before this patch, all values are pass-by-value. Changes to `&$arg2` are not propagated back out. The patch ensures that `&$arg2` propagates back out. --- Civi/Core/CiviEventDispatcher.php | 10 +++++- Civi/Core/Event/HookStyleServiceListener.php | 37 ++++++++++++++++++++ Civi/Core/Event/ServiceListener.php | 13 ++++--- 3 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 Civi/Core/Event/HookStyleServiceListener.php diff --git a/Civi/Core/CiviEventDispatcher.php b/Civi/Core/CiviEventDispatcher.php index d8db0119bc63..3aec51f3d1d3 100644 --- a/Civi/Core/CiviEventDispatcher.php +++ b/Civi/Core/CiviEventDispatcher.php @@ -155,7 +155,15 @@ public function addListenerService($eventName, $callback, $priority = 0) { throw new \InvalidArgumentException('Expected an array("service", "method") argument'); } - $this->addListener($eventName, new \Civi\Core\Event\ServiceListener($callback), $priority); + if ($eventName[0] === '&') { + $eventName = substr($eventName, 1); + $listener = new \Civi\Core\Event\HookStyleServiceListener($callback); + } + else { + $listener = new \Civi\Core\Event\ServiceListener($callback); + } + + $this->addListener($eventName, $listener, $priority); } /** diff --git a/Civi/Core/Event/HookStyleServiceListener.php b/Civi/Core/Event/HookStyleServiceListener.php new file mode 100644 index 000000000000..53a03c40b7c2 --- /dev/null +++ b/Civi/Core/Event/HookStyleServiceListener.php @@ -0,0 +1,37 @@ +hook_civicrm_foo($arg1, $arg2, ...); + */ +class HookStyleServiceListener extends ServiceListener { + + public function __invoke(...$args) { + if ($this->liveCb === NULL) { + $c = $this->container ?: \Civi::container(); + $this->liveCb = [$c->get($this->inertCb[0]), $this->inertCb[1]]; + } + + $result = call_user_func_array($this->liveCb, $args[0]->getHookValues()); + $args[0]->addReturnValues($result); + } + + public function __toString() { + $class = $this->getServiceClass(); + if ($class) { + return sprintf('$(%s)->%s(...$args) [%s]', $this->inertCb[0], $this->inertCb[1], $class); + } + else { + return sprintf('\$(%s)->%s(...$args)', $this->inertCb[0], $this->inertCb[1]); + } + } + +} diff --git a/Civi/Core/Event/ServiceListener.php b/Civi/Core/Event/ServiceListener.php index 63df85bc7149..faf58cc7fdf1 100644 --- a/Civi/Core/Event/ServiceListener.php +++ b/Civi/Core/Event/ServiceListener.php @@ -24,18 +24,18 @@ class ServiceListener { * @var array * Ex: ['service_name', 'someMethod'] */ - private $inertCb = NULL; + protected $inertCb = NULL; /** * @var array|null * Ex: [$svcObj, 'someMethod'] */ - private $liveCb = NULL; + protected $liveCb = NULL; /** * @var \Symfony\Component\DependencyInjection\ContainerInterface */ - private $container = NULL; + protected $container = NULL; /** * @param array $callback @@ -53,7 +53,7 @@ public function __invoke(...$args) { return call_user_func_array($this->liveCb, $args); } - public function __toString() { + protected function getServiceClass(): ?string { $class = NULL; if (\Civi\Core\Container::isContainerBooted()) { try { @@ -63,6 +63,11 @@ public function __toString() { catch (Throwable $t) { } } + return $class; + } + + public function __toString() { + $class = $this->getServiceClass(); if ($class) { return sprintf('$(%s)->%s($e) [%s]', $this->inertCb[0], $this->inertCb[1], $class); } From 6551756fbd6c44c40d03f0fb9dad8c63fd76b4f3 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 16 Aug 2022 14:53:31 -0700 Subject: [PATCH 3/3] HookStyleServiceListenerTest - Add test for running `hook_foo(&$alterable)` --- .../Event/HookStyleServiceListenerTest.php | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 tests/phpunit/Civi/Core/Event/HookStyleServiceListenerTest.php diff --git a/tests/phpunit/Civi/Core/Event/HookStyleServiceListenerTest.php b/tests/phpunit/Civi/Core/Event/HookStyleServiceListenerTest.php new file mode 100644 index 000000000000..4e5154316cd0 --- /dev/null +++ b/tests/phpunit/Civi/Core/Event/HookStyleServiceListenerTest.php @@ -0,0 +1,81 @@ +useCustomContainer(function (ContainerBuilder $container) use ($rand) { + $container->register('test.hssvlt', HookStyleServiceListenerTestExample::class) + ->addArgument($rand) + ->addTag('event_subscriber') + ->setPublic(TRUE); + }); + + $d = \Civi::dispatcher(); + + // Baseline + $this->assertEquals([], HookStyleServiceListenerTestExample::$notes); + $this->assertEquals($changeMe, $rand); + + // First call - instantiate and run + $d->dispatch('hook_civicrm_hssvlt', GenericHookEvent::create(['foo' => &$changeMe])); + $this->assertEquals($changeMe, 1 + $rand); + $this->assertEquals(["construct($rand)", "fired($rand)"], HookStyleServiceListenerTestExample::$notes); + + // Second call - reuse and run + $d->dispatch('hook_civicrm_hssvlt', GenericHookEvent::create(['foo' => &$changeMe])); + $this->assertEquals($changeMe, 2 + $rand); + $this->assertEquals(["construct($rand)", "fired($rand)", "fired(" . ($rand + 1) . ")"], HookStyleServiceListenerTestExample::$notes); + } + + /** + * Create and activate a custom service-container. + * + * @param callable $callback + * Callback function which will modify the container. + * function(ContainerBuilder $container) + */ + protected function useCustomContainer(callable $callback) { + $container = (new Container())->createContainer(); + $callback($container); + $container->compile(); + Container::useContainer($container); + } + +} + +class HookStyleServiceListenerTestExample implements HookInterface { + + /** + * Free-form list of strings. + * + * @var array + */ + public static $notes = []; + + public function __construct($rand) { + self::$notes[] = "construct($rand)"; + } + + public function hook_civicrm_hssvlt(&$foo) { + self::$notes[] = "fired({$foo})"; + $foo++; + } + +}