Conclusion: Postpone to D9

This possible decision to entirely replace hooks is postponed to Drupal 9.
Until then, in Drupal 8, we basically have two systems in parallel (#77 / #78):

  1. The traditional Drupal hook system.
  2. Event listeners / subscribers.
    These need a better documentation, which is discussed in a follow-up:
    #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners
    The benefit of events/subscribers, besides being slightly more flexible, is that the subscriber classes can have their dependencies injected via DIC.
  3. Only for special situations: Methods on the bundle class.

The D8 cycle will allow us to gain some experience with event subscribers, and compare them with hooks.

Original request

I realize that this will be a little controversial. /me dons flame-proof suit.

We have the Symfony EventDispatcher class in core, and I think we ought to use it as for our hook system. This has the following benefits:

1) Developers coming to Drupal from other systems can immediately understand what's happening (the Observer pattern is pretty common).
2) We could have functions that listen for multiple events.
3) We get rid of the magic function naming thing and replace it with explicitly declared events.
4) We get to leverage external code for something that we're already doing and get rid of a Drupalism in the process.

Patch attached. Adds the drupal_event() function in bootstrap.inc, converts hook_node_presave to the event dispatcher, and changes forum_node_presave to be a listener rather than a hook implementation. It's a fairly minimal changeset - I didn't want to spend a lot of time on it if it was going to be rejected, but I think there's enough there to see what it might look like.

Note: Right now, I'm just considering the event hooks. Alter hooks and declarative hooks are something that I'm not sure how to move forward on. My thought is that most alter hooks could be easily converted to event hooks (ex: instead of hook_form_alter, have form.pre_render or something). We may be able to handle them all the same way. I don't know because I haven't played with it.

Files: 
CommentFileSizeAuthor
hooks.patch5.1 KBcweagans
FAILED: [[SimpleTest]]: [MySQL] 35,889 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Comments

cweagans’s picture

Oh, one other thing:

We can replace this (yeah, yeah, I know...Trigger module doesn't exist anymore):

/**
* Implements hook_node_presave().
*/
function trigger_node_presave($node) {
  _trigger_node($node, 'node_presave');
}

With this:

drupal_event()->addListener('node.presave', function (Event $event) {
    _trigger_node($event, 'node_presave');
});

I like the second one better :)

JohnAlbin’s picture

In the second example above, when do you register event listeners? In the bare .module file? I'm just curious.

cweagans’s picture

In that example, it'd just replace trigger_node_presave - you'd put it in trigger.module.

Status:Needs review» Needs work

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

Xen’s picture

I do like hooks, but I like this too. Could use a bit of syntactic sugar (like drupal_event('presave', $node); perhaps an alternative way of adding listeners).

Any performance data? Can't imagine it should be worse than module_invoke, but for something this much used, it should be looked at.

cweagans’s picture

Nope, I don't have that kind of data yet. I *think* EventDispatcher might be a little faster, but it's just a hunch and I don't have anything to back it up.

neclimdul’s picture

So I've talked with cweagans on IRC about this but I'll restate it hear so its all out in the open for the community. I'm not really comfortable doing this all out in Drupal 8. I think it could actually be a novel approach to solving issues in places like hook_boot() and hook_init() where there is actually some goofy logic and we can greatly benefit from a more light weight explicit event system(just embed it into the kernel coming out of WSCCI for example). I don't think just exposing a global event dispatcher and explicitly registering hooks as listeners either solves or improves anything though.

I'm not going to say stop working on this by any means. Investigating could prove very informative but my gut reaction at the moment is to say hold considering accepting this till Drupal 9. We're doing some pretty invasive work on Drupal already with the initiatives(not saying wscci, all of them) in this release and changing too much risks leaving us with a very unstable release.

Piece said, please continue developing.

Crell’s picture

Noice!

+++ b/core/includes/bootstrap.inc
@@ -1009,6 +1010,17 @@ function drupal_load($type, $name) {
/**
+ * Returns the EventDispatcher object.
+ */
+function drupal_event() {
+  static $dispatcher = NULL;
+  if (!$dispatcher) {
+    $dispatcher = new EventDispatcher;
+  }
+  return $dispatcher;
+}
+

This is fine for now, but should eventually get folded into the dependency injection container: http://drupal.org/node/1497230

We may also want to question if we want to have multiple event dispatcher objects rather than one global one. Symfony, I believe, has one global one and then a few small ones for corner cases. Not sure of the details.

+++ b/core/lib/Drupal/Core/Entity/EntityEvent.php
@@ -0,0 +1,19 @@
+class EntityEvent extends Event {
+  public $entity;
+  public $type;
+
+  public function __construct(&$entity, $type) {
+    $this->entity = $entity;
+    $this->type = $type;
+  }
+}

You knew this was coming, but... No public properties, please. Add a getEntity() method. Also, entities are all going to be classed objects (work in progress) with a proper method to determine the entity type and bundle, so the $type property here is unnecessary. You can simply do $event->getEntity()->getType() (or whatever the method is called).

That also helps clean up some of the isset() and empty() logic below.

+++ b/core/modules/node/lib/Drupal/node/NodeEvent.php
@@ -0,0 +1,17 @@
+namespace Drupal\node;
+use Symfony\Component\EventDispatcher\Event;
+
+class NodeEvent extends Event {
+  public $node;
+
+  public function __construct(&$node) {
+    $this->node = $node;
+  }
+}

I don't see a need for this class. Most node hooks really ought to be entity hooks anyway, so let's just stick with the EntityEvent class. Fewer classes FTW. :-)

+++ b/core/modules/node/node.module
@@ -1090,8 +1092,10 @@ function node_save($node) {
-    module_invoke_all('node_presave', $node);
-    module_invoke_all('entity_presave', $node, 'node');
+    //module_invoke_all('node_presave', $node);
+    //module_invoke_all('entity_presave', $node, 'node');
+    drupal_event()->dispatch('node.presave', new NodeEvent($node));
+    drupal_event()->dispatch('entity.presave', new EntityEvent($node, 'node'));

As above, we can merge these into just entity.presave here, I think.

+++ b/core/modules/forum/forum.module
@@ -321,22 +323,23 @@ function forum_node_validate($node, $form) {
+drupal_event()->addListener('node.presave', 'forum_node_presave');

And here's the trick. Registration. Really, we don't want to still use a function here. Closures are a bit nicer, but still runs into the "must be loaded and parsed" problem that hooks have now.

Better than listeners are subscribers, which are, basically, multiple listeners wrapped up into a single object. That's sweet, in part because you can then have multiple listeners "clustered" and sharing data... or implement multiple listeners to the same event in either the same class or different classes. Think of allowing a module to implement a given hook multiple times without ugly if or switch statements internally, or documentation to say "these two code blocks are totally different, but, yeah..."

The trick there is that the subscriber class has to be instantiated to be registered. That runs right back into the same "must be loaded" scalability problem.

Symfony full stack addresses this by having an intermediary step, where listeners are registered in the dependency injection container and then on a given event are pulled out of the DIC (and only instantiated at that time) and registered, and then immediately fired. The DIC is then also build and compiled once to disk, so you don't need to go through the rebuild process every time. That's much much better, but we're not at that point yet. So, not sure if we want to wait, want to have an intermediary step, or what.

I do know that we do not want to have drupal_event() calls outside of a function in a module, period. :-)

cweagans’s picture

We're doing some pretty invasive work on Drupal already with the initiatives(not saying wscci, all of them) in this release and changing too much risks leaving us with a very unstable release.

While I can see where you're coming from, that's what we have tests for. If we want to make huge invasive changes to all of Drupal (and we can get it done in time), why not?

We may also want to question if we want to have multiple event dispatcher objects rather than one global one. Symfony, I believe, has one global one and then a few small ones for corner cases. Not sure of the details.

I think we should have one single event dispatcher that everything in core uses. There's nothing stopping contrib from instantiating a new one if somebody wants to do something crazy. The nice thing about having everything run through a single EventDispatcher is that you can get a list of all registered listeners and not have to worry about things like, "Oh, well that module is using this other EventDispatcher so I also need to get the data from there and merge it with this one and..."

You knew this was coming, but... No public properties, please. Add a getEntity() method. Also, entities are all going to be classed objects (work in progress) with a proper method to determine the entity type and bundle, so the $type property here is unnecessary. You can simply do $event->getEntity()->getType() (or whatever the method is called).

I didn't know about the public property rule, but that's easily dealt with. I wasn't sure if it would work, though. $node is being passed into the constructor by reference, so if there's a getEntity() method that returns $node, will the caller of getEntity() still be operating on the reference? Too many layers of code :)

