Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BAOs, Tests, etal - Support HookInterface and EventSubscriberInterface for auto-registration #20427

Merged
merged 12 commits into from
May 31, 2021

Conversation

totten
Copy link
Member

@totten totten commented May 26, 2021

Overview

This expands the functionality for declaring event-listeners in different classes, making it easier to participate in Symfony events. This expansion applies in two dimensions:

  • Allow subscribing in more contexts (unit-tests, APIv4 subscribers, and BAOs)
  • Allow subscribing to more types of events (hook_civicrm_* and civi.*)

The general approach is based on interface tagging -- in a supported context (e.g unit-test or BAO), you may mix-in HookInterface and/or EventSubscriberInterface and then declare your listeners.

Before

A headless test may implement HookInterface and then define methods with the hook_ prefix. It will be scanned for methods in the style of:

class ... extends ... implements HookInterface {
  public function hook_civicrm_alterContent(&$content, $context, $tplName, &$object) {}
}

However, this only works with traditional hooks. It does not work with Symfony-style events. Additionally, it only works with headless unit-tests -- there is no way to adapt it to other classes.

Separately, if an APIv4 subscriber (Civi\Api4\Event\Subscriber\*) implements EventSubscriberInerface, it will be scanned for listeners by calling getSubscribedEvents():

class ... extends ... implements EventSubscriberInterface {
  public static function getSubscribedEvents() {
    return ['civi.api.prepare' => 'myCiviApiPrepare'];
  }

  public function myCiviApiPrepare(\Civi\API\Event\PrepareEvent $event) {}

After

Declarative registration has been expanded in multiple ways.

(1) Declarative registration can be used on headless unit-tests (as before), APIv4 services (as before), and also on BAOs. All three types of classes support similar declarations (either HookInterface or EventSubscriberInterface).

(2) If a supported class implements the HookInterface, then it may use a naming-convention to declare listeners. The naming convention now works with all event-types.

class ... extends ... implements HookInterface {
  // Declare hook-style listeners using the `hook_` prefix.
  // This matches existing style.
  public function hook_civicrm_foobar($arg1, &$arg2, ...) {}

  // Declare Symfony-style listeners using the `on_` prefix.
  // This notation works with all events - not just hooks.
  public function on_civi_api_resolve(\Civi\API\Event\ResolveEvent $event) {}

  // Declare Symfony-styles listeners using the `self_` prefix.
  // This implies that we only care about events that target this specific entity.
  public function self_civi_api4_validate(ValidateEvent $event) {}
}

(3) If a supported class implements the EventSubscriberInterface, then it may use getSubscribedEvents() to declare a list of listeners.

class ... extends ... implements EventSubscriberInterface {
  public static function getSubscribedEvents() {
    return [
      'civi.api.prepare' => ['myCiviApiPrepare', 1234],
      'hook_civicrm_foo' => 'myFoo',
      'hook_civicrm_pre::MyEntity' => 'preMyEntity',
      '&hook_civicrm_alterBar' => 'myAlterBar',
    ];
  }

  public function myCiviApiPrepare(\Civi\API\Event\PrepareEvent $event) {}

  public function myFoo(GenericHookEvent $e) {}

  public function preMyEntity(GenericHookEvent $e) {}

  public function myAlterBar(&$barObject, $barMode, $barShape) {}
}

Technical Details

This exposes a couple new internal APIs for wiring-up listeners. Given an object or class, you may wire it up with:

// Hook-up listeners based on `static` class-methods
$map = EventScanner::findListeners('\Some\Class\Name');
$dispatcher->addListenerMap('\Some\Class\Name', $map);

// Hook-up listeners based on standard object-methods with a plain-old PHP object
$map = EventScanner::findListeners($someObject);
$dispatcher->addListenerMap($someObject, $map);

// Hook-up listeners based on standard object-methods with a service-object
$map = EventScanner::findListeners('\Some\Class\Name');
$dispatcher->addSubscriberServiceMap('some.service.id', $map);

The ideal arrangement is to split those two steps -- run the scan infrequently, store the results in a cache, and use the cache at runtime. For example, this uses the Container cache and defers addListenerMap() until runtime:

$map = EventScanner::findListeners($target);
$container->getDefinition('dispatcher')->addMethodCall('addListenerMap', [$target, $map]);

Other comments

The declarative style of registration has some advantages:

