CRM-19813 - Unify Symfony Events and hooks #9949
Conversation
|
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)? |
|
Also ... what are the documentation changes required for this? Do we need to add a section with the additional events now available to extensions? |
f20dbe2 to
2e6d29d
Compare
|
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:
In my ideal world, our stack of cheese slices would include somewhere within it:
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. |
|
@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.
|
Oop, found a problem. It arises when a hook's has an input public static function contactListQuery(&$query, $name, $context, $id) {If someone tries to use a A simple workaround is to change |
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.
|
jenkins, test this please |
| @@ -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); | |||
There was a problem hiding this comment.
what does this change do?
There was a problem hiding this comment.
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.
| } | ||
| else { | ||
| $count = is_array($names) ? count($names) : $names; | ||
| return $this->invokeViaUF($count, $arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $fnSuffix); |
There was a problem hiding this comment.
this is the legacy support
|
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. |
|
Note we should also notifiy the dev list |
|
@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.
|
@totten (Reposting here in case you miss my message on MM) On further testing, I notice that WordPress does not receive callbacks from |
|
@totten I presume from 4d8e83b that |
|
@totten Here's what's going on:
In summary - CiviCRM extensions cannot inspect Note: I have only tested this with standard page loads - not via cron or any other entry route. |
|
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.) |
Civi has three notions of events:
This PR aims to unify two of them -- hooks and Symfony Events. More specifically:
hook_civicrm_fooand (b) the event object extendsGenericHookEvent. 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:
civicrm_api(...)) and can be used in most PHP contexts, but some variants (like Smarty or CLI) can't easily support 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:
cv debug:event-dispatcherto inspect the list of event listeners.GenericHookEventis included in the relevant commit ("GenericHookEvent - Bridge between Symfony Events and hooks").CRM_Utils_Hook. I've adapted ~1/3 so far.