CRM-19813 - Unify Symfony Events and hooks #9949

totten · 2017-03-08T06:27:41Z

Civi has three notions of events:

Hooks: These are available to extensions (either native or CMS-based). They are usually described by a function signature. Hooks are the most recognizable/supported events, but they also have limitations (hard to inspect, prioritize, cancel, etc) because they must meet the lowest-common-denominator of CMS features.
Symfony Events: These are generally only used within core. They have more advanced features but haven't been presented for downstream usage. There's documentation available from Symfony.
Component-actions: These are adhoc functions which loop through core's list of "components". These are generally not well-understood.

This PR aims to unify two of them -- hooks and Symfony Events. More specifically:

Symfony Event Dispatcher become the canonical event system. All events are Symfony events. You can register event subscribers in core (or elsewhere).
Hooks are a special case in which (a) the event name looks like hook_civicrm_foo and (b) the event object extends GenericHookEvent. In this special case, the event will be simultaneously emitted through various CMS event systems (Drupal Hooks, WP Actions, etc).

If you were previously unaware that Symfony Events existed in core, then it seem like "yet another way". It is actually a unification of two existing ways. To understand how this unification positions us, consider an analogy to APIv3 CRUD:

For APIv3 CRUD, the public API has one set of semantics and multiple syntaxes. The semantics are based on entities+actions+params. The syntax for calling APIv3 varies depending on the technique/context (e.g. PHP logic vs PHP test vs JS vs REST vs CLI). There is a canonical mechanism which supports all features (civicrm_api(...)) and can be used in most PHP contexts, but some variants (like Smarty or CLI) can't easily support all features.
For hooks/events, the public API has one set of semantics and multiple syntaxes. The semantics are based on hook-name+params. The syntax for listening to an event varies depending on the technique/context (e.g. core code vs Civi extension vs Drupal module vs WordPress plugin vs Joomla plugin). There is a canonical mechanism which supports all features (Civi::service('dispatcher')) and can be used in most PHP contexts, but some variants (like Drupal/Joomla/WordPress modules) can't easily support all features.

Additional notes:

This means that core code can listen to hooks (the same way it already listens to events).
Hooks are the most visible/most accessible type of events. However, it's possible for any extension to work with the full features of the event-dispatcher.
Use cv debug:event-dispatcher to inspect the list of event listeners.
Some discussion of the design of GenericHookEvent is included in the relevant commit ("GenericHookEvent - Bridge between Symfony Events and hooks").
This is a WIP. There's another hook that I'm working on (for management of dynamic-assets) that is part of the impetus.
This is a WIP. To get full support for all hooks in Symfony Events, we need small patches to each stub in CRM_Utils_Hook. I've adapted ~1/3 so far.

CRM-19813: Hook priorities and core hooks to support LExIM

nganivet · 2017-03-08T08:32:12Z

Tim - Glad to see you've been working on this - specially the fact that core can now listen on hooks will be very helpful for the 'system' extensions recently discussed. I cannot review this as way to involved, but ... can I help (ie. adding small patches in the remaining 2/3 of the hook stubs)?

nganivet · 2017-03-08T08:35:34Z

Also ... what are the documentation changes required for this? Do we need to add a section with the additional events now available to extensions?

totten · 2017-03-24T19:30:52Z

I've removed the WIP flag because I've addressed the only blocker.

@nganivet I think the most helpful thing would be to smoke-test the patch in a few use-cases:

What: Apply the patch. Take a Drupal module, WordPress plugin, or Civi extension that you know well and which relies on core hooks. (Ex: IIRC, Sparkpost relies on several hooks.) Try using it like a regular person and see if it works. This doesn't need to be thorough evaluation of every codepath -- just a basic test (like you might do in a demo to a new user).
Why: Although I think this PR is pretty decent in terms of refactoring technique and unit-tests, there are still gaps in the test-coverage, and it would be pretty embarrassing if it broke the entire hook system in some common scenario. We're not likely to find anyone who can analytically prove the patch's safety, but a few light/diverse checks would helpful (per Swiss Cheese Model).

In my ideal world, our stack of cheese slices would include somewhere within it:

A Drupal module, a WordPress plugin, a Joomla plugin, a Civi extension
A hook that reads data -- and a hook that modifies data -- and one that returns data.
A hook that fires in a normal web-based UI and a hook that fires in backend/web-service context

