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

[totten]

Overview

Suppose you have a hook-listener class:

class Foo implements HookInterface {
  public function hook_civicrm_foo($arg1, $arg2, ...) {}
}

And suppose this class is registered as a service. Conceptually, when firing hook_civicrm_foo, it should run the equivalent of

Civi::service('foo')->hook_civicrm_foo($arg1, &$arg2, $arg3);

This fixes a bug in handling the & above.

(Note: Includes/depends on a commit from #24283.)

Before

It does run, but the data is passed to the hook by-value. References (eg &$arg2) are lost. Parameters cannot be altered.

After

References are preserved. Parameters can be altered. There's a test to show this.

Technical Details

1. This includes a couple other commits from Autoload services based on interfaces/annotations. Convert APIv4 services. #24276. These other commits touch code that already gets a lot of testing, and they enable additional testing of this bug.

2. There are two existing adapters, HookStyleListener and ServiceListener. Before, they were stacked on top of each other. Separately, they work with alterable params -- but something about the combination doesn't work. I fiddled for a while and couldn't find a dainty solution. So this just adds a third adapter which does the same thing (ie HookStyleServiceListener =~ HookStyleListener + ServiceListener).

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

[demeritcowboy]

I tried testing this but I might not be setting it up right. In the test here, it explicitly registers the class. In the docs it says it will automatically find it, so I tried that:

class CRM_Abc_Hook implements \Civi\Core\HookInterface {
  public function hook_civicrm_check(&$msg, $statusNames, $includeDisabled) {
    $msg[] = new CRM_Utils_Check_Message(
      'abcext',
      ts('Abc Ext'),
      ts('Abc abc abc'),
      \Psr\Log\LogLevel::WARNING,
      'fa-flag'
    );
  }
}

but it doesn't seem to pick up. Clearing cache didn't change anything. It makes some sense that I might need to register it, but then do the docs need updating? If I implement that same hook the "normal" way in the extension's main abc.php file it does work.

[totten]

(@demeritcowboy) I tried testing this but I might not be setting it up right. In the test here, it explicitly registers the class. In the docs it says it will automatically find it, so I tried that:

Quite right... There's a limitation -- which isn't super visible, but it's mentioned in the docs under "Which classes support declarative listeners?". Basically, up to current master@HEAD, it will auto-scan BAOs and headless-tests -- but anything else requires extra registration.

The aim of #24276 is to open up a broader auto-registration (which can work with any class). There's a test that relies on auto-registration in that PR: totten@0067e4a.

But... the test was failing... because of this bug. That branch is already fairly big, so I just extracted this piece (smaller/more readable PR). This PR only fixes the bug in CiviEventDispatcher. Auto-registration will come from the other PR.

[totten]

And yeah... it is a bit cumbersome to reproduce the bug without autoregistration... You basically have to write a service, register it manually, and then check that the service gets notifications. (Which is what the test 6551756 does.)

[demeritcowboy]

Aha. So r-run: I registered it and before the path it does enter the hook but the var doesn't seem to get updated. After the patch it does.