Problem/Motivation

Module authors converting their procedural code to services would like to implement hooks in said services without procedural code.

Proposed resolution

One, the event bridge. In ModuleHandler:

public function invokeAll($hook, $args = array()) {
  $event = new HookEvent("hook.$hook", $args)
  \Drupal::service('event_dispatcher')->dispatch($event);
  $return = $event->getResults();
// ... the rest of the function is unchanged.
}

this way people can write dependency injected services implementing a hook instead of procedural code. As we hope to attract new developers more accustomed to OOP, this is sort of important.

Alternatively we could put the hook definitions in a class like Drupal\System\HookDefinition\System, one method per hook with the doxygen and with empty bodies. Classes would extend these definitions, register themselves into the service container with the tag hook. On container compile, we could parse the classes of these services with existing Doctrine code to find the methods defined by them and store the hook => array-of-services list into the container and then invokeAll:

public function invokeAll($hook, $args = array()) {
  $result = array();
  $hook_method_name = Camelize($hook_name);
  foreach (\Drupal::container()->getParameter("hook.$hook_name") as $service) {
     // We could do something with the return value as well.
     call_user_func_array(array(\Drupal::service($service), $hook_method_name), $args);
  }
// ... the rest of the function is unchanged.
}

This would have the advantage of a) we could retire hooks even for D8 because we have a place where we can put the hook doxygen B) we never need to load all the hook definitions, unlike with the event bridge. It's a regression to load all that code, in D7 we added groups to hooks to avoid that. The main "against" here is that this not the familiar (?) event dispatcher but then again, the only difference here is that instead of tagging as 'event_subscriber' we need to tag with 'hook' and then subscription is automatic. Already the event_subscriber tag is a Drupalism, this isn't a big step ahead.

Remaining tasks

Update module handler as required
Create HookEvent
Convert remaining hook invocations that do not use ModuleHandler::invokeAll ie where module_implements is used instead. There are two kinds: one that wants to pass around references, these are easy to convert because invokeAll takes an array of arguments instead of a variable number of arguments and the array simply can contain the references. Those that want to record the module that returned certain information will need some consideration (permissions do this for uninstall). The problematic cases will likely be handled in a followup.

User interface changes

None

API changes

Module maintainers can implement hooks in their service classes.
ModuleHandler::moduleImplements likely becomes a protected method.

#1972300: Write a more scalable dispatcher

CommentFileSizeAuthor
#51 drupal_1972304_51.patch13 KBXano
#51 interdiff.txt1.42 KBXano
#47 drupal_1972304_47.patch13.68 KBAaronBauman
#43 drupal_1972304_43.patch12.84 KBXano
#40 drupal_1972304_40.patch9.64 KBXano
#40 interdiff.txt415 bytesXano
#37 drupal_1972304_37.patch9.78 KBXano
#17 1972304-hook-events.patch8.74 KBCrell
#14 1972304-hookevent-test-do-not-test.patch1007 bytescweagans
#14 with-eventdispatcher.d8.xhprof.txt826.5 KBcweagans
#14 without-eventdispatcher.d8.xhprof.txt824.47 KBcweagans
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

Updated issue summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue summary: View changes

Updated issue summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I'm unsure about "ModuleHandler::moduleImplements likely becomes a protected method" but otherwise, I think this is a very good idea, at least good enough to see some code!

chx’s picture

chx CreditAttribution: chx commented

You wont be able to call module_implements, it'll become an internal implementation detail. invokeAll (module_invoke_all) is the way to call every implementation of a hook.

Edit: this is the goal. Not necessarily in the first round.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

Updated issue summary.

Crell’s picture

Crell CreditAttribution: Crell commented

This sounds quite promising to me, and I'd love to see code for it. +1

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I would love to see that happening too.

What would work very nicely is:

  $event = new HookEvent($hook, $args);
  $dispatcher->dispatch($hook, $event);

So that you can implement a subscriber like:

class Subscriber implements EventSubscriberInterface {
  public function onUserUpdate(HookEvent $event) {
    //
  }

  static function getSubscribedEvents() {
    $events['user_update'][] = array('onUserUpdate', 50);
    return $events;
  }
}

From a consumer perspective, that's lovely.