If a couple people could try their own slices (whatever's familiar/easy for you) and report back, then I'll try a slice to fill in whatever gap remains.

nganivet · 2017-03-26T05:03:56Z

@totten I am putting testing this on my todo list, but am traveling starting tomorrow until April 4th. Will see if I have time while on the road, but most realistically not.

The normal `runHooks()` function has a weird protocol wherein results may be progressively merged (if they're non-empty arrays). This revision extends that behavior to each of the unit-test hook formulations. The patch enables better unit-testing of CRM-19813.

We're going to provide transitional wrapper function for `invoke()`, but we'll still need access to the original implementations. This renames the original implementations.

The `EventDispacherInterface` is widely-used across frameworks, and this makes it easier to lookup and autocomplete calls to the dispatcher.

The GenericHookEvent is used to expose all traditional hooks to the Symfony EventDispatcher. The traditional notation for a hook is based on a function signature: function hook_civicrm_foo($bar, &$whiz, &$bang); Symfony Events are based on a class with properties and methods. This requires some kind of mapping. Symfony Events has two conventions which might be used to support that mapping. One might implement event classes for every hook, or one might use the `GenericEvent`. This design-decision comes with a basic trade-off between size (total #files, #classes, #SLOC) and IDE assistance (docs/autocomplete): * `GenericEvent` has smaller size and less boiler-plate, but it also provides little IDE assistance. * Custom event classes provide more IDE assistance, but they also inflate the size (with lots of boilerplate). This patch implements `GenericHookEvent`, which is conceptually similar to `GenericEvent`, but it has a few modifications: * The `__get()` function returns references, which makes it easier to alter data. * The `getHookValues()` function returns an ordered list of hook arguments. The approach of `GenericEvent` / `GenericHookEvent` seems like a reasonable balance -- it starts out with little boilerplate, but we can incrementally introduce subclasses. The subclasses can: * Use docblocks for IDE support * Use declared properties for IDE support (though you may need to customize the constructor, etal). * Add semantic/businessy functions. * Override the `__get()` / `__set()` functions to be provide different getter/setter behavior.

There are a handful of events which have been dual-emitted by explicitly calling both the dispatcher and Hook::invoke(). The dispatcher now calls Hook::invoke automatically (if applicable), so we can omit that.

…l (11x)

…al (9x)

…tal (9x)

…al (10x)

…etal (10x)

…l (10x)

… (15x)

totten · 2017-03-31T03:19:20Z

Oop, found a problem. It arises when a hook's has an input $name like:

  public static function contactListQuery(&$query, $name, $context, $id) {

If someone tries to use a GenericHookEvent to access the corresponding $event->name, it leads to a conflict because the base class Event has its own private property named name.

A simple workaround is to change $name to something more precise. Fortunately, this should be safe because it does not change the ordering or semantics of hook params for existing listeners.

Traditionally, the names of the hook parameters were advisory/aesthetic. With the introduction of GenericHookEvent, they become first class symbols -- and they can theoretically conflict with properties of `GenericHookEvent` or `Event`. In particular, the parameter `$name` conflicts with `Event::$name` in Symfony 2.x.

totten · 2017-03-31T21:51:57Z

jenkins, test this please

eileenmcnaughton · 2017-04-11T01:46:21Z

CRM/Utils/Hook.php

@@ -298,10 +340,8 @@ public function requireCiviModules(&$moduleList) {
   */
  public static function pre($op, $objectName, $id, &$params) {
    $event = new \Civi\Core\Event\PreEvent($op, $objectName, $id, $params);
-    \Civi::service('dispatcher')->dispatch("hook_civicrm_pre", $event);
-    \Civi::service('dispatcher')->dispatch("hook_civicrm_pre::$objectName", $event);


what does this change do?

Good question. This change comes from c73e309.

The old lines were part of a quick-and-dirty attempt to support hook-events in core. (You might refer to them as dual-emit because they were emitted once through Symfony EventDispatcher and once through CMS.) However, that approach required a bunch of boilerplate: you write a new PHP file (Civi/Core/Event/PreEvent.php) for every hook, and then patch every hook to instantiate+dispatch it.

In the revised code, lines 333-335 are redundant. If you call dispatch('hook_foo', ...), then the event is dispatched everywhere. You don't need extra classes because all hooks can use GenericHookEvent.

There were 3 or 4 hooks which had the old boilerplate classes. I took an approach which felt more conservative -- the boilerplate classes are still there, but they now extend GenericHookEvent.

In retrospect, it would have been fair-game to entirely remove those boilerplate classes. Grepping universe, it appears that PreEvent (etal) have been internal to civicrm-core. But I don't mind the above because it's technically better refactoring technique.

eileenmcnaughton · 2017-04-11T01:46:42Z

CRM/Utils/Hook.php

+    }
+    else {
+      $count = is_array($names) ? count($names) : $names;
+      return $this->invokeViaUF($count, $arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $fnSuffix);


this is the legacy support

eileenmcnaughton · 2017-04-11T02:06:21Z

I've taken a look at this and feel it's OK to merge provided it gets some more testing in the rest of the release process. We are a good time in the cycle now. We do have reasonable in-core testing.

I'll test against the civixero extension which implements it's own hook extension, @seamuslee001 can you check on your multisite.

eileenmcnaughton · 2017-04-11T02:06:35Z

Note we should also notifiy the dev list

eileenmcnaughton · 2017-04-11T02:18:56Z

@nganivet can you please continue to keep doing testing on your todo list - albeit against master now

After merging civicrm#9949, some screens (like the contact-result list or "View Contact") reported warnings like: ``` Notice: Undefined index: in CRM_Core_BAO_Country::countryLimit() (line 90 of /home/foo/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/BAO/Country.php). Notice: Undefined index: in CRM_Core_BAO_Country::provinceLimit() (line 62 of /home/foo/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/BAO/Country.php). ``` Bisecting the git history revealed that it stemmed from switching `hook_civicrm_entityTypes` to go through EventDispatcher. civicrm@fb1d9ad#diff-8869a8f3c6318eb0580ce2aa04b713bfL1835 This hook is apparently similar to `hook_civicrm_container` in that both fires pre-boot. If we attempt to dispatch it through the container in a pre-boot environment, something initializes incorrectly. This change proposes a general rule: * If you fire a hook before the container or EventDispatcher is available... * Then don't try to use the container or EventDispatcher.

christianwach · 2017-04-11T12:31:31Z

@totten (Reposting here in case you miss my message on MM) On further testing, I notice that WordPress does not receive callbacks from hook_civicrm_entityTypes but I'm unable to trace why it's failing.

christianwach · 2017-04-11T12:37:12Z

@totten I presume from 4d8e83b that hook_civicrm_entityTypes needs special handling. I can see the successful callback in the output in my modified Show All The Hooks extension but only for the extension.

christianwach · 2017-04-12T11:14:27Z

@totten Here's what's going on:

When the code declaring the callback is in a Civi extension, the add_action() call declared there occurs after hook_civicrm_entityTypes has already fired. This accounts for the lack of activity in the callback.
When I declare a callback in an ordinary WordPress plugin, hook_civicrm_entityTypes is triggered and the callback receives data during a standard page load.

In summary - CiviCRM extensions cannot inspect $entityTypes via the add_action() method - they must use the Drupalesque function-name-as-callback method. Vanilla WordPress plugins can use add_action().

Note: I have only tested this with standard page loads - not via cron or any other entry route.

totten · 2017-04-12T22:47:54Z

Thanks again @christianwach. For anyone who stumbles across this, we chatted more over Mattermost, and I think that phenomenon makes sense -- you should pick the notation that is canonical for your package type. (For WP plugin, use WP notation. For Drupal module, use Drupal notation.)

civicrm-builder added the master label Mar 8, 2017

totten changed the title ~~(WIP) CRM-19813 - GenericHookEvent - Unify Symfony Events and hooks~~ (WIP) CRM-19813 - Unify Symfony Events and hooks Mar 8, 2017

totten force-pushed the master-hook-ev2 branch 2 times, most recently from f20dbe2 to 2e6d29d Compare March 8, 2017 13:25

totten force-pushed the master-hook-ev2 branch from 2e6d29d to 8ee1f25 Compare March 16, 2017 01:40

totten changed the title ~~(WIP) CRM-19813 - Unify Symfony Events and hooks~~ CRM-19813 - Unify Symfony Events and hooks Mar 24, 2017

totten added 17 commits March 30, 2017 15:39

CRM-19813 - CRM_Utils_Hook_* - Rename invoke() to invokeViaUF()

354345c

We're going to provide transitional wrapper function for `invoke()`, but we'll still need access to the original implementations. This renames the original implementations.

CRM-19813 - Civi::dispatcher() - Add lookup helper

9897fb1

The `EventDispacherInterface` is widely-used across frameworks, and this makes it easier to lookup and autocomplete calls to the dispatcher.

CRM-19813 - CRM_Utils_Hook - Cleanup existing dual-emit events

c73e309

There are a handful of events which have been dual-emitted by explicitly calling both the dispatcher and Hook::invoke(). The dispatcher now calls Hook::invoke automatically (if applicable), so we can omit that.

CRM-19813 - CRM_Utils_Hook::container() - Continue using legacy hooks

63b1853

CRM-19813 - CRM_Utils_Hook::invoke() - Use dispatcher when possible

e24a07e

CRM-19813 - CRM_Utils_Hook - Set names for links, preProcess, eta…

6a8f168

…l (11x)

CRM-19813 - CRM_Utils_Hook - Set names for managed, dashboard, et…

ba9d43d

…al (9x)

CRM-19813 - CRM_Utils_Hook - Set names for tokenValue, pageRun, e…

9c42b57

…tal (9x)

CRM-19813 - CRM_Utils_Hook - Set names for mailingTemplateTypes, et…

5b3f2d4

…al (10x)

CRM-19813 - CRM_Utils_Hook - Set names for config, optionValues, …

d128571

…etal (10x)

CRM-19813 - CRM_Utils_Hook - Set names for alterAPIPermissions, eta…

b95f41f

…l (10x)

CRM-19813 - CRM_Utils_Hook - Set names for alterReportVar, etal (10x)

027e669

CRM-19813 - CRM_Utils_Hook - Set names for entityTypes, etal (10x)

fb1d9ad

CRM-19813 - CRM_Utils_Hook - Set names for dashboard_defaults, etal…

5d4ba3b

… (15x)

(NFC) GenericHookEvent - Update docblocks

0d68139

totten force-pushed the master-hook-ev2 branch from f84441a to 0fcad35 Compare March 31, 2017 04:59

totten mentioned this pull request Mar 31, 2017

CRM-20600 - AngularJS - Allowing hooking and static-caching #10085

Merged

eileenmcnaughton reviewed Apr 11, 2017

View reviewed changes

eileenmcnaughton merged commit 66390ef into civicrm:master Apr 11, 2017

totten deleted the master-hook-ev2 branch April 11, 2017 03:05

totten mentioned this pull request Apr 11, 2017

CRM-19813 - Only dispatch through EventDispatcher after booting #10138

Merged

This was referenced Apr 13, 2017

use of Symfony EventDispatcher instead of CRM_Utils_Hook civicrm/org.civicrm.flexmailer#1

Closed

CiviEventInspector - Add util for displaying hook metadata #10161

Merged

totten mentioned this pull request Oct 24, 2017

CRM-21341 Drupal 8 Hook Support #11171

Merged

totten mentioned this pull request Nov 9, 2017

Convert from civi.flexmailer.* to hook_civicrm_* (#1) civicrm/org.civicrm.flexmailer#7

Closed

davialexandre mentioned this pull request Nov 16, 2017

PHCR-2647: Merge PHP content compucorp/civihr#2274

Closed

MegaphoneJon mentioned this pull request Sep 20, 2018

Replace key callbacks with Symfony listeners civicrm/org.civicrm.shoreditch#316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-19813 - Unify Symfony Events and hooks #9949

CRM-19813 - Unify Symfony Events and hooks #9949

totten commented Mar 8, 2017 •

edited

Loading

nganivet commented Mar 8, 2017

nganivet commented Mar 8, 2017

totten commented Mar 24, 2017 •

edited

Loading

nganivet commented Mar 26, 2017

totten commented Mar 31, 2017 •

edited

Loading

totten commented Mar 31, 2017

eileenmcnaughton Apr 11, 2017

totten Apr 11, 2017 •

edited

Loading

eileenmcnaughton Apr 11, 2017

eileenmcnaughton commented Apr 11, 2017

eileenmcnaughton commented Apr 11, 2017

eileenmcnaughton commented Apr 11, 2017

christianwach commented Apr 11, 2017

christianwach commented Apr 11, 2017

christianwach commented Apr 12, 2017

totten commented Apr 12, 2017

CRM-19813 - Unify Symfony Events and hooks #9949

CRM-19813 - Unify Symfony Events and hooks #9949

Conversation

totten commented Mar 8, 2017 • edited Loading

nganivet commented Mar 8, 2017

nganivet commented Mar 8, 2017

totten commented Mar 24, 2017 • edited Loading

nganivet commented Mar 26, 2017

totten commented Mar 31, 2017 • edited Loading

totten commented Mar 31, 2017

eileenmcnaughton Apr 11, 2017

Choose a reason for hiding this comment

totten Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Apr 11, 2017

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 11, 2017

eileenmcnaughton commented Apr 11, 2017

eileenmcnaughton commented Apr 11, 2017

christianwach commented Apr 11, 2017

christianwach commented Apr 11, 2017

christianwach commented Apr 12, 2017

totten commented Apr 12, 2017

totten commented Mar 8, 2017 •

edited

Loading

totten commented Mar 24, 2017 •

edited

Loading

totten commented Mar 31, 2017 •

edited

Loading

totten Apr 11, 2017 •

edited

Loading