CiviEventDispatcher - Fix pass-by-reference of hook-style arguments for service-based listeners

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);
   }
 
   /**

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
    *

Civi/Core/Event/HookStyleServiceListener.php

@@ -0,0 +1,37 @@
+<?php
+
+namespace Civi\Core\Event;
+
+/**
+ * A hook-style service-listener is a callable with two properties:
+ *
+ * - The parameters are given in hook style.
+ * - The callback is a method in a service-class.
+ *
+ * It is comparable to running:
+ *
+ *   Civi::service('foo')->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]);
+    }
+  }
+
+}

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);
     }

tests/phpunit/Civi/Core/Event/HookStyleServiceListenerTest.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Civi\Core\Event;
+
+use Civi\Core\Container;
+use Civi\Core\HookInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+/**
+ * Register a service (eg 'test.hssvlt') with a hook method (eg `function hook_civicrm_hssvlt()`).
+ * Ensure that the hook method can alter data.
+ */
+class HookStyleServiceListenerTest extends \CiviUnitTestCase {
+
+  public function tearDown(): void {
+    HookStyleServiceListenerTestExample::$notes = [];
+    parent::tearDown();
+  }
+
+  public function testDispatch() {
+    $changeMe = $rand = rand(0, 16384);
+
+    $this->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++;
+  }
+
+}