I don't see a need for this class. Most node hooks really ought to be entity hooks anyway, so let's just stick with the EntityEvent class. Fewer classes FTW. :-)

I agree, but we have the distinction between hook_node_presave and hook_entity_presave right now, so I was preserving that. Please open another issue for that :)

And here's the trick. Registration. Really, we don't want to still use a function here. Closures are a bit nicer, but still runs into the "must be loaded and parsed" problem that hooks have now.

Better than listeners are subscribers, which are, basically, multiple listeners wrapped up into a single object. That's sweet, in part because you can then have multiple listeners "clustered" and sharing data... or implement multiple listeners to the same event in either the same class or different classes. Think of allowing a module to implement a given hook multiple times without ugly if or switch statements internally, or documentation to say "these two code blocks are totally different, but, yeah..."

The trick there is that the subscriber class has to be instantiated to be registered. That runs right back into the same "must be loaded" scalability problem.

Yep, registration is the big problem here. I did it the easy way, but I'm sure there's a better way to do it. I looked into the subscriber thing, but didn't have enough time to play with it - it looked interesting though.

The thing that I absolutely want to avoid is adding lots of extra complexity on top of the hook system. That, to me, would be failing at this task.

Crell’s picture

I didn't know about the public property rule, but that's easily dealt with. I wasn't sure if it would work, though. $node is being passed into the constructor by reference, so if there's a getEntity() method that returns $node, will the caller of getEntity() still be operating on the reference? Too many layers of code :)

$node is not passed by reference. It's passed by handle. Vis, the handle is passed by value but both variables now refer to the same object, which is not the same thing as the variable. Still, method or no method does not change that. That's actually a good thing, as it saves memory. Just make the property protected and add a getEntity() method, and all will be right with the world. :-)

I agree, but we have the distinction between hook_node_presave and hook_entity_presave right now, so I was preserving that. Please open another issue for that :)

That can easily be addressed during conversion later. There's no need to duplicate both hooks with events, just the one. :-) Even at that, you can pass the same event object to two dispatch calls, so you still don't need the second class.

cweagans’s picture

I've been trying to figure out how to do registration.

Crazy idea: what if we listed the implemented hooks in the module's .info file? Something like this:

hooks[node.presave] = 'name_of_responder_function'
hooks[node.load] = 'name_of_other_function'

Could even do things like:

hooks[node.presave] = 'name_of_responder_function'
hooks[node.presave] = 'name_of_different_function'

to have two separate functions called for the node.presave event.

wizonesolutions’s picture

This is DX-- for me. I would like additional flexibility, but I don't want to get rid of magic naming as a default. That makes things convenient and predictable. I don't mind if the way the magic works changes, but I want there to be magic. Definitely not having to maintain it in the info file.

cweagans’s picture

Well of course it's a DX-- for existing Drupal developers - who really enjoys change - but there are a bunch of other frameworks that require explicit registration. In addition, I have witnessed a great number of WTFs coming from new Drupal developers - magic function naming isn't something that immediately makes sense to developers coming in from other frameworks. Explicit declaration of listeners is. I've gone through some other frameworks and figured out how their event system works:

CodeIgniter

Rather than repeating what the docs say, I'll just link to it: http://codeigniter.com/user_guide/general/hooks.html (It's much easier that way)

CodeIgniter has exactly seven hooks that people can respond to: pre_system, pre_controller, post_controller_constructor, post_controller, display_override, cache_override, and post_system. If you want your code to respond to those hooks, then you have to first turn on the hook system, then explicitly declare the Class, Function, and Filename of the event handler.

Wordpress

Wordpress has two separate concepts that fall into the scope of our hook system: Actions and Filters.

Wordpress actions are the equivalent to our event hooks (hook_node_presave, hook_node_save, etc.). In order to register an event listener, you have to call `add_action()` with information about your action. This example is taken from http://codex.wordpress.org/Function_Reference/add_action#Simple_Hook. email_friends() will be called when do_action('publish_post') is called elsewhere in the wordpress source.

function email_friends( $post_ID ) 
{
   $friends = 'bob@example.org, susie@example.org';
   wp_mail( $friends, "sally's blog updated", 'I just put something on my blog: http://blog.example.com' );

   return $post_ID;
}
add_action('publish_post', 'email_friends');

Symfony

Obviously, Symfony uses EventDispatcher. Docs are here: http://symfony.com/doc/current/components/event_dispatcher.html

You either have to explicitly call `addListener` or `addSubscriber`.

Kohana

I wasn't able to find any documentation on the hook system in Kohana 3.2, but the 2.x hook system worked much the same as CodeIgniter.

Yii

Yii has Component Events and Behaviors: http://www.yiiframework.com/doc/guide/1.1/en/basics.component#component-... and http://www.yiiframework.com/doc/guide/1.1/en/basics.component#component-.... I'm not really sure how it works, but I think you have to explicitly declare your listeners.

Joomla

I didn't even bother because we all know Joomla is a joke. In all seriousness, this is all I could find: http://docs.joomla.org/Extension_Installer/Installer_Hooks

Not helpful.

Explicit listener registration is common among other frameworks. I want to make it a priority to make it easier for users from those other frameworks to come into Drupal. Using the Symfony EventDispatcher would also mean that our event registration/response functionality has 1) some commonality with other systems, and 2) it's less Drupal-specific code.

wizonesolutions’s picture

OK, I can see where you're coming from, and, now that I think about it, jQuery does it that way as well. Ease of coming to Drupal from other frameworks is probably a plus since it's key to Drupal getting more popular.

I still would prefer this stuff not be in the .info file, though, if possible.

cweagans’s picture

The .info file suggestion was just an idea - not necessarily the way I want to go, but I was hoping that it might spark some other ideas. I mean, we /could/ just put the listener registration in the .module file as it is in the patch, but Crell said in #8 that he absolutely did not want that.

I don't know how the dependency injection stuff works, and that might be a good way to go, but I don't know enough about it to even start to make an assessment.

Crell’s picture

Symfony full stack uses configuration files that will call addListener/addSubscriber for you, I believe. Plus, it has a compile step (which we don't have at present) that pre-generates code based on the configuration.

I would much much much prefer that if we move over to Symfony-style events with explicit registration that we avoid functions and use Subscriber classes. It just makes it so much easier than having to worry about what files are loaded when. That does run into an issue with instantiating objects in order to register them, of course. Symfony full stack uses some magic in the dependency injection container to make subscribers services in the DIC, and then instantiate them "just in time". I don't fully know the details of how that works because I didn't expect that we would be going there, so haven't looked into it in detail.

cweagans’s picture

Sure, but you'd have to have a whole bunch of subscriber classes in your module, probably in the PSR-0 directory structure. Creating all those directories and a class just to respond to the node presave event seems way overkill.

Most of the other systems that I looked at in #13 had explicitly registered event responders, but they did so right after the function was defined (as it is in my patch). Is there a reason that we should not do that?

Crell’s picture

You can group your subscribers however you like. That's one of the nice things about them. I also don't see how that's overkill at all. Creating a class is cheap. The extra "overhead" is writing "class Foo impelements Bar". Really, that's not a high cost. :-) And directories are free.

Remember code weight. If you have to parse all code before you can do anything, then a system the size of Drupal basically is incapable of even starting up without APC. That's a design flaw (and one that Drupal 7 already suffers from badly.) If you have to parse all code *and* instantiate all objects, well, then you're falling into the trap that the "eek, OO bad and slow" people keep ranting about, in this case correctly. The setup cost becomes so high that you lose all performance advantage of using OO.

And registering listeners/subscribers with code in a .module file that is outside of any class or function is simply not on the table at all. That's a hack that stopped being acceptable in 2004. (Yes some modules still do it. It's still wrong. :-) )

cweagans’s picture

The extra "overhead" is writing "class Foo impelements Bar". Really, that's not a high cost. :-) And directories are free.

I'm not talking about code overhead. I'm talking about developer overhead. If a new Drupal developer is trying to figure out how to react to a new node being saved, I'm thinking that it's going to be difficult to explain that you have to `mkdir -p sites/all/modules/yourmodule/lib/yourmodule/Event/Subscriber.php` and then create a class that implements Bar with a certain method inside that automagically responds to certain events. That doesn't get us anywhere - we already have functions that automagically respond to events.

In addition, if we use Subscriber classes that are autoloaded, what is left in the .module file? None of the event hooks...maybe some of the alter and info hooks. I'd guess that we'd probably want to convert those to EventDispatcher in the future as well, so those would be gone. So basically, all you have left is

What if we repurpose hook_hook_info() to be the explicit hook declaration thingy for Drupal?

function forum_hook_info() {
  return array(
    'node.presave' => 'forum_node_presave',
    'node.save' => array(
      'forum_node_save',
      'forum_node_save_second_responder',
    ),
  );
}

and then a single function in Drupal is responsible for gathering up all of that info and registering the hooks with EventDispatcher?

Then, if you want to create an event Subscriber that responds to all node events, you could do something like this:

function forum_hook_info() {
  return array(
    'node' => 'ForumNodeListener',
  );
}

Then, ForumNodeListener could respond to any node events.

Xen’s picture

Following the discussion, I'll just throw in some thoughts on the DX perspective.

As much flak the magic naming method is getting, it still has some plus points that seems overlooked.

Explicitly registering listeners might be popular in other frameworks, but implicitly 'registering' things are too. Declaring a __set method on an object is an example. The DRY principle relies much on implicitcy to make things feel simpler.

Magic naming fosters consistency. mymodule_node_load *has* to be called that in order to work. When using explicit registering, you soon end up with mymodule_when_node_loaded in one module and myothermodule_on_loading_node in another. Some might argue that 'proper' developers will use a consistent naming convention, but relying on unwritten rules or coding standard specifications is a bit of a failure and is the kind of thing that alienates new developers.

Using functions for as the base of magic naming has its known problems, but there could be something to work with by fiddling with classes/interfaces/namespaces/filenames, that could work better. Say any class implementing NodeHookListener will get it's load() method called instead of hook_node_load, or some other documented 'implicit' behaviour (I'm sure someone else can come up with better ideas). Could be something that's really just a addListener wrapper for EventDispatcher. Or something entirely different yet.

I'll claim that one of the reasons that we have so many hooks to play with today is the fact that they're so easy both to trigger and to implement. If you replace module_invoke with 3 lines of code and having to define the hook specification in another place, you'll have killed the hook system in one major version flat...

That said, I'm not advocating that we don't mess with the hook system in an attempt to fix its failings, but I think that wholesale adopting the ways of other frameworks might end up throwing out the baby out with the bathwater. Hooks needs to be kept extremely simple to use and understand, as they're a major part of what makes Drupal so goddamn flexible.

wizonesolutions’s picture

I concur. You expressed what I was trying to say much better than I could. Thanks.

cweagans’s picture

I'll claim that one of the reasons that we have so many hooks to play with today is the fact that they're so easy both to trigger and to implement. If you replace module_invoke with 3 lines of code and having to define the hook specification in another place, you'll have killed the hook system in one major version flat...

I disagree. In all of the other frameworks that I've looked at, hook implementation is a two-step process: define your function, then add your listener. Plus, we're already kind of doing that for WSCCI - just adopting a bunch of stuff from Symfony. That said...

Could be something that's really just a addListener wrapper for EventDispatcher. Or something entirely different yet.

What if we just used EventDispatcher under the hood? Keep the magic naming - those would be automatically registered with the EventDispatcher - but allow for non-standard naming with manual use of addListener(). (I say this not because I think it's the right way to go, but because it is most likely to be agreeable with other devs: most people seem to want to keep the magic naming)

This would basically involve unifying all of the event hooks (node presave and all of that) to take a single argument (an object that extends Event) that contains all of the relevant data for that Event. We could do the exact same thing with alter hooks and info hooks as well: it'd basically end up that the object gets passed around to all of the listeners to do operations on, then gets returned to the caller. Dispatching an event would look something like this:

// $node_event contains the node object and some other information
drupal_event()->dispatch('node.presave', $node_event);
bojanz’s picture

At DrupalCon we talked about exploring plugin registration through annotations (the Doctrine2 kind, and their parser code).
If we went that way, that would allow us to reuse them for registering hooks as well, giving us explicit registration with no downside (we have a "Implements" comment anyway, it's just not parsed).

cweagans’s picture

Oh, that would be really really really cool! If that option is available, that's where my vote is.

Crell’s picture

Issue tags:+symfony

Subscriber classes do not use magic naming. There's no magic happening at all, except for calling the interface-defined static method to register the methods that you explicitly specify. Have a look at the kernel patch to see that in action.

Annotations for registration are a possibility, but they're not a silver bullet. You still need to find the class in question to get its annotation, and then normall you'd parse the code and use reflection. But since you cannot un-interpret code, that runs into memory issues unless you do what the registry does and string-parse the file to get its docblock. And then, well, you're back to string parsing your own code. :-)

Xen’s picture

I disagree. In all of the other frameworks that I've looked at, hook implementation is a two-step process

Which proves what? That it's used? Yes. That it's the best solution? No.

The fact that Wordpress' hooks seems to be documentable on a single page, suggests to me that it's not quite as heavily used as Drupals (and I should say that Wordpress is mature enough to warrant the comparison): http://codex.wordpress.org/Plugin_API/Action_Reference

Yes, other systems is using the define and register method, that doesn't mean they have a hook system that's as universally used as hooks is in Drupal.

Which

What if we just used EventDispatcher under the hood?

That was what I was trying to suggest. If it makes sense, yeah. But if we're shoehorning the hook system into EventDispatcher, for no better reason than "that's how the others do it", I'd vote against.

Keep the magic naming - those would be automatically registered with the EventDispatcher - but allow for non-standard naming with manual use of addListener()

That wont buy us anything but an additional way to implement hooks, which in my opinion is just a step back (consistency please).

cweagans’s picture

The fact that Wordpress' hooks seems to be documentable on a single page, suggests to me that it's not quite as heavily used as Drupals (and I should say that Wordpress is mature enough to warrant the comparison): http://codex.wordpress.org/Plugin_API/Action_Reference

http://api.drupal.org/api/drupal/includes%21module.inc/group/hooks/7

Our hooks are documentable on one page as well. Most Wordpress plugins use hooks much the same way that Drupal modules do. In any case, my point was not that it's the best way to do it, but that it's a common way to do it. In the interest of making it easier for developers from other systems to come into Drupal, I see the "best" hook implementation method at the one that is more consistent with other systems, as long as we do not sacrifice performance and DX in the process.

donquixote’s picture

Most of what is being suggested here can happily coexist with each other.
These things can easily be tried in D7 contrib, and some already do exist somewhere.

  1. magic-named functions (= current Drupal hooks)
  2. magic-named classes (did that once, but discarded)
  3. explicitly registered functions (= callbacks - e.g. page callback, access callback, etc)
  4. explicitly registered objects (plugins). E.g. in Crumbs module.
  5. explicitly registered class names. E.g. views plugins registered with hook_views_plugins()
  6. explicitly registered class name + constructor args.

In all the cases where one of the above techniques is used, there are reasons why the author chose this method over another. At least, that was my experience when playing with each of them once :)

------------

All the "explicitly registered" things currently existing in Drupal use a hook (= magically named function) as a starting point, where the actual registration happens.

In other frameworks, a lot of this happens in a boot script or based on a xml config.
I doubt this would be practical for Drupal. If you install Drupal, you are not going to wire up an xml file.

Someone suggested info files registration - this is just as static as magic naming, but far less DRY -> not cool.
The topic starter fires the registration from top-level scope, that is, outside any class or function. This is even worse than doing it in a hook. Or rather, it uses the most unpredictable hook ever - file inclusion.

-------------

My conclusion:
Let's not sell the listener/observer/subscriber objects as something new. Everyone can wire up a listener or subscriber chain in, and people are already doing it (I do it, at least).
The new thing would be the registration method, if this is something other than a hook. But yet I have not seen anything convincing. Magic-named classes are in most cases worse than magic-named functions, imo.

sun’s picture

Component:other» base system

We need to properly analyze the real pros and cons of our current hook system:

+ simple and quick to code and implement.
+ no registry required.

- impossible to subscribe to multiple events.
- impossible to subscribe to all events of a certain kind/class.
- custom order per hook is cumbersome.
- code size/bloat loaded on every request (breaking performance on hosts without opcode cache).
- not really self-documenting (only api.d.o is able to parse and hint to hook_NAME() docs).

Given that, I'd approach something along the lines of #20 and #19 combined into one. Read those two comments if you haven't already.

However, the largest problem of our hook system by far is the order/sort in which implementations are invoked.

- Implementations are ordered by module weight by default.
- Modules may use module_implements_alter() to adjust invocations for a particular hook, which is utterly complex.
- Module dependencies are not taken into account at all.
- Symfony's EventDispatcher would do the same (and hardcode even more weights; incompatible with our modular system)

The real need of module developers is a hook sorting mechanism that works along the lines of hook_update_dependencies(); i.e.:

+ Specify exactly after (or before) which other hook implementation yours needs to run.
+ Otherwise, execute in (reversed) order of module dependencies.

i.e., no hardcoded weights in registered events, no hardcoded module weights, no module_implements_alter(), but instead only a smart algorithm that performs $(me).before(other) and $(me).after(other) for every hook (not global).

catch’s picture

chx had an idea a while ago about passing the hook dispatcher to the hooks themselves, so they can tell it they want to run again later, not sure if that's in an issue.

Also cross-linking #1331486: Move module_invoke_*() and friends to an Extensions class which I think we could do parallel to any other efforts - that wouldn't change anything to do with hook implementations, just refactor hook invocation.

donquixote’s picture

@sun (#29)
I would argue that a lot of the "cons" do not apply to the majority of use cases where hooks are currently used.
For the rest, developers are free to use a custom subscriber/listener/plugin system or other techniques. We don't need to use hooks for everything, it is just one tool available.

Besides, using EventDispatcher or other registration plugin systems does not magically solve all these problems.

- custom order per hook is cumbersome.
[..]
- Implementations are ordered by module weight by default.
- Modules may use module_implements_alter() to adjust invocations for a particular hook, which is utterly complex.
- Module dependencies are not taken into account at all.
[..]
+ Specify exactly after (or before) which other hook implementation yours needs to run.
+ Otherwise, execute in (reversed) order of module dependencies.

More control over the order of hooks is sometimes necessary, but in most cases it is not.
module_implements_alter() may not be perfect, but in most cases it does the job.
And, if we really want sorting by dependencies, I imagine this can all be done within the current hook system.

No need to replace it entirely.

- impossible to subscribe to multiple events.
- impossible to subscribe to all events of a certain kind/class.

This would indeed be quite nice. But again, only in some cases.
Currently we already have things like hook_form_alter() vs hook_form_FORM_ID_alter(). This is a bit ugly, but it works.
Where this is not enough, developers can use a custom-tailored plugin system instead of procedural hooks.

- code size/bloat loaded on every request (breaking performance on hosts without opcode cache).

There are already ideas for putting hook implementations into separate files, etc. And opcode cache is recommended anyway.

- not really self-documenting (only api.d.o is able to parse and hint to hook_NAME() docs).

This would be improved if we use _hook_ as the separator, or if we switch to namespaces for procedural and require hook_ as a prefix for hook implementations (let's not get into details, just accept that there are possible solutions).

- Symfony's EventDispatcher would do the same (and hardcode even more weights; incompatible with our modular system)

As mentioned, probably a lot of the problems will be just the same, in a new dress.

-----------------

To extend on your list, I would mention "dynamic subscription" to events. E.g. subscribe only if condition X is true, or subscribe N different instances of a "listener". Again, this is an indicator that hooks may not be the best solution to the problem, but they can still be the solution of choice for 90% of other cases.

---------------

Conclusion:
- Consider more powerful variations of hook_module_implements_alter()
- Consider to change the hook syntax to require _hook_ as a separator, or something similar.
- Where hooks don't do the job, use a suitable subscriber/listener/plugin system.
- If we notice common patterns with those plugin systems, consider to add a subscriber/listener/plugin library in core.

I don't see a reason to kill the hook system.

cweagans’s picture

To be clear, I absolutely do not want to "kill the hook system".

Currently, we have two hook systems in core (the Symfony one, and the existing Drupal one). I am proposing that we pick one and run with it. Since our use of Symfony components requires that we have EventDispatcher around, I'm leaning toward using EventDispatcher.

Note that we could, in theory, just use EventDispatcher to replace module_invoke(). We could continue to have magic function names and all that, but I want to take it a step further and use the flexibility that EventDispatcher gives us, like hook ordering.

I think the hook ordering that sun proposed is a good direction to go, and it doesn't necessarily need to be a mandatory aspect of hook implementation. You'd only really need to use it when your hook implementation needs to run before some other module's hook implementation.

There are already ideas for putting hook implementations into separate files, etc. And opcode cache is recommended anyway.

1) This is one of those ideas. If you need to subscribe to an entire class of events (for instance, all node events: presave, save, update, etc.), then you'd create a subscriber class (which would live in a separate file in the PSR-0 directory structure. Normal hook implementations would continue to live in the .module file.
2) Yes, an opcode cache is recommended, but it's absolutely pathetic that Drupal cannot run well WITHOUT one. There are plenty of other web applications that run okay without an opcode cache (not great - just "okay"). We cannot do that because of how much code we're loading.

Where this is not enough, developers can use a custom-tailored plugin system instead of procedural hooks.

This is exactly the kind of thing that I want to avoid. If we have developers creating their own hook/plugin systems because the one that lives in core is not flexible enough, then I see that as a problem. This issue is about expanding the flexibility of our hook system, while adopting more Symfony code and reducing the number of Drupal-specific things that new developers have to learn.

- Consider more powerful variations of hook_module_implements_alter()

That's what I'm trying to do. EventDispatcher is very flexible. If it's not flexible enough, then we can extend it easily.

- Where hooks don't do the job, use a suitable subscriber/listener/plugin system.
- If we notice common patterns with those plugin systems, consider to add a subscriber/listener/plugin library in core.

To address the first point: our hook system is essentially a subscriber/listener system. We just don't label it as such.

To address the second point: as I said before, we have TWO subscriber/listener/plugin systems in core: the Symfony one and the Drupal one. I see no reason to continue maintaining the Drupal hook system if the Symfony system can do what we need, and do it better.

donquixote’s picture

To be clear, I absolutely do not want to "kill the hook system".
[..]
Note that we could, in theory, just use EventDispatcher to replace module_invoke().

I think the primary matter of discussion should be how the hook system (or whatever we replace it with) looks to a module developer.
E.g. can the module developer just write "function mymodule_foo()" or "function mymodule_hook_foo()", or does she need to create an object and register it?

Whether we use EventDispatcher or something else "under the hood" within module_invoke, is an implementation detail. If a solution based on EventDispatcher is more performant or flexible, I think noone will complain (as long as we keep the procedural hook syntax).

To address the first point: our hook system is essentially a subscriber/listener system. We just don't label it as such.

The very important difference is that hook implementations are (usually) not explicitly registered, but discovered based on magic naming. This point is conveniently skipped in these discussions (see also #28, on different types of subscribing vs "being discovered")

To address the second point: as I said before, we have TWO subscriber/listener/plugin systems in core: the Symfony one and the Drupal one. I see no reason to continue maintaining the Drupal hook system if the Symfony system can do what we need, and do it better.

I think it is just wrong to assume that all listener/subscriber/plugin systems must be the same.
I can only tell from my own modules, where I use different variations of OOP plugins/listeners and procedural hooks. Basically, I have tried most of the variations in #28, and I am happy to have all of them available.
My conclusion is: Use a plane when you need to fly. Use a car when you need to drive. And start the helicopter from a boat, if that helps.

cweagans’s picture

Here is the plan that I'd like to move forward with. Feedback is welcome.

Create a magic-named hook that describes events that a module will subscribe to

This would be similar to what I mentioned in #19. For the forum module, it might look like this:

function forum_hook_info() {
  return array(
    'node.presave' => 'forum_node_presave',
    'node.save' => array(
      'forum_node_save',
      'forum_node_save_second_responder',
    ),
  );
}

Alternatively, if we were going to wire up a subscriber class, it would look like this:

function forum_hook_info() {
  return array(
    'node' => 'ForumNodeSubscriber',
  );
}

This will be the only remaining hook that relies on magic-naming, and will be used by the second step:

Extend EventDispatcher to be aware of our module system and handle registration accordingly

We should extend EventDispatcher so that it is aware of what modules are enabled. For every enabled module, our EventDispatcher class would go grab the hook info from hook_hook_info() (or whatever we call it) and register the events in the normal Symfony EventDispatcher style.

At this point, we'd have a pretty decent hook system in place, but I think that in order to really justify the amount of work this would take, we should go on to...

Remove module weights from the system table in the database

sun's suggestion above makes a lot of sense to me. Often, a hook just needs to be run before another module's hook, so describing that should be pretty easy. If I remember correctly, EventDispatcher already supports dispatch ordering, so our subclass should be able to build on that to make it really easy for module developers to have fine-grained control over when their hook gets executed (but only if they want that control - this step should NOT be required for hook implementation).

Create complex subscriber classes

There was some talk in this issue about splitting your hook implementations out of your .module file into some other file. It might be kind of handy if we could wire up more complex event listeners for subscriber classes:

function forum_hook_info() {
  return array(
    'node;form.alter' => 'ForumNodeSubscriber',
  );
}

In this example, the ForumNodeSubscriber class might have listeners for a bunch of node events (presave, save, update, etc.), as well as a listener for the form.alter event (maybe it would be altering the node_form).

webchick’s picture

At a glance, #34 sounds like a total DX trainwreck to me. :(

The great thing about module development in today's world is that I can extend my module's functionality on the fly by just copy/pasting in an example hook implementation and then modifying it to fit my needs (and in D7, I'm forced to clear the cache after, which is annoying but otherwise this works).

This approach sounds like I would need to not only copy/paste an example hook implementation and clear the cache, but I'd also need to edit this "info" function repeatedly, making careful sure not to make any typos in the array declaration, not to misspell anything like "FormNodeSubscriber" instead of "ForumNodeSubscriber", etc. When something's not working, instead of checking two things (did I prefix the hook name properly and did I clear the cache?) I now am checking not only those things but also a litany of things that might be wrong with my info function.

I get that the hook system has edge cases that are really sharp and irritating when you hit them (e.g. order of hook implementations, namespace collisions). But it's also something that you can explain to a new developer in about 10 minutes and they go "Ah-ha." I'm really concerned for the learnability of Drupal if we go this route.

Crell’s picture

#32 largely summarizes my position. We have two parallel event systems in core right now. That's dumb. We should have one. The EventDispatcher one is the more flexible one, and the one we cannot eliminate because of the Symfony dependencies. Even if it's not perfect, let's use it.

My only concern is whether that happens in Drupal 8 or Drupal 9 for time reasons, not whether or not it happens. Long-term, I have no interest at all in retaining procedural hooks as they make unit testing hard to impossible.

As far as exposing listeners/subscribers to the system from modules, think outside the box. This is a problem that Symfony bundles have already solved, including lazy-loading of event objects for performance reasons, and we should look there for inspiration.

cweagans’s picture

Would it make a difference if we kept the magic naming? That is, everything I said in #34 as well as the magic naming that we currently have.

So, for the form.alter event, forum_form_alter() would be assumed to be an implementation of that hook. Then, by implementing that hook that I described above, you could wire up additional listeners for that event, or you could wire up a subscriber class.

Crell’s picture

cweagans: Making the "primary" system EventDispatcher a la Symfony but with a BC shiv to detect and register legacy hooks is a possibility we should look into. Yes, that would make "hooks" a second-class implementation... but I'm OK with that since I'd rather transition over anyway.

webchick is correct that we need to make sure the DX for full on listeners/subscribers is straightforward. I don't think we need to go as far as "you can use it by blindly copying and pasting and not having a clue WTF you're doing". That's a lower bar than I am interested in setting, but we do need to keep good DX in mind.

donquixote’s picture

@cweagans (#37):
So your *_hook_info() would be a more powerful version of *_module_implements_alter().
(and then "under the hood" it is all about events and subscribers)
I'm absolutely fine with that.

The magic naming for the default case is DRY and "convention over configuration".
(we may still change the syntax to _hook_ as a separator, but that's a different story)

cweagans’s picture

Okay, so one last decision:

Should we provide some kind of configuration option that would allow us to disable the magic naming convention? The benefit is that the function name detection could be skipped, which might make this part of Drupal a little more performant, but the downside is that core would need to explicitly register all hooks.

donquixote’s picture

#40,
do you want to disable magic naming for a specific module, or for a specific hook/event, or just globally?
I think globally would not make sense at all.

"per module" could be useful, but that's an optional feature we can add any time we want. The option could be set within *_hook_info().
"per hook/event" would follow my idea to have different flavors of the hook/event/subscriber system. Question would be, where to set the option.

Another question is, how will people invoke hooks in the future? Is it still module_invoke_all() ?
For my taste this seems fine, at least as an alias.

------

Btw, unlike Crell, I still think that procedural magic naming should be the "normal thing", whereas the explicit subscription via *_hook_info() should be used whenever a more dynamic behavior is required. (DRY, DX, convention over configuration, etc)

effulgentsia’s picture

How about something like:

  • A function that can find all magically named hook implementations and return an equivalent hook_hook_info() array. For example, drupal_default_hook_implementations($module).
  • If a module does not implement hook_hook_info(), then the system defaults to the above result.
  • If a module does implement hook_hook_info(), then it can choose whether to add the defaults or not. For example, if it does not want to add defaults, it would return array(...), and if it does want to add defaults, it would return array(...) + drupal_default_hook_implementations($module).
donquixote’s picture

Btw, for hook_hook_info(), what about a syntax like this,

<?php
function forum_hook_info($api) {
 
$api->register('node.presave', 'forum_node_presave')->weight(-3);
 
$api->register('node.save', 'forum_node_save');
 
$api->register('node.save', 'forum_node_save_second_responder');
 
$api->subscribeClass('node;form.alter' => 'ForumNodeSubscriber');
}
?>

This would allow to use the hook for other things, which do not fit into the array pattern.

<?php
function forum_hook_info($api) {
 
$api->setWeight('node.presave', 'forum_node_presave', 11);
 
// process one before the other
 
$api->setOrder('node.save', 'forum_node_save', 'blog_node_save');
 
$api->setDefaultHookFile('node.save', '%module.node.inc')
}

function
mymodule_hook_info($api) {
 
// No magic naming for this module.
  // ($api already knows the module name)
 
$api->noMagicNaming();
}
?>
cweagans’s picture

do you want to disable magic naming for a specific module, or for a specific hook/event, or just globally?
I think globally would not make sense at all.

"per module" could be useful, but that's an optional feature we can add any time we want. The option could be set within *_hook_info().
"per hook/event" would follow my idea to have different flavors of the hook/event/subscriber system. Question would be, where to set the option.

It would be interesting to disable implicit registration globally in an effort to move away from that model. It's not consistent with any of the other systems that I've played with, and I want to make Drupal easier for developers coming from those other systems. See #13 for details.

In addition, I want to clarify that we absolutely *do NOT need* multiple flavors of hook/event/subscriber systems. Part of the reason for this issue to exist is to remedy the fact that we have two in core right now. We need to move toward one single event dispatcher system and stick with it.

Another question is, how will people invoke hooks in the future? Is it still module_invoke_all() ?
For my taste this seems fine, at least as an alias.

My thought was that we would just call $dispatcher->dispatch(); No need for a procedural wrapper around that.

Btw, unlike Crell, I still think that procedural magic naming should be the "normal thing", whereas the explicit subscription via *_hook_info() should be used whenever a more dynamic behavior is required. (DRY, DX, convention over configuration, etc)

The thing is, it's not normal. Again, #13 has details. Explicit registration is so much easier to understand than implicit registration. While it might be less efficient in terms of keystrokes, it's very nice to be able to see an overview of what functions are going to react to a given event, and hook_hook_info() (or whatever we call it) would let us see that.

Procedural code results in a bunch of crap getting tossed into the .module file, and then it all has to be interpreted on every request. I would like to see us move mostly to subscriber classes in the future. Autoloading++

And finally:

$api->register('node.presave', 'forum_node_presave')->weight(-3);

I want to get away from numeric weights for modules/hook implementations. It's a pain in the ass to manage if your setup is really particular about execution order, and it make much more sense to just say what you really mean:

$api->register('node.presave', 'forum_node_presave')->before('some_other_module_node_presave');

That is, you want to run your hook before some_other_module. Let the computer figure out order of execution based on the criteria specified in the event responder registrations.

I realize that there is a certain amount of resistance that will go along with a big sweeping change like this, but I would urge you to go take a look at how other systems do things. It's really not as big of a DX trainwreck as it seems to be - it becomes very intuitive when you get used to it, and it's very easy for people that are familiar with other systems to learn.

Crell’s picture

I wouldn't even consider hook_hook_info() as the place for this. Someone needs to go dig through Symfony's full stack implementation. I know they have a solution for this question in there; I've talked to some Symfony devs about it. I just haven't looked into it in depth myself.

If only subscribers' static method had access to outside data, we could just set the listener's weight to be $some_other_listeners_weight + 1. ;-)

Or maybe dynamic weighting is something we should see about pushing upstream to Symfony?

donquixote’s picture

The thing is, it's not normal.

What I don't like: I see a lot the argument of "this is how others do it", but nowhere do I see "this is how others do it, and their life is much easier".
Also, I have not seen any example about our own life becoming easier with explicit registration. It is only getting worse.

"DRY" (Don't Repeat Yourself)

When the DRY principle is applied successfully, a modification of any single element of a system does not require a change in other logically-unrelated elements

If we change functionality X, then how many places in code do we need to change?
Explicit registration: 2 places (*).
Implicit registration (magic naming): 1 place.
Implicit wins.

(*) You could argue that the registration and the implementation are "logically related". The wiki article does not really define what that means.
But anyway, the disadvantages of not being DRY apply 100% to any explicit registration model that we could suggest.
Exactly what webchick is pointing out in #35.

"Convention over configuration"

Convention over configuration (also known as coding by convention) is a software design paradigm which seeks to decrease the number of decisions that developers need to make, gaining simplicity, but not necessarily losing flexibility.

The phrase essentially means a developer only needs to specify unconventional aspects of the application. For example, if there's a class Sale in the model, the corresponding table in the database is called “sales” by default. It is only if one deviates from this convention, such as calling the table “products_sold”, that one needs to write code regarding these names.

When the convention implemented by the tool you are using matches your desired behavior, you enjoy the benefits without having to write configuration files. When your desired behavior deviates from the implemented convention, then you configure your desired behavior.

Isn't that super convincing.
Magic naming for the win.

--------

Crell (#45),

I wouldn't even consider hook_hook_info() as the place for this. Someone needs to go dig through Symfony's full stack implementation. I know they have a solution for this question in there; I've talked to some Symfony devs about it. I just haven't looked into it in depth myself.

They are probably using some XML files or bootstrap script?
I don't think any of this does fit into the Drupal universe. Which site builder can be expected to write configuration files that target specific classes?
Anyway, no matter where the subscription happens, if it is not by magic naming, then it is going to make our life more unpleasant.

cweagans’s picture

I am familiar with the concept of DRY and Convention over Configuration, thank you.

What I don't like: I see a lot the argument of "this is how others do it", but nowhere do I see "this is how others do it, and their life is much easier".

Also, I have not seen any example about our own life becoming easier with explicit registration. It is only getting worse.

While I could counter with the statement that "worse" is only your opinion and you don't have substantive evidence to back up such a subjective claim, I will instead say that there are a few benefits that come with explicit registration:

1) You get to see a quick overview of every single event that your module listens for - a mapping between events and their listeners.

2) A quick "grep -r node.save *" will give you a list of every function that listens for the node.save event (along with the places that it's dispatched from). Put another way, you can easily get a list of every single line in the codebase that listens for or dispatches a given event. Seems pretty useful to me. This is a side effect of having the events explicitly registered.

3) Being consistent with other systems is a lot more important than people realize: there is a huge need for skilled Drupal developers, but the Drupal learning curve tends to make people not want to use Drupal. Part of the reason for this learning curve is the number of Drupal-isms that we have: things that we do in Drupal core that are unlike other systems. If we reduce that number, then the learning curve gets a little less steep, and more developers may be inclined to join the project. While this is purely speculative, I think that it's more or less accurate.

If we change functionality X, then how many places in code do we need to change?
Explicit registration: 2 places (*).
Implicit registration (magic naming): 1 place.
Implicit wins.

I'm not quite sure what you're getting at here. The only time you need to edit the hook registration array is when you'd be adding, removing, or renaming a hook implementation. Anything else would continue to be inside the function body. There are also other factors that you need to balance here: explicit registration gives us a lot of extra flexibility that we wouldn't otherwise have with implicit registration.

Again, I'm not opposed to having some sort of compatibility layer in place that allows for implicit registration, but I want to make sure that I can disable that layer if I'm not going to use it, and the only way that I can do that is if all core hooks are explicitly registered.

I wouldn't even consider hook_hook_info() as the place for this. Someone needs to go dig through Symfony's full stack implementation. I know they have a solution for this question in there; I've talked to some Symfony devs about it. I just haven't looked into it in depth myself.

They are probably using some XML files or bootstrap script?
I don't think any of this does fit into the Drupal universe. Which site builder can be expected to write configuration files that target specific classes?

This is not a site builder concern. This is a developer concern. As long as the hooks are registered somehow (doesn't matter which method, be it explicit or implicit), when the module is enabled, the hooks will be called.

Anyway, no matter where the subscription happens, if it is not by magic naming, then it is going to make our life more unpleasant.

This is an opinion. Having played with most of the other frameworks mentioned above, I can say that explicit registration makes code more readable/understandable, and it's really not that much extra work. Seriously, we're talking like four extra seconds/hook implementation, and that's if you type slow.

If you type like webchick, it's more like .25 seconds. =D

donquixote’s picture

Ok, so what about:

  • in D8, we use sth like EventDispatcher "under the hood", with full compatibility for procedural hook implementations and for module_invoke_all(). The new hook_hook_info() will feel like a more powerful hook_module_invoke_alter(), although it does already bring the full EventDispatcher stuff. (If Crell can find a suitable alternative to hook_hook_info(), fine)
  • Let people get familiar with the new possibilities during the D8 lifespan.
  • For D9, we decide if the new explicit registration is actually more useful and popular than the procedural magic naming. If this is the case, then we switch over completely.
donquixote’s picture

#44 (cweagans) - bottom

I want to get away from numeric weights for modules/hook implementations. It's a pain in the ass to manage if your setup is really particular about execution order, and it make much more sense to just say what you really mean:

$api->register('node.presave', 'forum_node_presave')->before('some_other_module_node_presave');

There can be reasons for a weight system, and there can be reasons for dependency-based ordering. There could even be ways to combine both worlds.
I think it does not need to be decided here.

My point was, the syntax in #43 can be easily extended to allow any of that. It's not the weight that I want to sell.

nod_’s picture

Ok so we're talking about events in this issue and while it's not javascript, it's similar enough for a couple of things.

Event weight ordering: same issue in JS (brought up by the same person :) I agree with cweagans and his solution looks really neat.

Event naming: This is mainly why I'm posting here. I understand things are not settled down yet, but please keep in mind that over in JS there is already a agreement about how to name things and in which order.

Let's take the node.save event. It makes sense, looks good but in something like jQuery (other libs follow this convention) this would be save.node since you're firering the save event with a node "namespace" (ok this is JS gibberish, this has nothing to do with PHP). This is about jQuery. A better engineered library, YUI uses something different that would fit better here. with YUI you would name the event node:save.

While I understand PHP and JS events don't have much in common about what they do, it would suck to have to declare something like node.save in php and having to do it backwards, save.node, in JS.

I'm not necessarily saying we should follow the jQuery way of naming things, merely that it should be consistent in our PHP and JS.

cweagans’s picture

Perhaps - I would be pretty resistant to doing save.node instead of node.save though. I would, however, be okay with doing it in the style of YUI and using node:save.

In any case, I'm not sure that consistency between PHP and JS is a necessary evil at this point: they are different languages used for different things, and I'm not sure that we should try to make them behave similarly to each other at this point.

donquixote’s picture

Isn't the real question, what is more likely, that we want to respond to save.* / *.save, or to node.* / *.node ? Or will both be easily possible?

donquixote’s picture

2) A quick "grep -r node.save *" will give you a list of every function that listens for the node.save event (along with the places that it's dispatched from). Put another way, you can easily get a list of every single line in the codebase that listens for or dispatches a given event. Seems pretty useful to me. This is a side effect of having the events explicitly registered.

With a smarter magic naming (_hook_ separator, or namespaces with \\hook_), you would have the same grep effect.

Besides, with your explicit registration, your grep could just return $api->register('node.presave', $callback);, where the $callback variable is set up somewhere else. Or the other way, having $api->register($hook, $callback);, grep would only find the line with $hook = 'node.save';. Of course, this is only the case if people make heavy use of the dynamic aspect of explicit registration.

------

Overall, for the case where dynamic registration is less important, the explicit registration adds one degree of freedom (the name of the callback) that does not express anything. This is what Xen said in #20.

To mitigate this, I would suggest a registration syntax that automatically chooses the name of the callback:

<?php
function mymodule_hook_info($api) {
 
// No auto registration based on magic naming, except for those explicitly registered below.
 
$api->noAutoRegistration();
 
// Subscribe mymodule_foo() for hook_foo() (or to the event "foo")
 
$api->implements('foo');
 
// Subscribe mymodule_foo_1() for hook_foo() (or to the event "foo")
 
$api->subscribe('bar', 'mymodule_foo_1');
}
?>

Note: The $api object has nothing to do with EventDispatcher. It is just an object passed around to functions that implement hook_hook_info(). (I call that "InjectedAPI", but maybe there are better terms)
It is necessary to distinguish, because: hook_hook_info() should fire on cache clear and the result saved to the cache, while EventDispatcher registration should happen in a lazy way, before the hook/event fires.

nod_’s picture

@51: yeah true. An alternative solution is making the events looks completely different (not using a dot in PHP) to avoid any confusion between the two. It's pretty much impossible to change the way event names are used and declared in JS.

Thanks for hearing me out. I'll let you finish on the real problem of this issue.

Crell’s picture

Just to muddy those waters, nod_, Symfony events use dot notation. They also use decreasing-specificity (node.save, kernel.response, etc.) rather than jQuery-style increasing specificity (save.node).

Bah. :-)

neclimdul’s picture

@crell I think that clears the waters actually. I don't see a problem differing event notation styles from JS and in PHP because they'll mean different things and are in line with the tools being used. Using the Symfony standard only makes sense.

I still am on the fence for the idea as a whole but that technical detail I feel comfortable commenting on. *returns to lurking*

effulgentsia’s picture

Since I've just spent a bunch of time benchmarking some Symfony code over in #1578090: Benchmark/profile kernel, I'd like to raise a performance concern that I hope is helpful to whoever works on this. My biggest frustration with PHP is the enormous overhead (relative to C, Java, JS, and I'm guessing just about any other language) of calling a function, overhead that is adding a measurable impact to the WSCCI kernel patch, the first D8 patch that is extensively using Symfony architecture. While we may be willing to accept that overhead for something that comes with huge benefits like HttpKernel has, I don't necessarily see changing all hook invocation to event dispatching as having enough benefit to justify a similar performance regression if there turns out to be one.

If you look at EventDispatcher::dispatch(), it adds 2 extra function calls (setDispatcher() and setName()) that get run even if there are no listeners, and another 2 function calls (getListeners() and doDispatch()) before it even gets to any real dispatching logic. Contrast this with module_implements(), module_invoke_all(), and drupal_alter() that are highly micro-optimized for repeated invocation. Given this, my hunch is that converting all hook invocation to use Symfony's EventDispatcher class as-is won't fly. Some options:

  1. Use EventDispatcher for most hooks, but not for ones that are fired very many times per request.
  2. Subclass both EventDispatcher and Event and remove those extraneous function calls.
  3. Convince Symfony maintainers to micro-optimize EventDispatcher and Event upstream.
  4. Close this issue as "won't fix" and restrict EventDispatcher to bootstrap-level and Symfony-level events, but not use it for strictly-Drupal, post-bootstrap hooks.

Also, given how much there is still left to do on all the D8 initiatives before December, I don't plan on helping out that much on this issue. But if the rest of you are more excited about this than the other things you can be helping out on, then by all means, scratch that itch. If this can be solved cleanly and performantly, it'll make Drupal code more enjoyable to write, which is awesome.

cweagans’s picture

So far, the symfony maintainers have seemed pretty open to receiving patches from the Drupal community, and if it means that Symfony will be faster, then why not?

I'd say that we should try to do this, and if the performance is unacceptable, then we should identify what's causing the performance issues (sounds like EventDispatcher->dispatch() is a good place to start in that case) and fix them.

In other words, I don't want to not do something that I see as valuable on the basis that it might not be good enough right away. There are lots of things in core that started out that way (and a few things that never got further than that but ended up in core anyways) =D

fabpot’s picture

No need to convince me. If those micro-optimizations help, they are more than welcome.

donquixote’s picture

@cweagans, Crell, others:
The initial suggestion explicitly says "event hooks". How do registration hooks fit into the picture? E.g. hook_image_styles(), hook_menu(), etc? Imo it would feel weird to register a callback or class for the image_styles event, etc.

For my taste it is perfectly ok to have procedural magic named hooks for various registration things, while the observer / listener / subscriber pattern can be used for more dynamic events. As mentioned, this is already possible now in D7, people can construct any observer chain that they want.

-------------

@fabpot, nice to see you here :)
I would love to hear some of your ideas on the problem.

I think the heart of the problem is really the registration. Either the registration of observers/listeners/subscribers to (global) events, or the registration of just anything (image styles, route paths, etc).

I would be curious how this works in symfony.
From last times I played with symfony and Zend, I remember I had to do do some wiring-up in the index.php. Also, I think I have seen something about configuration files (yml, I guess), that are used to wire up different components.

If I understand correctly, unlike in Drupal, a symfony "module" is just a passive library that provides classes to be used. These classes then need to be actively instantiated from application code, or wired up in a custom bootstrap or configuration file.
The classes provided by a symfony module are not presented in a nice admin GUI, they are simply just available as code.

In Drupal this is quite different: Modules actively intervene in the application flow, with no additional custom application code to wire it up. Plugins (image styles etc) provided by modules are made known to Drupal so that they can be listed in an administration UI, with description, title, configuration form etc.

Drupal by default has no "application code". It only has reusable module code, and a configuration layer to turn all this into an "application". All of that configuration (in D7) lives in the database.

The Drupal UI-driven configuration layer needs to know a lot more about the "available components", than the symfony custom bootstrap code, or file-based configuration.

Do you have an idea of how this can fit together?

cweagans’s picture

Title:Use Symfony EventDispatcher for event hooks» Use Symfony EventDispatcher for hook system

There, I fixed it.

RobLoach’s picture

cweagans’s picture

Status:Needs work» Postponed

I'm going to temporarily postpone this on #1599108: Allow modules to register services and subscriber services (events). Once we figure out how we want to do registration, we can move forward on this issue.

chx’s picture

I would much much much prefer that if we move over to explicit registration <= I wouldn't

that we avoid functions <= avoiding functions for avoiding functions sake is not helpful

and use Subscriber classes. <= nor is using classes just because.

Not everything that Drupal does is wrong and what Symfony does is good. There are measures I use to decide where using a class is appropriate. Encapsulation. What do you encapsulate in an event listener, a class which has a single method? Implementing a meaningful interface. Well, I am not sure whether "answering to an event" is meaningful / helpful. And so on.

Drupal hooks are a wonderful, easy to implemented, easy to understand tool.

donquixote’s picture

@chx,
what's the "if" in there? :)

Anyway, your post pretty much reflects my thoughts in previous posts, so I would tend to agree.

However, I can see a point for all this, if:

  1. We let those subscribers participate in Dependency Injection - which I assume will be the case. This can be the death to a lot of static cache variables, static function calls, global state, etc.
  2. We agree on reasonable defaults, that allow to bypass explicit registration in "the average case". E.g. some standard class names that will be automatically subscribed.
  3. We agree on reasonable patterns for which of the traditional "hooks" should be combined in the same class.

If we do all this, then the result could actually be quite sexy.
I have not looked at symfony EventDispatcher enough to see if this will be the case. I think it depends what we make of it.

In fact in some D7 modules we already see stuff like:

<?php
/**
 * Implements hook_menu()
 */
function mymodule_menu() {
  return
give_me_object()->hook_menu();
}
?>
cweagans’s picture

Status:Postponed» Active

Okay, I've had an idea, thanks to cosmicdreams.

What if we keep magic function naming, but we write out a PHP file that registers hooks with the EventDispatcher with our fancy new PHP writer?

That way, we still have the good DX of being able to just create a function to implement a hook, but everything is written out to PHP and is cacheable by APC. I think there could be a bit of a performance gain there after the first page load, but I'm not sure.

What do you think of this idea?

Crell’s picture

I don't follow. We could write a bridge that adds our hook functions as listeners to events, but I don't know what that would buy us. We still can't lazy-load them. Writing functions out to a new file would not help either, as we'd still have to manually load the entire file.

pounard’s picture

Yes rewriting hooks for the sake of rewriting them doesn't sound good to me, it will mainly bring an overhead due to the bridging we would create to do that.

I think that porting hooks to events is something that should be done when on a use-case basis when necessary. There is way too many hooks in core alone (not even counting contrib) this would mean registering thousands of events and listeners: it might be very taxing in term of performances for the whole system. The ideal solution is to reduce the number of hooks until they disappear (in the long term) for the benefit of precompiled registered DIC components and plugins.

cweagans’s picture

That's not really ideal, though, because we're still loading a crapload of code for two separate hook systems.

pounard’s picture

Loading code doesn't raise red flags to me: OPcode caches such as APC are efficient enough to make this insignificant.

Crell’s picture

We've reached and exceeded our ability to avoid loading craploads of code as long as we have procedural systems. We have a clear way forward to resolve that. Let's use it.

podarok’s picture

Priority:Normal» Major
Issue tags:+Hook implementations

with #71 and real overloading of procedural systems lets make it major?

and I want to test it on my http://drupal.org/project/dfw

cweagans’s picture

Priority:Major» Normal

What? No. This isn't major.

Crell’s picture

Why is this major? We're already tidying up hooks in #1331486: Move module_invoke_*() and friends to an Extensions class. I don't yet see a compelling argument to build hooks on top of events, especially when I'd rather we spend the D9 cycle eliminating hooks and converting them to events.

If you want to use events instead of hooks for something, you absolutely can, today, without issue. I would actively encourage it.

chx’s picture

Status:Active» Postponed

What #74 has to say.

catch’s picture

Yeah I don't see any reason to push for this now, especially given the performance concerns of a wholesale move to Event Dispatcher (although it sounds like there's room for optimizing that).

In terms of loading less code, there is far lower-hanging fruit than this - for example format_rss_item() and format_rss_channel() in common.inc. Converting a procedural system to an autoloaded PSR-0 class doesn't result in any less code used if it's something that's in the critical path like the hook system - and building drupal_alter() and similar on top of event dispatcher doesn't even necessarily mean less code either.

donquixote’s picture

Ok, I agree with this being postponed to D9. I am actually happy about it!

As a consequence, developers in D8/contrib now have a choice between
1. traditional hooks
2. Event subscribers
3. Methods on the bundle object.

Again this is great, because now we have time to compare these different options, per use case.

---------------

However, I have the feeling that option 2. and 3. are not really well documented and standardized yet.
Or did I just miss something?
There is the Symfony documentation, but we want to know what is the recommended way to use those things in a Drupal module.

Even if the original request is postponed, this should be discussed as a follow-up.
(or if I am just blind, then it is probably sufficient to update the issue summary)

Crell’s picture

Custom Drupal-defined events, what few there are, are not yet well documented. There's an issue for that(tm): #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners

Methods on the bundle class I'd discourage. There's a small set of use cases where they may be needed to avoid a circular dependency, but it's not really recommended.

So really, the alternatives for D8 are hooks and subscribers. (And we can debate endlessly elsewhere which is "preferred"; I'd say events, not surprisingly...)

jibran’s picture

Status:Postponed» Active
catch’s picture

Version:8.x-dev» 9.x-dev

That issue being committed really means moving this to 9.x.

xjm’s picture

cweagans’s picture

cweagans’s picture

Status:Active» Closed (duplicate)

You know, I'm just going to go ahead and mark this as a duplicate. The other issue has a patch. We can open a new issue to remove the current hook system in the future. #1972304: Add a HookEvent

cweagans’s picture

Issue summary:View changes

Update issue summary with conclusion.