chx’s picture

chx CreditAttribution: chx commented

Alternative proposal: the event dispatcher model does not have a place to put documentation. So, let's change system.api.php into Drupal\System\HookDefinition\System.php or something like that and have classes extending those. The class would have methods with doxygen and empty function bodies. The implementation class would look like this:

namespace Drupal\modulename\Hook;

use Drupal\System\HookDefinition\System as SystemDefinition;

class System extends SystemDefinition {
  function pageBuild(&$page) {
   ....
  }
}

So one implementation class per defining module -- views hook implementations in one class, taxonomy hook implementations in another etc. These classes would need to be registered into the container and tagged as hook.

So then we have a list of classes based on the tagged service definitions. We can use core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.php to find the methods therein and record an array ($hook_name => array($list_of_services)) parameter into the container much like the module_implements cache now.

This does not require including all the implementation classes in one go, ever which is also a distinct and important advantage over eventdispatcher.

Crell’s picture

Crell CreditAttribution: Crell commented

Actually the convention in some parts of the Symfony world, which we've been sort of adopting, is to define a constant class that has the event names in it and documentation. Cf:

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...

(Which is modeled on the KernelEvents class from HttpKernel.)

I think that works well enough on the documentation front.

"One class for all hooks" is a terrible hack of OO design, and not one I would support. I much prefer the original proposal. It's simple, flexible, and clean.

chx’s picture

chx CreditAttribution: chx commented

It's not one class for all hooks, it's one class per defining module. That's extensible and much smaller.

Where do you plan to put http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... in the event subscriber model?

Crell’s picture

Crell CreditAttribution: Crell commented

That's still less clean and less flexible than the original proposal. EventSubscribers let you put multiple listeners in the same object instance however you want, in whatever way makes sense for you.

And I think hook_page_build() is already slated for execution by SCOTCH... It would be replaced by a combination of a view listener and a wrapping _controller (like the kind we're wiring up with route enhancers now).

I still like #0/#4 better. (I'd prefix the hook name with hook. when turning it into an event to avoid namespace collisions, but that's a minor quibble.)

chx’s picture

chx CreditAttribution: chx commented

That's dodging the question, I could've picked any other hook. hook_help, hook_forms, hook_openid whatever. Where do you plan to put their doxygen?

chx’s picture

chx CreditAttribution: chx commented

To sum up: the class version allows for total removal of hooks, in D8 or D9 doesn't matter because it gives you a point to hold the doxygen. A move from .module to class is totally painless, it's just moving the function and changing the function name. It's much easier to implement a hook because your IDE will directly link you to the API documentation in the class parent. Finding hook implementations is the same as before -- you were searching for ^function.*_hook_name or somesuch, now it's just function hookName. It enforces modules firing hooks to document their hooks. It's all advantages. The disadvantages are minor: there's a little magic here in invoking these classes but it's truly minimal, if you get the EventDispatcher, you will understand how these are fired: if you tag a service 'event_subscriber' you need to manually register your events, if you tag something 'hook' the it registers the methods automatically

The event bridge has no point to put the doxygen on. It regresses from Drupal 7 that at one point you need to load all the hook implementations to find what implements what. In D7 we added grouping to avoid this. To find a hook implementation, you need to grep for the constant and hope it's only used in the getSubscribers method. A move from .module to .class requires the explicit registration of the methods. And the method names will never be consistent and your IDE won't help at all in what to name them or what arguments they get.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

updated issue summary

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

added class method

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +DX (Developer Experience), +Increases learning curve, +Stalking Crell

Some tags.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

We discussed this issue yesterday with @chx, @Dries, @effulgentsia, @Moshe Weitzman, and myself. Here's Dries' summary of the approach to take, based on where we are currently at in the release cycle:

Step 1: do we want event-only Drupal in the long-term? If so:
Step 2: can we do a BC layer in Drupal 8 that is performant?
If the answer to Step 2 is 'no' -> postpone to D9
If the answer to Step 2 is 'yes' -> ship D8 with that BC layer

Crell’s picture

Crell CreditAttribution: Crell commented

Step 1: Yes.
Step 2: Needs benchmarks to know for sure. The overhead for a no-implementation event is, I think, fairly low, but it's there.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented
FileSize
without-eventdispatcher.d8.xhprof.txt824.47 KB
with-eventdispatcher.d8.xhprof.txt826.5 KB
1972304-hookevent-test-do-not-test.patch1007 bytes

I just did some quick profiling, and IMO, we should fire an EventDispatcher event every time a hook is invoked, which basically results in a choice between our normal hook system and eventdispatcher.

xhprof files for before and after the patch are attached.

Summary: looks like it only adds 30k of memory usage and an extra ~350 function calls to call event dispatcher for invokeAll() call (and this is obviously a no-implementation event - just using the built-in GenericEvent for the sake of testing).

Based on that, can we move forward here?

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

Also, those were run in a fresh install of a Drupal 8 site (installed with Standard) on the front page. No content, one user, etc.

chx’s picture

chx CreditAttribution: chx commented

I think we could move forward here -- if many implementations turn out to be slow(er) than we will get #1972300: Write a more scalable dispatcher done.

Crell’s picture

Crell CreditAttribution: Crell commented
FileSize
1972304-hook-events.patch8.74 KB

Here we go with take 1.

This patch adds a HookEvent, and fires it from invokeAll(). It also introduces a mechanism to handle returns from hook event listeners, so that we have parity with the current capabilities of hooks. These work in both the installer and normal runtime (ooo...). I then converted history_cron() to a listener. history_cron() is not exactly the best candidate for doing so, but it was dead-simple so allowed me to exercise the code a little. Other hooks could easily be converted to the same subscriber class, or history could get refactored to be a service that the events just talk to, or whatever. That's all out of scope for now. :-)

While I was at it, I fixed a bug in the parameter order for CachedModuleHandler's constructor, which was buggy. (Optional parameters must come at the end.)

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Active » Needs review
chx’s picture

chx CreditAttribution: chx commented

That's a beginning, however, the big work here is getting rid of the module_implements loops which cweagans have voluntereed for (thanks!)

Also if you'd fire the event first you wouldn't need to merge it back.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented
Assigned: Unassigned » cweagans

Per IRC convo w/ chx and Crell, I'll be working on this for a few days (I'll have a solid block of hours to spend on it on Monday night).

Status: Needs review » Needs work

The last submitted patch, 1972304-hook-events.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

The approach in #17 fires the event after running all of the procedural implementations. #19 suggests to do it before. In either case though, if a developer chooses to use an event subscriber vs. a procedural implementation, they're not just making a stylistic choice, they're taking their implementation out of the expected per-module-weight order. I think that will create a lot of problems in the wild.

Related to this, why bridge in this direction? Wouldn't it make more sense to bridge the other way? Make callers of $moduleHandler->invokeAll() instead call $eventDispatcher->dispatch()? And create a Drupal-specific dispatcher that can invoke procedural implementations intermixed by priority/weight with OO subscribers?

Crell’s picture

Crell CreditAttribution: Crell commented

effulgentsia: To answer the second question, I worry about the performance impact. invokeAll() is currently faster than dispatch(). We have ideas (see links above) for how to make dispatch faster, but since at least for now most modules will be using hooks, not hook events, that would be additional overhead. Also, changing every instance of $moduleHandler->invokeAll() to $dispatcher->dispatch() would be a lot of work when we're trying to minimize the amount of non-critical work we have to do.

As far as the order, I don't think it's as big a deal as you make it out to be. Most hook implementations are order-agnostic. When they're not, there's often a double-hook setup to use instead (hook_info / hook_info_alter, etc.). Leveraging module weights is rare; and in those cases that you care, events offer more fine-grained order control. The only place it would be an issue is "I have to run before module X's implementation", in which case you'd have to use a hook, no event. I think that's less common than "I have to run after module X's implementation", so it's better to make "before" the hook-only option rather than vice versa.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented
Assigned: cweagans » Unassigned

So, some things came up and I'm definitely not going to have time to tackle this. If somebody wants to work through it, here's a todo list:

☐ core/modules/user/user.admin.inc: foreach (module_implements('permission') as $module) {
☐ core/modules/user/user.module: foreach (module_implements('permission') as $module) {
☐ core/modules/user/user.module: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.php: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/filter/Permissions.php: foreach (module_implements('permission') as $module) {
☐ core/modules/user/lib/Drupal/user/Plugin/views/access/Permission.php: foreach (module_implements('permission') as $module) {
☐ core/modules/image/image.module: foreach (module_implements('image_effect_info') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeFormController.php: foreach (module_implements('node_validate') as $module) {
☐ core/modules/node/lib/Drupal/node/NodeFormController.php: foreach (module_implements('node_submit') as $module) {
☐ core/modules/node/node.module: foreach (module_implements('node_info') as $module) {
☐ core/modules/search/search.module: foreach (module_implements('search_info') as $module) {
☐ core/modules/search/search.module: foreach (module_implements('search_preprocess') as $module) {
☐ core/modules/action/tests/action_loop_test/action_loop_test.module: foreach (module_implements('watchdog') as $module) {
☐ core/modules/rdf/rdf.install: $modules = module_implements('rdf_mapping');
☐ core/modules/rdf/rdf.module: foreach (module_implements('rdf_namespaces') as $module) {
☐ core/modules/rdf/rdf.module: $modules = module_implements('rdf_mapping');
☐ core/modules/file/file.module: foreach (module_implements('file_download_access') as $module) {
☐ core/modules/help/help.module: $modules = module_implements('help');
☐ core/modules/help/lib/Drupal/help/Tests/HelpTest.php: foreach (module_implements('help') as $module) {
☐ core/modules/field/field.crud.inc: foreach (module_implements('field_attach_purge') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_form') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_load') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_validate') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_attach_extract_form_values') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_insert') as $module) {
☐ core/modules/field/field.attach.inc: foreach (module_implements('field_storage_pre_update') as $module) {
☐ core/modules/field/field.module: $modules = array_merge(module_implements('field_info'), module_implements('field_widget_info'));
☐ core/modules/field/field.module: foreach (module_implements('field_access') as $module) {
☐ core/modules/field/field.info.inc: foreach (module_implements('field_info') as $module) {
☐ core/modules/field/field.info.inc: foreach (module_implements('field_storage_info') as $module) {
☐ core/modules/filter/filter.module: foreach (module_implements('filter_info') as $module) {
☐ core/modules/system/system.module: foreach (module_implements($hook) as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.php: foreach (module_implements('theme') as $module) {
☐ core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php: foreach (module_implements('theme') as $module) {
☐ core/lib/Drupal/Core/Plugin/Discovery/InfoHookDecorator.php: foreach (module_implements($this->hook) as $module) {
☐ core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.php: foreach (module_implements($this->hook) as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php: foreach (module_implements('entity_load') as $module) {
☐ core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php: foreach (module_implements($this->entityType . '_load') as $module) {
☐ core/includes/common.inc: foreach (module_implements('cron') as $module) {
☐ core/includes/common.inc: foreach (module_implements('page_build') as $module) {
☐ core/includes/schema.inc: foreach (module_implements('schema') as $module) {
☐ core/includes/theme.inc: foreach (module_implements('theme') as $module) {
☐ core/includes/bootstrap.inc: foreach (module_implements('watchdog') as $module) {
☐ core/includes/menu.inc: foreach (module_implements('help') as $module) {
☐ core/includes/menu.inc: foreach (module_implements('menu') as $module) {

Some of those are going to be really tricky. It might be worth getting a few people together around a table at Drupalcon and just churn through the list.

Crell’s picture

Crell CreditAttribution: Crell commented

I'm unclear on how HookEvent as currently imagined would help with that situation... Can someone clarify?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Yeah I also don't see the connection. Cleaning up hook invocations to use the ModuleHandler is a separate issue and something that should be done anyway.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I worry about the performance impact. invokeAll() is currently faster than dispatch().

I worry about that too, but this issue suffers from that no matter what, since once we make invokeAll() call dispatch(), then it will suffer the performance of it. #14 indicates that it's not a problem for a home page with no content: I don't yet know how that will translate to other scenarios.

As far as the order, I don't think it's as big a deal as you make it out to be. Most hook implementations are order-agnostic. When they're not, there's often a double-hook setup to use instead (hook_info / hook_info_alter, etc.).

So are we saying we'll leave ModuleHandler::alter() not dispatching an event? Wouldn't the same motivations for using event subscribers instead of procedural code (DI, stylistic preference) apply to _alter() hooks too? And it is extremely common for modules to need to control order of _alter() hooks. The idea that as soon as ModuleX chooses to change their procedural _alter() implementation into an OOP one that ModuleY is then also required to do the same, just to be able to run later, is both unfair to ModuleY (hey, don't make me change my simple function into a class with a ton of boilerplate code) and to ModuleX (hey, I'd like to benefit from DI in this implementation, but wait, if I make the change, it might break all sites using this module in conjunction with any 1 of 10,000 modules that might have a procedural alter implementation that is currently running after mine).

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented

From IRC:

chx, I wasn't sure how to respond to Crell here: http://drupal.org/node/1972304#comment-7409118 I figured that was something that was already hashed out somewhere as part of a longer-term goal and that we were just doing it in that issue. Is that something you can elaborate on?
http://drupal.org/node/1972304 => Add a HookEvent #1972304: Add a HookEvent => Drupal core, base system, normal, needs work, 27 comments, 19 IRC mentions
cweagans: if we put the hookevent in invokeall then we need all hooks call invokeall
cweagans: those that use module_implements are exempt currently
cweagans: that's not good
Oh! Right, I see
Okay. That was the missing part

Crell’s picture

Crell CreditAttribution: Crell commented

Ah, I see. Although the reason those cases bypass invokeAll() is because they care about the module that is responding. I don't see how "just" using an event would help that situation. If anything it means we need to change the way the event works to track that, which is quite problematic since events are not 1:1 coupled to modules by design. (One module can have a dozen different listener methods in 3 different subscribers for the same event if it felt like it.)

Alex: I think I see what you're getting at; yes we should be adding events to alter(), too. (I thought it wasn't needed as that method would just wrap invokeAll(), but apparently I was wrong.) I don't know how you suggest to resolve the ordering question, though. Interweaving hooks and events together would be horribly complex since their ordering mechanisms are so vastly different. We've run into such before/after questions before (does form_FORM_ID_alter run before or after form_alter?), and the answer has always been "meh, pick one and deal with it." I see no way that we could do better than that here. Certainly I'd rather have that problem than not get HookEvent in at all because we couldn't figure that out.

IMO, event-second makes more sense (as I explained above) but if we end up with event-first, it's not the end of the world. *shrug*

chx’s picture

chx CreditAttribution: chx commented

> Although the reason those cases bypass invokeAll() is because they care about the module that is responding.

Sigh. I wish people would read the issue summary. There are *extremely* few that care about the module, the real reason those happen is because you can't do module_invoke_all('hookname', &$a) but you can do ->invokeAll('hookname', array(&$a)) because invokeAll happens to use an array and arrays are exempt from call-time-reference...

Those that do care about which module will need to be dealt with in a followup. I think that's user permissions anyways... and perhaps the theme registry.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I discussed this with chx a week or so ago and I'm still not sure about it. From performance, to dealing with dynamic hooks, to what extent it matters that people get a choice between events and hooks when building 8.x modules.

Overall the procedural code that concerns me in 8.x is the mountains of stuff still left in /core/includes, let alone all the procedural API functions left in modules.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I agree with catch. Furthermore, there can be a D8 contrib module that swaps out the module_handler service to one that dispatches events, and it can then iterate on thorny questions like invocation order. The only thing that would sway me to think this is worth doing in D8 core is if we have real use cases within core where hook implementations are problematic (e.g., we need DI or something like that). Though, since we can have hook implementations invoke services, I'm not aware of where we have a DI need that can't be solved that way.

I also agree with #26 that we should clean up all hook invocations to actually use invokeAll() or alter(), rather than custom iterating function names in all the places identified in #24.

Crell’s picture

Crell CreditAttribution: Crell commented

The big impetus for this issue is DX: Telling devs "sometimes you have to go all OO... other times you have to go PHP 4 procedural, have fun" is unkind. Bridging hooks to events lets people who want to just stay OO stay OO for a much bigger percentage of the time.

I'd love to see /includes turn into services; some of it has open issues to do that, much doesn't. I haven't had the bandwidth to dig into that rats nest yet except where it touches on other things I'm working on (eg, current_path(), etc.). That's separate from this issue, though.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If you're writing a completely self-contained custom module then you'd be able to stick to OO. Well except for all the places which would still be procedural even with this change like preprocess.

If you're working on a complex site or patching contrib modules then you'll have to deal with "sometimes this happens in a hook, sometimes it happens in an event" for exactly the same thing, that doesn't feel like an improvement.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
Issue tags: -DX (Developer Experience), -Increases learning curve, -Stalking Crell

#17: 1972304-hook-events.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +Increases learning curve, +Stalking Crell

The last submitted patch, 1972304-hook-events.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_1972304_37.patch9.78 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, drupal_1972304_37.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
+++ b/core/modules/history/history.services.yml
@@ -0,0 +1,4 @@
+    class: Drupal\history\EventSubscriber\CoreSubscriber

+++ b/core/modules/history/lib/Drupal/history/EventSubscriber/CronSubscriber.php
@@ -0,0 +1,53 @@
+namespace Drupal\history\EventSubscriber;
...
+class CronSubscriber implements EventSubscriberInterface {

it is either CoreSubscriber or CronSubscriber:P

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
interdiff.txt415 bytes
drupal_1972304_40.patch9.64 KB

Status: Needs review » Needs work

The last submitted patch, drupal_1972304_40.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +Needs tests 
+++ b/core/modules/history/history.module
@@ -120,15 +120,6 @@ function history_write($nid, $account = NULL) {
-function history_cron() {

+++ b/core/modules/history/history.services.yml
@@ -0,0 +1,4 @@
+services:
+  history.cron:

This example is not good because we have no tests for the hook at all

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

Updated issue summary.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
drupal_1972304_43.patch12.84 KB

Re-roll. I also chose to test the patch by adding a unit test for the event and extending the one for ModuleHandler instead of changing the History module.

Next to that, I renamed HookEvent::AddToReturn() to HookEvent::setReturnValue() and HookEvent::getReturn() to HookEvent::getReturnValues() to be more self-explanatory and consistent with other methods within Drupal.

Status: Needs review » Needs work

The last submitted patch, 43: drupal_1972304_43.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review

43: drupal_1972304_43.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 43: drupal_1972304_43.patch, failed testing.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman commented
Status: Needs work » Needs review
FileSize
drupal_1972304_47.patch13.68 KB

#43 doesn't apply cleanly
here's a re-roll to address changes in LocalTaskIntegrationTest.php and fuzz

Status: Needs review » Needs work

The last submitted patch, 47: drupal_1972304_47.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review

47: drupal_1972304_47.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 47: drupal_1972304_47.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.42 KB
drupal_1972304_51.patch13 KB

The problem was an incorrect argument order for CachedModuleHandler. This caused a class instantiation error, to which Drupal responded by loading the maintenance theme, which in turn needed the module handler as well.

Status: Needs review » Needs work

The last submitted patch, 51: drupal_1972304_51.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

The exceptions are caused by #2170989: Migrate provides invalid arguments when invoking hooks.

I just realized that this patch will break \Drupal\Core\Extension\ModuleHandlerInterface::implementsHook(), as it allows modules to respond to hook invocations without having a procedural hook implementation. We may have to make \Drupal\Core\Extension\ModuleHandler::implementsHook() check all event subscriber services and see what events they subscribe to.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

As described in the issue summary, this patch adds events to invokeAll(), and we can add them to alter() too. However, there is no way to add them to invoke() without breaking BC. invoke() expects a single return value, but allowing events there lets modules respond to the hook using a procedural implementation and an event listener, which means the return value potentially is two values instead.

Then there is the question of \Drupal\Core\Extension\ModuleHandlerInterface::getImplementations() and \Drupal\Core\Extension\ModuleHandlerInterface::implementsHook(), both of which associate procedural hook implementations with modules. Any code that relies on these may incorrectly assume that no module implements a particular hook, while all implementations are event listeners.

The most straightforward solution is to get rid of any functionality that deals with module-specific hook implementations. These include:

  • \Drupal\Core\Extension\ModuleHandlerInterface::invoke()
  • \Drupal\Core\Extension\ModuleHandlerInterface::getImplementations()
  • \Drupal\Core\Extension\ModuleHandlerInterface::implementsHook()
  • \Drupal\Core\Extension\ModuleHandlerInterface::()

Any code that needs to implement a specific hook implementation either shouldn't use hooks at all (because the hook API is abused to support magic callbacks), or require return values to contain a module key that specifies the module the values belong to.

This change would break backwards compatibility, but it does allow us to convert the thousands of lines of hook implementations we have in core, and more in contrib, to unit-testable OOP code.

My proposal is that we either do not introduce events at all, or we go all the way and make sure events can be fired for any hook implementation.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

cweagens grepped for it before: https://drupal.org/comment/7408792#comment-7408792

In general we certainly have to evaluate the performance of the event system. Let's be honest. It is NO big deal to call from hooks to services.

Crell’s picture

Crell CreditAttribution: Crell commented

Perhaps we're going about this all wrong.

The long-term goal is to transition "event-ish" hooks away from hooks and toward the event system, and do so in a gradual way rather than a hard "Break all the things!" approach. Ideally, during at least some of the Drupal 8 cycle module developers could use either hooks or events to accomplish various tasks, and in Drupal 9 the events would keep working exactly as they already were while the hooks would go away. That would give people a LONG time to get around to converting their modules with both forward and backward compatibility.

That goal doesn't require the hook system or the event system to transparently "front" for the other. That's one way of doing it, but not the only.

Do we really *need* events to show up in implementsHook()? What do we actually get out of that? I think, as a practical matter, we probably don't need that.

Many modules now have a hook invocation, followed by a rules invocation. Two event systems that fire in parallel, but independently of each other. Rules implementations don't show up in module_implements(), and module implementations don't show up in a list of rules implementations. Why not just treat events the same way?

So take, eg, the entity event hooks. Just stick a call to an entity event next to them, design that the way we want it to be in both D8 and D9, and call it a day. Don't try to sledge-hammer port everything in one big BC-layer patch. Let them coexist peacefully but clearly communicate the longer term plan.

Or if we really want, we can centralize the event calls but accept that it's not going to be perfect parity... and that's OK. We don't care that an event listener doesn't look exactly like a hook implementation to the hook system. And that's OK.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

Do we really *need* events to show up in implementsHook()? What do we actually get out of that? I think, as a practical matter, we probably don't need that.

[...]

Or if we really want, we can centralize the event calls but accept that it's not going to be perfect parity... and that's OK. We don't care that an event listener doesn't look exactly like a hook implementation to the hook system. And that's OK.

IMHO having events for some, but not all hook invocations is confusing and causes WTFs. It's not a disaster, but it will waste people's time because of the inconsistency and it will only be a partial solution anyway.

Many modules now have a hook invocation, followed by a rules invocation. Two event systems that fire in parallel, but independently of each other. Rules implementations don't show up in module_implements(), and module implementations don't show up in a list of rules implementations. Why not just treat events the same way?

This is fine with me, but will not really aid in the smooth transition that this issue aims to accomplish: offer support for both hooks and events without anyone having to think about it.
If developers simply want dependency injection and the ability to unit test their code, @dawehner suggested they can create a service with 'hook implementations'. The real hook implementations will then be no more than one-line procedural wrappers for methods on services.

chx’s picture

chx CreditAttribution: chx commented

See the original post of #1331486: Move module_invoke_*() and friends to an Extensions class for the original vision on how these thing should look. The plan was to have invoke replace invokeAll and instead of getting a full list of implementation, just a isImplemented was to be exposed. If #1972300: Write a more scalable dispatcher happens then the isImplemented is very easy to do.

Crell’s picture

Crell CreditAttribution: Crell commented

Xano: My point is that "without anyone having to think about it" ma be a pipe dream, given the oddities around implementation, ordering, discovery, etc. We may be better off in the long run not pretending that they're the same system, just supporting both in parallel for a time.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Priority: Normal » Major
Issue tags: +beta target

Whenever I travel to an event and talk to a non-Drupal developer, everything about D8 makes sense to them except for hooks. I'd like to escalate the importance of this issue, or something like it, that allows us to introduce a transitional step that makes it easier for non-Drupal developers to approach our event system while not requiring re-writing all of core.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

I personally started accepting we won't be getting this in core, as we cannot reliably introduce this feature for all hooks. Introducing it partially will allow people to write event listeners (even though the events will not be properly classed) for some hooks, but it will cause developers having to look up which hook invocations support the event and which do not. This also limits us in refactoring the hook code, as the event integration does not depend on the hook itself, but on how it is invoked.

I do not mean to scare people away from this issue, but if there is indeed no way around these limitations, our time might be better spent introducing dedicated (classed) events for the most important hook invocations.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

Also:

introduce a transitional step that makes it easier for non-Drupal developers to approach our event system

It is not our event system, but Symfony's, which uses specific classes for events. Introducing one single HookEvent class will technically work, but it will still require developers to read the hook documentation in order to figure out what the event does, as we cannot document that on the class. Apart from being a partial solution, this is also not fully in line with how Symfony does things.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Related issues: +#2237831: Allow module services to specify hooks
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Whenever I travel to an event and talk to a non-Drupal developer, everything about D8 makes sense to them except for hooks. I'd like to escalate the importance of this issue, or something like it, that allows us to introduce a transitional step that makes it easier for non-Drupal developers to approach our event system while not requiring re-writing all of core.

This won't reduce that confusion though. It'll just mean that some code doing exactly the same thing lives in hook implementations, while some lives in events.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

Also, if you just want dependency injection and testability, you can do something like http://drupalcode.org/project/payment.git/blob/refs/heads/8.x-2.x:/payme... and http://drupalcode.org/project/payment.git/blob/refs/heads/8.x-2.x:/payme....

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Closed (won't fix)
benjy’s picture

benjy CreditAttribution: benjy commented

Couldn't this still go in for 8.1?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Closed (won't fix) » Postponed

We can at least talk about it again then.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans as a volunteer commented
Parent issue: » #2616814: Delegate all hook invocations to ModuleHandler

This issue can't be completed in a reasonable way without solving #2616814: Delegate all hook invocations to ModuleHandler first.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

I'm experimenting with this in contrib if anyone wants to give me any feedback: http://drupal.org/project/hooks

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Issue tags: -beta target +Needs issue summary update

Tagging for issue summary update. The patch/title here is adding an event, but the issue summary talks about tagged services which is what's being discussed over at #2237831: Allow module services to specify hooks.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Postponed » Active
Issue tags: -Stalking Crell

8.1 is long out

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

olexyy.mails@gmail.com’s picture

olexyy.mails@gmail.com CreditAttribution: olexyy.mails@gmail.com commented

Please look at:
https://www.drupal.org/sandbox/olexyymailsgmailcom/hook_manager

- it should be rather fast;
- it uses caching and static;
- priority is supported;
- we have progress in naming convention problems;
- we can inject services to classes that implement hooks;

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented

#78: How does this differ from #70?

olexyy.mails@gmail.com’s picture

olexyy.mails@gmail.com CreditAttribution: olexyy.mails@gmail.com commented

Technically differes from#70, this is not events but plugins. And yes, bonuses are the same plus performance.

Internally it works almost luke existing solution.

My implementation supports invoke all as well as alter.
This is oop hook implementation which should be faster than events.
In my module new hooks are defined and can be added as plugins, like blocks or entities.

The idea is rather simple:
Annotation to plugin contains list of canonical names for hooks with corresponding priority. (f.e. hook_theme => 0)
We need to implement this hook as method in camel case, so this is public function hookTheme();

This approach can be additional to main implementation, that makes system very suite and flexible.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

No Sssweat’s picture

No Sssweat CreditAttribution: No Sssweat commented

You might find this contrib module handy, Hook Event Dispatcher.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Consensus Enterprises, Colan Schwartz Consulting commented

#71: Should we mark this one as a duplicate of #2237831: Allow module services to specify hooks, or the other way around? I don't think we need both.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Category: Task » Feature request
Priority: Major » Normal
Status: Active » Closed (won't fix)
Issue tags: -DX (Developer Experience), -Increases learning curve, -Needs issue summary update

I reopened this issue 4 years ago. I don't think I needed to, after all.
Events can be added when they are appropriate, and hooks are still relevant on their own.
Deprecating individual hooks and replacing them with events could also be done on a case-by-case basis.