  • Declarations can be cached. If you have a broad/sparse event list (e.g. many events, many listeners, but if only a few are ever needed in a typical page-load), then it's more efficient to get the cached listeners (require_once 'CachedCiviContainer.XXX.php') than to imperatively poll each potential listener speculatively (foreach ($baos as $bao) { require_once '$bao.php'; $bao::initialize(); }).
  • Declarative style is more forgiving about lifecycle-issues. For example, headless unit-tests may reboot the container multiple times (e.g. during setUp(), e.g. when activating an extension temporarily) -- declarative style requires a little less care/knowledge to get the registrations working correctly.

@civibot
Copy link

civibot bot commented May 26, 2021

(Standard links)

@mattwire
Copy link
Contributor

Documentation should probably go here: https://docs.civicrm.org/dev/en/latest/testing/phpunit/#reference

@totten totten changed the title Support EventDispatcherInterface and HookInterface for auto-registration Support EventSubscriberInterface and HookInterface for auto-registration May 26, 2021
@totten totten force-pushed the master-scanner branch 2 times, most recently from ad12f72 to 0323b59 Compare May 26, 2021 21:42
totten added 8 commits May 26, 2021 16:52
… an object

```
$map = EventScanner::findListeners($someObject);
$dispatcher->addListenerMap($someObject, $map);
```
…r' to 'EventScanner'

The removes duplicate code and expands the functionality to support methods
of the form `_on_{$symfony_event}()`.
…terface` or `HookInterface`

Before: There is no pleasant way for a BAO to register a listener for an
event.  True, one can use `Civi::dispatch()`, but (lacking a good
initialization point) it's hard to register reliably.  True, you can also
add new items to `\Civi\Core\Container::createEventDispatcher()`, but
this is rather out-of-the-way.

After: A BAO class may optionally implement `EventDispatcherInterface` or
`HookInterface`. Methods will be automatically registered. The list
of listeners will be cached in the container.
This is just an example of doing declarative event-registration from a BAO.
This is just an example of doing declarative event-registration from a BAO.
@totten
Copy link
Member Author

totten commented May 27, 2021

@totten totten changed the title Support EventSubscriberInterface and HookInterface for auto-registration BAOs (etal) - Support EventSubscriberInterface and HookInterface for auto-registration May 27, 2021
@totten totten changed the title BAOs (etal) - Support EventSubscriberInterface and HookInterface for auto-registration BAOs, Tests, etal - Support HookInterface and EventSubscriberInterface for auto-registration May 27, 2021
@totten
Copy link
Member Author

totten commented May 28, 2021

Just an aside - this PR touches both HookInterface and EventSubscriberInterface. Of course, these interfaces already exist, and they do similar things, so one might call them redundant. The two interfaces have different histories and trade-offs. This PR merely brings them into some of kind of alignment. Compare:

  • Before: Classes used for X may listen via HookInterface (but not EventSubscriberInterface), and classes used for Y may listen via EventSubscriberInterface (but not HookInterface).
  • After: Classes used for X or Y may listen via any supported interface (HookInterface or EventSubscriberInterface or both, if you're crazy).

Both interfaces have test-coverage, and the delta between them is fairly small, so I don't think it's very hard to maintain both (or to add other variations!).

In the docs PR, I briefly mentioned some of the trade-offs - namely, HookInterface is more pithy, and it's more amenable to mixing (via traits). EventSubscriberInterface can handle funny event names, multiple listeners, and listener priorities.

There is perhaps another discussion about blessing one preferred interface (e.g. phase-out HookInterface or phase-out EventSubscriberInterface). If anyone wants that, then I'd suggest filing a separate ticket. The scope of that would be different. But even if it happens, a hypothetical transition will be easier with this in (i.e. you'll need some transitional period during which consumers can switch).

@@ -115,6 +115,14 @@ class CRM_Core_BAO_RecurringEntity extends CRM_Core_DAO_RecurringEntity {
const MODE_NEXT_ALL_ENTITY = 2;
const MODE_ALL_ENTITY_IN_SERIES = 3;

public static function getSubscribedEvents() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten my only misgiving on this PR is that it adds a few functions with stingy addition of return hints (and possibly type hints). I think return hints are definitely preferred practice these days & it's probably better to set your IDE to recommend them (the EA plugin does this).

I think it's worth just reviewing the functions added in this PR & adding return hints & type hints where possible

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed up a few commits to add more type-hints. Some notes:

  • For getSubscribedEvents(), the type-signature and semantics are dictated by EventSubscriberInterface. I suppose, technically, it is legal for an implementation of getSubscribedEvents() to specify a narrower return-type than the interface (e.g. getSubscribedEvents(): array is narrower than getSubscribedEvents()). Though it feels odd to deviate from the interface.
  • There are some new/internal functions (e.g. normalizeListenerMap() and mergeListenerMap()). These would be OK with more hints. Added via 60a1dbc.
  • For Symfony-style listeners (on_*($event) functions), I think it would be legit to always return void, so added that via e435f17. Although this reminds me an a (slightly unpleasant) fact... that sometimes hook_*($arg1, $arg2, ...) will return data. 65bc654 fixes support for that edge-case.
  • I wouldn't agree PHP-language type-hints are "definitely preferred". Changes like phpunit's retroactive type-hinting seem like gratuitous dephell, IMHO. But, yeah, if it's a new or obscure function, and if the signature is amenable to type-hinting in the current language, then 👍.

totten added 4 commits May 31, 2021 00:22
Note: This is uncommon and discouraged for new hooks, but some hooks require returning values, e.g

```php
function hook_foo() {
  return ['my-data'];
}
```

This should fix compatibility with those.
When using Symfony-style listeners, all inputs and outputs for the event go
through the event object.

Note that `hook_*()` notation does allow return values, For these functions,
it is *normal* to return void, but some existing hooks rely on returning
array-data.  It could be misleading if we made it appear that all `hook_*()`
examples have to return void.
@eileenmcnaughton eileenmcnaughton merged commit 9daf325 into civicrm:master May 31, 2021
@totten totten deleted the master-scanner branch July 8, 2021 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants