Updated: Comment #N

Problem/Motivation

To register event subscribers currently, you have to add a service with the "event_subscriber" tag.

Considering that we don't use tagged services for discovery consistently in core, this is exposing an unnecessary implementation detail.

I think we can improve DX by adding a dedicated events.yml with a simple schema for registering events and subscribers.

Something like this:


namespace Drupal\rdf;

final class RdfMappingEvents {
  const MAP_TYPES_FROM_INPUT = 'rdf.map_types_from_input';
}

namespace Drupal\rdf\EventSubscriber;

use Drupal\rdf\RdfMappingEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class MappingSubscriber implements EventSubscriberInterface {

  static function getSubscribedEvents() {
    $events[RdfMappingEvents::MAP_TYPES_FROM_INPUT] = 'mapTypesFromInput';
    return $events;
  }
}
services:
  rdf.mapping:
    class: Drupal\rdf\EventSubscriber\MappingSubscriber
    tags:
      - { name: event_subscriber }

Proposed resolution

Could be something like this:

events:
- rdf.map_types_from_input
subscribers: 
  rdf.map_types_from_input: Drupal\rdf\EventSubscriber\MappingSubscriber::mapTypesFromInput

Or generically speaking:

subscribers:
  config.importer.import:
    - callback: config_snapshot_subscriber:onConfigImporterImport
      priority: 40
  config.importer.validate:
    - callback: Drupal\Core\EventSubscriber\ConfigImportSubscriber::onConfigImporterValidate
      priority: 40
  config.save:
    - callback: config.factory:onConfigSave
      priority: 255

For dependency injection we can use the ContainerInjectionInterface

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves DX
Issue priority Major because it affects the way of all event subscriptions
Disruption Disruptive for core/contributed and custom modules/themes because it will change the event subscription logic (from tag:event_subscriber to events.yml)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tsphethean’s picture

Assigned: Unassigned » tsphethean

I can take a look at this.

dawehner’s picture

I fear this would let people move logic into their event subscribers, but yeah we can't prevent that anyway.

msonnabaum’s picture

I dont see how the current way discourages that.

yched’s picture

I'd be very very +1 to that.
We don't have that many events so far, and possibly wont have much in D8 (except contrib can still go wild on them), but if we did, the current human discoverability of "who reacts to what" is terrible. An easy to find registry would help a lot when you are trying to figure out how a random contrib / custom module works.

dawehner’s picture

Well, basically all IDEs has built in support for finding events: Just search for the used places of the constant.

tsphethean’s picture

So here's a very early stab at this, which moves event subscription for user and views modules into module.events.yml and parses them during the building of the container. Tests seem to be passing on my local.

@msonnabaum - couple of things that I wanted to clarify, in the example module.events.yml you posted there was:

events:
- rdf.map_types_from_input

was this intended to register the event, or the subscription to the event? I couldn't figure out why this would be needed for registering the events?

In this patch I've also continued adding the event_subscriber tag to the service definition for the container, so that RegisterKernelListenersPass wouldn't need modifying (it continues to add listeners by looking for tagged events in the container, it's just now we'd force the adding of the tag in the YamlFileLoader::parseEventSubscriberDefinition method) , but I don't know if this is desirable?

Also the current implementation would allow for the content of a module.events.yml to be put in a module.services.yml and still working, because I'm modifying the Yaml load and validate methods - am I better off extending the YamlFileLoader to a YamlEventFileLoader class so we separate the logic?

Grateful for any thoughts.

tsphethean’s picture

Status: Active » Needs review
Crell’s picture

I really don't see how #6 buys us anything. It's the same YAML definition; all we've done is remove the tag from it and move it to a new file, which then effectively becomes a duplicate of the services.yml file but with everything having an event_subscriber tag.

I don't see any DX benefit here that is worth the confusion that will inevitably cause. For this to be useful, we would need an entirely new event registration mechanism separate from EventSubscriberInterface. But I don't see a great deal of value to that for the confusion it would cause with other Symfony-based projects, to say nothing of the confusion within Drupal of yet another way to do things, and the work that will inevitably be required when someone decides that "Drupal must be consistent" so we have to rewrite very frickin' event subscriber we have.

Really, I don't see the point.

msonnabaum’s picture

Assigned: tsphethean » msonnabaum
FileSize
22.99 KB

I appreciate the effort, but I agree that #6 isn't buying us much.

The attached patch is more what I had in mind. It's really just exposing the ability to use symfony's event listeners rather than subscribers. They are simpler, require less boilerplate, and more closely resemble event dispatchers in other languages/frameworks.

I converted a handful of events to show how it would look. The syntax supported is the same as controllers: you can specify class::method or service:method.

Most subscribers are registered as a service soley to declare they are subscribers. These service entries are removed and they use the simple class::method notation in events.yml.

Some listeners are complex enough to warrant needing a service entry, and that's fine. This patch just decouples those ideas. It may be an extra step for the listeners who will need services, but I think they are the minority and the simplicity is worth it.

Many of the subscribers that have dependencies only have the dependency because we strictly separate every subscriber into a dedicated object. Then the subscriber's method simply delegates directly to the dependency. For the simpler classes like ParamConverterManager, I dont think it buys us much to do this. As an example, I removed ParamConverterSubscriber and just registered ParamConverterManager to handle the one event.

I dont think we should do this across the board, but I think we should in the simpler cases. It's a nice example of how contrib can avoid even more boilerplate.

msonnabaum’s picture

Also, I would have preferred to avoid the compiler pass, but we dont really have a choice as long as we're using a compiled container. An event listener who's callable is an instance of a class with a method can't be dumped, so I had to create services for them dynamically in the pass.

Status: Needs review » Needs work

The last submitted patch, eventyaml-2023613-9.patch, failed testing.

Crell’s picture

I'm confused. #9 now has a combination of event subscribers, event subscribers not tagged as event subscribers, and listeners registered only through the events.yml file. Why 3 different patterns? Even I can't figure that out, which means I doubt most module devs will either.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
25.1 KB

Forgot to add the yaml discovery stuff from #2050289: Add class to make yaml discovery reusable, which this patch depends on.

Status: Needs review » Needs work

The last submitted patch, eventyaml-2023613-12.patch, failed testing.

msonnabaum’s picture

@Crell - The final patch would have everything in events.yml as event listeners unless we found a compelling reason to leave some as subscribers. The ones that are there and not tagged are simply services that need their class name changed and I didnt bother.

The question of whether to keep support for tagged event subscribers is up for debate, but I don't think it should be what we encourage contrib to use.

chx’s picture

The win would be to move the whole of getEventSubscribers method into yml and compile the thing in a way that we have per event arrays right in the container so that in case of an event the poor system doesnt need to call every subscriber.

Crell’s picture

Mark: It's not up for debate. Even if we have a different preferred mechanism I see no reason to remove functionality that is common in Symfony, especially as that's an API change at this point.

chx: We could do a compiled EventDispatcher without using a yaml file. I did some skunkworks code on that last year; a compiled dispatcher based solely on subscriber services is totally doable.

msonnabaum’s picture

Sure, I dont care if we leave it in.

effulgentsia’s picture

+++ b/core/core.events.yml
@@ -0,0 +1,20 @@
+subscribers:
+  kernel.request:
+    - callback: Drupal\Core\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestDetermineSiteStatus
+      priority: 40
+    - callback: Drupal\Core\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestMaintenance
+      priority: 30
+++ b/core/core.services.yml
@@ -387,44 +380,24 @@ services:
-  maintenance_mode_subscriber:
-    class: Drupal\Core\EventSubscriber\MaintenanceModeSubscriber
-    tags:
-      - { name: event_subscriber }
+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -58,14 +56,4 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
-  /**
-   * {@inheritdoc}
-   */
-  static function getSubscribedEvents() {
-    // In order to change the maintenance status an event subscriber with a
-    // priority between 30 and 40 should be added.
-    $events[KernelEvents::REQUEST][] = array('onKernelRequestDetermineSiteStatus', 40);
-    $events[KernelEvents::REQUEST][] = array('onKernelRequestMaintenance', 30);
-    return $events;
-  }

I agree that this is much better DX. I'd love to see this get in if it can be done without breaking BC, which seems like it can be.

Just a minor q: do we need the "subscribers" key in the events.yml file? The issue summary includes an "events" key as another possible top-level key, but #13 doesn't have it, and I'm not clear what the utility of that would be. And if there's only one possible top-level key in the file, then why not drop it?

effulgentsia’s picture

Issue tags: +API addition
tim.plunkett’s picture

Fixing tags

neclimdul’s picture

mindless reroll. no changes just fixing conflict with entity services renames and the yaml discovery going in.

letting the trade offs sort of sink in still.

xjm’s picture

webchick’s picture

Priority: Normal » Major

And I think we decided the win here makes this at least major.

xjm’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +beta blocker

Discussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.63 KB

Started some rerole.

damiankloip’s picture

Hang on, so to quote Dries in #2145041: Allow dynamic routes to be defined via a callback:

I'm not sure I'd call it 'Major' but it is certainly a DX improvement

This totally changed what was (not anymore, not an event) the most common event used, yet this is deemed critical now?

Also, we are trying to improve DX here - What about the fact that we just rely on keys defined in YAML with this patch, which you could very easily typo. At least with using event constants you would get a fatal error straight up. This is a big reason for symfony using them. Just saying..

Status: Needs review » Needs work

The last submitted patch, 26: event-2023613.patch, failed testing.

webchick’s picture

Priority: Critical » Major

Yeah, I am not sure why this was moved to critical. What we discussed was making it a beta blocker because in terms of DX, the win here is actually quite large (compare the before/after code), and if we change it after beta we are not providing a stable API.

Restoring priority, keeping the tag.

Crell’s picture

As long as it's confined to just adding a simplified channel for registering events, it's not even beta-blocking as it's a pure API addition. It could be added at any time. Moving existing subscribers from services.yml to events.yml shouldn't be an API change either, since they all still end up in the container anyway.

If we're talking about removing support for registering subscribers via services.yml, then that's a huge and completely unnecessary API change that per #17 I firmly and absolutely oppose.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.62 KB

Added a bunch of unit tests, converted a lot of existing event_subscribers and fixed a bunch of issues which came up while manual testing.

Status: Needs review » Needs work

The last submitted patch, 31: event-2023613.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.62 KB

A space can change a lot.

Status: Needs review » Needs work

The last submitted patch, 33: event-2023613.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.65 KB
533 bytes

The last submitted patch, 35: event-2023613.patch, failed testing.

dawehner’s picture

FileSize
65.43 KB
1.81 KB

.

jibran’s picture

Some doc issues.

  1. +++ b/core/lib/Drupal/Core/EventListenerPass.php
    @@ -0,0 +1,126 @@
    +   * Find the event listeners in event.yml files and add it to the container.
    

    Finds

  2. +++ b/core/lib/Drupal/Core/EventListenerPass.php
    @@ -0,0 +1,126 @@
    +  protected function prepareListener(ContainerBuilder $container, $event_id, $listener) {
    ...
    +  protected function defineService(ContainerBuilder $container, $class) {
    ...
    +  protected function parseCallback($callback) {
    

    doc block missing.

jibran’s picture

Title: Consider moving event registration from services.yml to events.yml » Move event registration from services.yml to events.yml
Status: Needs review » Needs work

We are done with the consideration. we are implementing it now. :)

The last submitted patch, 37: event-2023613.patch, failed testing.

dawehner’s picture

If you look at the current patch you will realize that most of the event subscribers do have dependencies so you will end up with
one entry in the .services.yml file as well as one in the .events.yml file:

services:
     authentication_subscriber:
     class: Drupal\Core\EventSubscriber\AuthenticationSubscriber
  kernel.request:
    # Priority must be higher than LanguageRequestSubscriber as LanguageManager
    # access global $user in case language module enabled.
    - callback: authentication_subscriber:onKernelRequestAuthenticate
      priority: 300

This requirement for 2 different places feels wrong and also not really improves the DX much, too be honest.
It would be cool to be able to do everything in the services.yml file. Symfony allows that by default using the following syntax:

services:
    cmt_admin.exception.action_listener:
        class: Cmt\AdminBundle\EventListener\AdminActionListener
        arguments: [@service_container] [@templating]
        tags:
            -   { name: kernel.event_listener, event: kernel.exception, method: onKernelException }

we could do this a little bit more generic using event_subscriber, event (maybe skipping event_subscriber could even work).

services:
    cmt_admin.exception.action_listener:
        class: Cmt\AdminBundle\EventListener\AdminActionListener
        arguments: [@service_container] [@templating]
        tags:
            -   { name: event_subscriber, event: kernel.exception, method: onKernelException, priority: 12 }
Crell’s picture

#41: That would only work for subscribers that have only a single listener, no?

dawehner’s picture

#41: That would only work for subscribers that have only a single listener, no?

>
No, there is no problem with using multiple tags with the same name.
Damnit, I should have copied the full example from symfonfy (http://symfony.com/doc/current/cookbook/event_dispatcher/before_after_fi...):

# app/config/config.yml (or inside your services.yml)
services:
    demo.tokens.action_listener:
        class: Acme\DemoBundle\EventListener\TokenListener
        arguments: ["%tokens%"]
        tags:
            - { name: kernel.event_listener, event: kernel.controller, method: onKernelController }
            - { name: kernel.event_listener, event: kernel.response, method: onKernelResponse }
dawehner’s picture

Issue summary: View changes
dawehner’s picture

FileSize
71.48 KB
11.34 KB

msonnabaum explained that we not use services but the ContainerInjectionInterface (adapted the issue summary for that).

Crell’s picture

I believe (but could be shown wrong wrong) this means that two listeners in the same class would not fire on the same object; they'd be in 2 separate objects. That means they couldn't share data properties. While an argument could be made that's good (as well as a case for allowing shared object properties for cache purposes or avoiding duplicate shared logic), it's certainly not at all obvious.

msonnabaum’s picture

@Crell - That is documented well in the symfony docs on listeners vs subscribers. We shouldn't default to subscribers, but those who need them won't have trouble finding them.

dawehner’s picture

I believe (but could be shown wrong wrong) this means that two listeners in the same class would not fire on the same object; they'd be in 2 separate objects. That means they couldn't share data properties. While an argument could be made that's good (as well as a case for allowing shared object properties for cache purposes or avoiding duplicate shared logic), it's certainly not at all obvious.

We register a service in the background, so it should be possible to only have one instance at the end.

damiankloip’s picture

That's if you have a service. What about if you just use the dependency injection interface hack? :)

msonnabaum’s picture

I think there was a miscommunication in #45. I'm not suggesting we remove any services with dependencies. It's fine to just use the existing single ":" syntax for those callbacks.

Yes, you now have to edit two files, but I dont think that's a bad thing necesarilly. Creating a service and wiring up an event listener are two totally separate things Yes, they sometimes go together, but not always. When you don't have dependencies, creating a service just so you can listen to an event is considerably more awkward, so I think the tradeoff is reasonable.

Also, in many cases, creating a class soley to listen to events will be overkill, so you can just set the callback to a method on what you would normally inject into the subscriber. I'm not suggesting we do that here though. I'm not particularly interested in that fight.

dawehner’s picture

I think there was a miscommunication in #45. I'm not suggesting we remove any services with dependencies. It's fine to just use the existing single ":" syntax for those callbacks.

Wasn't the point of the issue to improve the DX? This is worse DX from my point of view, especially if we could it do better.

. When you don't have dependencies, creating a service just so you can listen to an event is considerably more awkward, so I think the tradeoff is reasonable.

So good so far, though just the following ones in core.services.yml won't have some: AjaxSubscriber, AjaxResponseSubscriber. All other ones have dependencies.

effulgentsia’s picture

That is documented well in the symfony docs on listeners vs subscribers. We shouldn't default to subscribers

What docs is that? From my read of Symfony docs, it seems that "listener" means either a callable or a class/object that does not implement getSubscribedEvents(), and that "subscriber" means a class/object that does implement getSubscribedEvents(). That seems orthogonal to me to whether an object gets reused or not, declared as a service or not, etc. Speaking of which, since this patch removes that method from all the *Subscriber classes, shouldn't we rename them to *Listener as well?

Wasn't the point of the issue to improve the DX? This is worse DX from my point of view

So currently in this patch, we have *.events.yml files with a single top-level key of "subscribers". What if we change that to "listeners", but also add a "services" top-level key, and allow service definitions of listeners that have dependencies in there?

dawehner’s picture

So currently in this patch, we have *.events.yml files with a single top-level key of "subscribers". What if we change that to "listeners", but also add a "services" top-level key, and allow service definitions of listeners that have dependencies in there?

In order to allow that we would have to support the full container syntax. In core we already have services with sychronized method based injection. I hope we can easily reuse the existing yamlLoader functionality.

Crell’s picture

With subscribers today:

- You edit services.yml
- You edit your code file

That's 2 files.

With events.yml as proposed here:
- You edit events.yml
- You edit your code file
- You may or may not edit services.yml

That's 2-3 files.

So... what's the win here? Adding another way to register events adds more conceptual overhead for developers, since they have to be aware of another way that a module they're working with could be registering events. It's also more effort to document, write books about, etc. There needs to be a trade-off benefit for that. And so far, I'm not really seeing it.

In the generic sense, "sticking stuff in services.yml" translates to "You write your class, then you use services.yml to tell Drupal all it needs to know about your class. The rest is magic." Using events.yml doesn't change that basic equation, even if it does move it around a bit.

effulgentsia’s picture

You write your class, then you use services.yml to tell Drupal all it needs to know about your class. The rest is magic.

The "magic" boils down to tags. Without tags, there's no magic: you just declared a named service that doesn't automagically do anything until some code explicitly instantiates it. But tags create automagic behavior.

The issue summary says:

Considering that we don't use tagged services for discovery consistently in core, this is exposing an unnecessary implementation detail.

I think there's some truth to that: we've made up a bunch of service tags (in addition to the ones Symfony made up), and each one creates automagic in its own special way, so module authors need to learn the quirks of each one that they need to interact with.

Here are the tags currently being used by core modules, followed by the modules that define at least one service with that tag, ordered by the number of modules:

access_check: 
  book, config_translation, contact, content_translation,  edit, field_ui, 
  filter, node, rest, search, shortcut, system, tracker, update, user
event_subscriber: 
  ban, config_translation, content_translation, field_ui, hal, 
  locale, user, views
cache.bin: block, ckeditor, filter, migrate, rest, toolbar, views
breadcrumb_builder: book, comment, forum, system, taxonomy
path_processor_inbound: image, language, system
theme_negotiator: block, system, user
encoder: hal, serialization
needs_destruction: locale, views
normalizer: hal, serialization
paramconverter: locale, views_ui
authentication_provider: basic_auth
entity_resolver: serialization
path_processor_outbound: language
route_enhancer: comment
string_translator: locale

I think that especially for the highly popular tags (access_check, event_subscriber, maybe the next couple as well), it's worth asking whether:
a) they really need to be services in all cases, rather than lightweight callables in the majority/simple cases.
b) it's better DX for their definition to be buried with everything else in *.services.yml, or whether they're common/important enough buckets of Drupal to warrant their own YAML file.

That's not really an answer to #54, just additional background on the question.

msonnabaum’s picture

@Crell - Number of files != complexity. Also, that's completely ignoring what goes *in* the files.

+1 to #55. I don't really know how to argue this issue anymore than I have previously. I just don't see why we should accept that events (what should eventually replace hooks), should be coupled to a service container.

DX isn't about the number of files you edit or the number of characters you have to type, it's about simplicity. My proposal is objectively simpler.

yched’s picture

I agree with #56.

The argument IMO is that
"ease of discovery for a human person evaluating a module on git.drupal.org > module author having to write one additional entry in a yml file"

It's easy to check which hooks a module implements, and that makes a large part of understanding what a module does and how it does it. That discoverability is lost with events, where it becomes much more complicated to figure out who reacts to what.

I guess there can be runtime devel tools that spit this out for you, but that only works if you download and install the module...

Crell’s picture

If you're developing with Drupal, you're downloading and installing a module anyway. That's what devel is for.

I am not convinced by #56's claim that a separate file is "objectively simpler". Certainly it also has to be weighed against the "yet another way of doing the same thing" problem it creates, which does have mental overhead for developers downstream. (i.e., you can't rely on events.yml being the canonical source for all event registrations; even if we ripped out support for tagged-service-registration, which I'd be against doing, listeners can still be registered at runtime anyway, by design.)

Too, if the real issue that is being objected to is that tagged services as a concept are too complicated, then a solution should address tagged services in general, not just events. Per #55, we have as many if not more access checker services as we do event listeners, at least in core. Whether that is the same in contrib or not I don't think any of us can predict at this point. Why are events so special, then, rather than all of the other common tagged services?

I would actually recommend we punt on this question. We have a lot of really new systems and mechanisms in D8. Let's let people try them out in the wild and see if, in practice, putting event listeners in services.yml is as obtuse as some fear it will be. I don't think it will be, in practice, as most modules won't have the zillion services that core.services.yml does, but I could certainly be wrong about that. But let's ride on the one file, one mechanism approach for a bit and get some real-world usage of it.

Then if we find that tagged services really is a big DX problem -- even with documentation, training, and a little experience -- we can add a more universal tagged services alternative in 8.1 or 8.2, with more experience to guide that decision. We have this new release cycle process coming in Drupal 8 for a reason, and this sounds like exactly the sort of situation it can help with.

msonnabaum’s picture

I am not convinced by #56's claim that a separate file is "objectively simpler".

No claim was made that "a separate file" is simpler, I said the proposal was simpler. It's the difference between specifying an event name and a callback, and registering a service (a completely unrelated concept), with a special "tag" (a symfonyism that won't be familiar to anyone outside of that community), then implementing a specific interface with a static method in addition to the callback.

I cannot imagine an argument that the former isn't simpler.

Certainly it also has to be weighed against the "yet another way of doing the same thing" problem it creates, which does have mental overhead for developers downstream. (i.e., you can't rely on events.yml being the canonical source for all event registrations; even if we ripped out support for tagged-service-registration, which I'd be against doing, listeners can still be registered at runtime anyway, by design.)

If we did this, we should use events.yml throughout core as an example of the simplest method. I would guess that this would be by far the most popular method, leaving the tagged service to only those who *really* like that method. I doubt it would be used enough to cause confusion.

Also, this is more an argument to get rid of event-like hooks than methods of registering events.

Too, if the real issue that is being objected to is that tagged services as a concept are too complicated, then a solution should address tagged services in general, not just events. Per #55, we have as many if not more access checker services as we do event listeners, at least in core. Whether that is the same in contrib or not I don't think any of us can predict at this point. Why are events so special, then, rather than all of the other common tagged services?

Tagged services aren't being objected to, I'm simply arguing that they should not be used for high level APIs that the majority of custom modules and contrib will be interacting with.

We've done a pretty bad job thus far of converting event-like hooks to events, which is why I believe there hasn't yet been much concern over this, but when we do convert some of the big ones, it'll be a huge difference in complexity and verbosity. So yes, there are more access checkers, but that's because we haven't converted enough hooks yet.

To illustrate the impact I'm describing, here are some examples showing how flag_node_save would work (yes, I know there are no entity events, but there *really* should be for D8, and most certainly will be for D9).

D7

function flag_node_save($node) {
  // code
}

D8, only method supported currently

flag.services.yml

services:
  flag.node_event_subscriber:
    class: Drupal\flag\NodeEventSubscriber
    arguments: ['@flag.flagger']
    tags:
      - { name: event_subscriber }
namespace Drupal\Flag;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Drupal\node\NodeEvents;
use Drupal\node\Event\NodeSaveEvent;

class NodeEventSubscriber implements EventSubscriberInterface {


  public function construct(Flagger $flagger) {
    $this->flagger = $flagger;
  }

  public function onNodeSave(NodeSaveEvent $event) {
    $node = $event->getNode();
    // code
  }

  static function getSubscribedEvents() {
    $events[NodeEvents::SAVE][] = array('onNodeSave');
    return $events;
  }
}

D8 with proposed patch, using functions

flag.events.yml

subscribers:
  node.save:
    - flag_node_save
function flag_node_save($event) {
  $node = $event->getNode();
  // code
}

D8 with proposed patch, using a class

flag.events.yml

subscribers:
  node.save:
    - Drupal\Flag\NodeListener::onSave
namespace Drupal\flag;

use Drupal\node\Event\NodeSaveEvent;

class NodeListener {

  public function onSave(NodeSaveEvent $event) {
    $node = $event->getNode();
    $flagger = Flag::getFlagger();
    // code
  }
}

D8 with proposed patch, using a service

flag.events.yml

subscribers:
  node.save:
    - flag.node_listener:OnSave

flag.services.yml

services:
  flag.node_listener:
    class: Drupal\flag\NodeListener
    arguments: ['@flag.flagger']
namespace Drupal\Flag;

use Drupal\node\Event\NodeSaveEvent;

class NodeListener {

  public function construct(Flagger $flagger) {
    $this->flagger = $flagger;
  }

  public function onSave(NodeSaveEvent $event) {
    $node = $event->getNode();
    // code
  }
}

Now you might be saying to yourself, "Why would we support functions and classes with static method calls, they are forbidden!" My point in showing these three versions is to point out that enforcing a coding style is none of core's business. The current method forces us to use services, when events have nothing to do with services. Even if your listener has dependencies, you dont need to use a service! The question of whether or not people should be registering their listeners as services is irrelevant. There's simply no reason we need to enforce a "best practice" at the API level. This is the kind of thing that makes people angry about D8.

I would actually recommend we punt on this question. We have a lot of really new systems and mechanisms in D8. Let's let people try them out in the wild and see if, in practice, putting event listeners in services.yml is as obtuse as some fear it will be. I don't think it will be, in practice, as most modules won't have the zillion services that core.services.yml does, but I could certainly be wrong about that. But let's ride on the one file, one mechanism approach for a bit and get some real-world usage of it.

Hopefully the examples above illustrate why this is not something to wait on.

yched’s picture

If you're developing with Drupal, you're downloading and installing a module anyway. That's what devel is for.

I strongly disagree. With my site builder hat on, I spend a lot of time on git.drupal.org looking at modules code. That's much more revealing that reading the project page, and a couple minutes are often enough to reject the module or take it for a live evaluation.

If D8 means you cant easily understand how a module is structured and plugs with which parts of the system by looking at its code, but instead need to actually download and run it locally so that some drush tool can analyze it for you, I'd say we have made a step backwards. Human discoverability is a feature.

@msonnabaum's examples in #59 are pretty telling on that aspect.

dawehner’s picture

My proposal earlier:

flag.services.yml

services:
  flag.node_listener:
    class: Drupal\flag\NodeListener
    arguments: ['@flag.flagger']
    tags:
      - {name: event_subscriber, event: node.save, callback: onSave }
namespace Drupal\Flag;
use Drupal\node\Event\NodeSaveEvent;
class NodeListener {
  public function construct(Flagger $flagger) {
    $this->flagger = $flagger;
  }
  public function onSave(NodeSaveEvent $event) {
    $node = $event->getNode();
    // code
  }
}
Crell’s picture

While I'd be OK with #61, I don't think that addresses msonnabaum's issues. Which, if I could simplify (Mark, correct me if I'm misstating), boil down to "events aren't services so shouldn't be in the services.yml file".

While I agree that an event listener doesn't really fit the canonical definition of a service (semi-global stateless single-purpose "doer of a thing"), the DIC we're using, in practice, handles more than pure services. Parameters aren't really services; All tagged-service use is not, strictly speaking, pure-service-logic; the current user object isn't really a service; the request as a service certainly doesn't make sense by that standard, etc. That is, the Container object we have is already being used for far more than pure-services; Event subscribers are the least of the offenders in that regard.

I'm sure this is going to sound like a classic logical fallacy ("one person did it wrong so it's OK for everyone to", whatever that fallacy is called), but my point is rather that it makes little sense, to me, to purify the container in one area but not in all the others that are, actually, worse offenders. That's inconsistent, and I believe would lead to worse DX overall. And, for better or worse, the DIC we're using actively supports these ancillary uses.

As a side note:

Also, this is more an argument to get rid of event-like hooks than methods of registering events.

(yes, I know there are no entity events, but there *really* should be for D8, and most certainly will be for D9).

I agree 1000%, and am quite disappointed that we backed off on converting node hooks to events. That's such an obvious place to do so I think it was a mistake not to, but at this point in the dev cycle I don't think Dries and Co. would authorize any more such direct conversions.

msonnabaum’s picture

@Crell - Wrong logical fallacy. I think you got straw man there :)

It's not about putting things in the DIC that aren't "pure services," it's about using the DIC as a discovery mechanism rather than a DIC.

msonnabaum’s picture

Also, #61 makes the situation worse. Simplifying the registration of events means reducing the concepts and APIs involved, not reducing the number of characters you have to type.

dawehner’s picture

I totally agree that typing is not a proper metric for measuring DX.

What I would really hate as a developer though would be to put information into 3 different files, which you basically would all need, as 80% of all events need stuff injected. You first look into the events yml file, then you go to the services yml file to actually know which class is involved vs. just look up the stuff in the services yml file. I really can not see how your approach involves less concepts.

msonnabaum’s picture

That argument could be used to say that nearly all forms of discovery should be tagged services, but surely we're not going to do that, right?

Again, having dependencies != needing an entry in the DIC. I know we've been dogmatic about it in core, but it's simply unnecessary in many cases. We can't come up with a decent estimate based on core, because as I stated before, we haven't yet converted some of our most obvious use cases. 80% is just false, we dont know yet.

My approach involves fewer concepts because it doesn't *require* a service, just as creating a controller or a views plugin doesn't require creating a service. It's an unnecessary concept to couple to discovery.

neetu morwani’s picture

Assigned: msonnabaum » neetu morwani
dawehner’s picture

My approach involves fewer concepts because it doesn't *require* a service, just as creating a controller or a views plugin doesn't require creating a service. It's an unnecessary concept to couple to discovery.

I guess you are right in the case where we use events in many many places, though I still wonder whether this is worth to make it hard for the good-practise one.

Xano’s picture

If this file is for event listeners and not for events, can we please name it accordingly? We don't want to end up in a situation where we are working on exposing actual events and then realize we shot ourselves in the foot in January 2014.

+999 for the general idea, though :)

xjm’s picture

This is either critical, or not a beta blocker. Which? :)

msonnabaum’s picture

At this point I'm just waiting for someone like webchick to see #59 and freak out.

msonnabaum’s picture

At this point I'm just waiting for someone like webchick to see #59 and freak out.

effulgentsia’s picture

@xjm: there isn't consensus on that. See #25 and #29.

There also isn't consensus on what the solution should be, because both #59 and #68 are good points. I'm still in favor of msonnabaum's proposal, but don't really know how to address #68 yet.

dawehner’s picture

I really fear that using the events.yml approach pushes people towards using \Drupal for events and or not writing decoupled components.

msonnabaum’s picture

Again, the argument in #74 could be used to argue that *all* discoverables in Drupal should be tagged services, and we clearly aren't doing that.

The event system should have no opinion about coupling because it's not the event system's concern.

dawehner’s picture

Well, the difference is that the other discoverables aren't base components but regular swappable pieces.
Do you want to let subscribers just use services and defer the logic?

msonnabaum’s picture

I have no opinion on what people decide to do with their subscribers. I only care that we can register a callback for an event.

effulgentsia’s picture

I guess you are right in the case where we use events in many many places, though I still wonder whether this is worth to make it hard for the good-practise one.

I think what we want to optimize DX for is modules (both core and contrib) rather than for core/lib. With that in mind, here's the 9 current event subscribers in HEAD provided by core, non-test modules:

(ban) BanSubscriber: depends on 1 module service
(content_translation) ContentTranslationRouteSubscriber: depends on 1 module service
(hal) HalSubscriber: no dependencies
(field_ui) RouteSubscriber: depends only on core services
(language)  ConfigSubscriber: no dependencies
(language) LanguageRequestSubscriber:  depends on 2 module services
(system) SystemConfigSubscriber: no dependencies
(user) MaintenanceModeSubscriber: depends only on core services
(views) RouteSubscriber: depends only on core services

What we have is 3 subscribers with no dependencies, 3 with dependencies only on services defined in core.services.yml, and 3 with dependencies on a module-defined service (in all cases, a service defined by the same module).

My understanding of #2134513: [policy, no patch] Trait strategy is that once we fix bot to run PHP 5.4, we'll be able to write traits with getter and setter methods for all services in core.services.yml (we don't have to do all of them, but we can choose to do all the ones that make sense), where the getter method could return the service that's in \Drupal:: if the setter method hasn't been called, and that there's nothing about this that isn't "good practice", because you can still unit test the class by having the test invoke the setter method.

This would then potentially mean that 6 of the 9 subscribers have no need for dependency injection, and therefore, no need for an entry in *.services.yml.

If we manage to figure out #1972304: Add a HookEvent, then contrib will also be able to implement listeners for hooks like hook_node_view() / hook_node_view_alter(), which very often also don't have dependencies or only have dependencies on core services that could be accessed through traits.

So, I think what this issue represents is much better DX for event listeners that have no need to be in services.yml, and only marginal inconvenience for ones that do. And while it's hard to know this with any precision, based on the above, I could easily see >50% of good-practice contrib event listeners being in the former camp.

So, for me, this issue is an overall improvement. @dawehner: any thoughts on this reasoning?

I really fear that using the events.yml approach pushes people towards using \Drupal for events and or not writing decoupled components.

Does my claim about traits address this concern? If not, do you also have this concern about route controllers, and if not, why the difference?

Crell’s picture

effulgentsia: I consider the "service traits" we are planning to add to be "application level only". (Controllers, some plugins, etc. Things that have utility base classes now.) Domain logic classes and infrastructure classes should stick to "normal" injection. Which then begs the question of whether event listeners are application level logic or not. My gut feeling is that it will vary a bit with the event listener.

effulgentsia’s picture

My gut feeling is that it will vary a bit with the event listener.

@Crell: my opinion is that the 3 subscribers in #78 that say "depends only on core services" are application level. Do you agree or disagree?

Crell’s picture

The field_ui and views Route subscribers, probably. The Maintenance Mode subscriber is, I think, highly debatable whether it's application level or framework level.

dawehner’s picture

Status: Needs work » Needs review
FileSize
72.82 KB

My understanding of #2134513: [policy, no patch] Trait strategy is that once we fix bot to run PHP 5.4, we'll be able to write traits with getter and setter methods for all services in core.services.yml (we don't have to do all of them, but we can choose to do all the ones that make sense), where the getter method could return the service that's in \Drupal:: if the setter method hasn't been called, and that there's nothing about this that isn't "good practice", because you can still unit test the class by having the test invoke the setter method.

Traits are a really good tool for services which don't really add business value. One good example is the translation manager. In case you have a service you really should better explicit define your dependencies.

Rerolled the patch before the suggestion, people seem to disagree, this is fine.

Status: Needs review » Needs work

The last submitted patch, 82: event-2023613-82.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
79.12 KB
9.88 KB

Just a couple of fixes. Note: we have to support optional services

Status: Needs review » Needs work

The last submitted patch, 84: event_subscriber-2023613-84.patch, failed testing.

catch’s picture

Issue tags: -beta blocker +beta target

This is either not a beta blocker or it should be critical, can't block the beta on non-critical stuff.

Switching to beta target - if you think it should block, please bump to critical.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
79.24 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 87: event_subscriber-2023613-87.patch, failed testing.

The last submitted patch, 87: event_subscriber-2023613-87.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
86.89 KB
16.77 KB

Additional two expanding the documentation this also fixes quite a bunch of the failures, hopefully.

Status: Needs review » Needs work

The last submitted patch, 90: event_subscriber-2023613-90.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
87.57 KB

.

tim.plunkett’s picture

Assigned: neetu morwani » Unassigned
Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/EventListenerPass.php
    @@ -0,0 +1,228 @@
    + * Definition of Drupal\Core\EventListenerPass
    

    Contains \Drupal\

  2. +++ b/core/lib/Drupal/Core/EventListenerPass.php
    @@ -0,0 +1,228 @@
    + * Additional you can also specify a priority:
    

    Additionally

  3. +++ b/core/lib/Drupal/Core/EventListenerPass.php
    @@ -0,0 +1,228 @@
    +    // TODO: This logic should abstracted somewhere.
    

    I think we can link to #2165475: Provide a generic class resolver here? And @todo

  4. +++ b/core/modules/user/user.events.yml
    @@ -0,0 +1,4 @@
    +subscribers:
    +  kernel.request:
    +    - callback: Drupal\user\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestMaintenance
    +      priority: 35
    
    +++ b/core/modules/user/user.services.yml
    @@ -21,10 +21,6 @@ services:
    -  user_maintenance_mode_subscriber:
    -    class: Drupal\user\EventSubscriber\MaintenanceModeSubscriber
    -    tags:
    -      - { name: event_subscriber }
    

    Changes like this make sense to me.

  5. +++ b/core/modules/views/views.events.yml
    @@ -0,0 +1,3 @@
    +subscribers:
    +  routing.route_dynamic:
    +    - Drupal\views\EventSubscriber\RouteSubscriber::dynamicRoutes
    

    This, not so much. Where is the corresponding change?

This is pretty rough for services, since it means a third place to look for things...

dawehner’s picture

This is pretty rough for services, since it means a third place to look for things...

Let me quote from above.

What we have is 3 subscribers with no dependencies, 3 with dependencies only on services defined in core.services.yml, and 3 with dependencies on a module-defined service (in all cases, a service defined by the same module).

My understanding of #2134513: [policy, no patch] Trait strategy is that once we fix bot to run PHP 5.4, we'll be able to write traits with getter and setter methods for all services in core.services.yml (we don't have to do all of them, but we can choose to do all the ones that make sense), where the getter method could return the service that's in \Drupal:: if the setter method hasn't been called, and that there's nothing about this that isn't "good practice", because you can still unit test the class by having the test invoke the setter method.

This would then potentially mean that 6 of the 9 subscribers have no need for dependency injection, and therefore, no need for an entry in *.services.yml.

Sadly the assumption of using traits for DI might not be a proper one. Personally I would like to just give people possibilities not fences.

effulgentsia’s picture

Sadly the assumption of using traits for DI might not be a proper one.

In #81, Crell said that the field_ui and views route subscribers are probably application-level, rather than infrastructure or domain level. The former currently has an entity manager as the only constructor param, and the latter has entity manager and state. I think we're agreed in #2134513: [policy, no patch] Trait strategy that EntityManager and State will have traits. If that's the case, and if we're agreed on these subscribers being application level, then does that mean we can/should use the traits instead of the constructor args in them?

I would like to just give people possibilities not fences

The patch doesn't prevent contrib from using the 'event_subscriber' tag if they want to, right? So then the question is whether we promote that as a "Drupal Approved (TM)" practice (and use it in a couple places in core) or as something we prefer people to not do. The benefit of not doing it in core, and asking contrib to not do it as a general rule, is easier discoverability (for humans) per #57. Any thoughts on how to strike that balance?

effulgentsia’s picture

Any thoughts on how to strike that balance?

How about this?
- For subscribers in core/lib, we stick to what's in HEAD: i.e., an 'event_subscriber' tag and the static registration method inside the class.
- For core/modules, we generally use events.yml, unless there's a really good reason not to (TBD what those reasons might be). In cases where we don't use events.yml, we place the subscriber class in Drupal\MODULENAME\EventSubscriber\, which makes it easy for a human to discover all of them.

effulgentsia’s picture

--- /dev/null
+++ b/core/modules/ban/ban.events.yml

@@ -0,0 +1,4 @@
+subscribers:
+  kernel.request:
+    - callback: ban.subscriber:onKernelRequestBannedIpCheck
+      priority: 40

When we remove the static method registration in the class, they go from being subscribers to listeners. So I think we should change the top-level key in the YAML to listeners. Or, per #69, perhaps rename the file to *.event_listeners.yml, and then remove the top-level key entirely?

msonnabaum’s picture

This is pretty rough for services, since it means a third place to look for things...

That's one way to look at it. Another would be, you now have a sensible place to see what events a module subscribes to, rather than it being in services, which is not at all obvious, considering we still use hooks, plugins/annotations for discovery. An argument to leave events in services.yml for discoverability is an argument to move practically all discoverables to tagged services.

Also, I still find that any talk about DI is a distraction and not relevant to the issue.

dawehner’s picture

Status: Needs work » Needs review
FileSize
88.04 KB

+++ b/core/modules/views/views.events.yml
@@ -0,0 +1,3 @@
+subscribers:
+ routing.route_dynamic:
+ - Drupal\views\EventSubscriber\RouteSubscriber::dynamicRoutes
This, not so much. Where is the corresponding change?

Oh yeah, this is certainly an error. Can we please talk about whether we need to be able to keep the RouteSubscriberBase?

How about this?
- For subscribers in core/lib, we stick to what's in HEAD: i.e., an 'event_subscriber' tag and the static registration method inside the class.
- For core/modules, we generally use events.yml, unless there's a really good reason not to (TBD what those reasons might be). In cases where we don't use events.yml, we place the subscriber class in Drupal\MODULENAME\EventSubscriber\, which makes it easy for a human to discover all of them.

Here is the latest patch

dawehner’s picture

FileSize
1.89 KB

And the interdiff because drupal.org does not allow me to upload two files at the same time, for some reason.

Status: Needs review » Needs work

The last submitted patch, 99: event_subscriber-2023613-95.patch, failed testing.

Dries’s picture

Looking at Mark's example in #59 (link: #2023613-59: Move event registration from services.yml to events.yml), this looks like a fantastic change. It gets us from 'this is really hard' to 'this is great'. I don't believe this has to block beta, but I would really love to see us ship with this in Drupal 8.

dawehner’s picture

Status: Needs work » Needs review
FileSize
87.08 KB

.

Status: Needs review » Needs work

The last submitted patch, 103: event_subscriber-2023613-103.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
88.03 KB
1.25 KB

The other ones aren't reproducable, let's have a look whether they work this time.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 105: event_subscriber-2023613-105.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
86.75 KB

Merged 8.x, but might have resolved some conflicts incorrectly.

Status: Needs review » Needs work

The last submitted patch, 108: event_subscriber-2023613-108.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
101.63 KB

Fixed hopefully many of the missing pieces of the reroll.
I still think that it is a huge DX regression to require to touch 3 files in most usecases. I really think that core modules covers the common/recommended way to achieve stuff.

Just some small stats:
We do have 53 services and 19 callbacks including core and 18, 10 excluding core.

Status: Needs review » Needs work

The last submitted patch, 110: events-2023613-110.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

110: events-2023613-110.patch queued for re-testing.

The last submitted patch, 22: drupal8.base-system.2023613-22.patch, failed testing.

The last submitted patch, 45: events-2023613.patch, failed testing.

dawehner’s picture

FileSize
101.5 KB

On top of that the uploaded patch fixes the trailing whitespaces.

sun’s picture

I've read the entire issue and discussion (which, fwiw, could have stopped around #20, since arguments were only repeated later on).

My impression of the current state is that @Crell still disagrees that the proposed change is an improvement, @msonnabaum mostly argues on semantic grounds, and some others like or dislike it because of DX (in either direction), and some more people didn't state anything that would help to evaluate their feedback (including @Dries).

What I like about the proposed change:

  1. Single registry (per extension) that lists event listeners including priority.

  2. Supports callbacks both in classes and services.

What concerns me about it:

  1. New, additional way for doing the same thing. (defeats 1. above)

  2. To get dependencies properly injected, the class of an event listener has to be registered in two instead of one place.

  3. This custom approach is unknown and thus not documented anywhere outside of Drupal. — Though admittedly, our current approach using 'event_subscriber' tags doesn't appear to be documented practice either.

  4. Various DX concerns on the actual patch/implementation:

    1. Strange single colon (:) syntax instead of existing @-prefix service notation. (inconsistency)
    2. The top-level key/property in the events.yml files is still "subscribers", even though what follows is a list of "listeners". (confusing)
    3. It's not clear to me whether events.yml can also be used to register a subscriber class? (consistency)

Considering these pros/cons, I'm ambivalent on the proposed change. The net win/loss in terms of DX and architecture appears to be neutral. I could live with and without it.


Lastly, I was pointed here from https://drupal.org/node/2158571#comment-8703881 after mentioning the following...

Hard-coded priorities require you to know the hard-coded priorities of all of your dependencies — not just one or two, but all. And all of those can change without notice at any time, causing your module/subscriber to break at any time, out of a sudden.
[...]
[This] is a much more general flaw that ought to be a separate, critical, beta-blocking issue in my book. Symfony's EventDispatcher was designed for a piece-meal application framework that is under control of a single developer; it is not aware of the concept of a modular, extensible application, in which event subscribers are NOT under control of any single party. We can't release without an EventDispatcher that is able to operate on relative dependencies. Doing so would seriously screw up contrib for the entire lifetime of D8.

...so I actually expected to see a much larger change (than just juggling some definitions/registrations around) — namely:

Replacing all the priorities with "before" and "after" references to other event listeners. If the referenced definitions exist at compile time, then the listeners are Graph-sorted accordingly. If none of the references exist, then the order of a listener does not matter.

I'm not sure whether that is on-topic or off-topic here. But compared to the neutral evaluation result above, that would truly resolve a major architectural problem, and the idea of declaring event listeners in a separate file would inherently make much more sense.

dawehner’s picture

Well, from my perspective we could need something like this issue in order to be able to resolve your more advanced plans. We need a central place to change and collect all the subscriber data,
not done on runtime, probably.

Tbh. I would really like to skip the primary key: what is the point of a primary key if there is just one. We don't use XML here.

Wim Leers’s picture

Replacing all the priorities with "before" and "after" references to other event listeners. If the referenced definitions exist at compile time, then the listeners are Graph-sorted accordingly. If none of the references exist, then the order of a listener does not matter.

FYI: sdboyer has been working on precisely that with his asset libraries/Assetic patch. Exactly the same pattern/logic. He might be interested in this issue as well.

dsnopek’s picture

+1000 on the idea of "before" and "after" for events! @Wim Leers: Thanks for pointing out @sdboyer's work. I think this is the issue: #1762204: Introduce Assetic compatibility layer for core's internal handling of assets

sdboyer’s picture

@dsnopek - that is the issue, but the graph library is already in core. what may be useful to look at in that patch is the AssetGraph, which demonstrates how 'before'/'after' data can be turned into proper edges at the time a vertex is added to the graph.

fwiw, gliph is designed with speed in mind, specifically so that it is feasible for use at request time - for cases exactly like this. i'd been toying with the idea of making a patch for Symfony that creates something like this upstream, but doing it in Drupal first makes perfect sense.

catch’s picture

While it might be optimized for speed, I very much doubt it'd be feasible to do event listener ordering at request time. Event listeners are a lot more numerous than assets, and they'll only get more so as time goes on.

sdboyer’s picture

@catch - depends on what's being done, and what can be cached. it's certainly more expensive than a foreach across a linear list. of course, in this case, one might also be able to take it out of the request time context by introducing a step in container building that transforms before/after information into a topologically sorted list. then we're back to linear traversal.

both of our assertions seem to be the kind that are supposed to be tested, rather than used as reasons, absent evidence, for any course of action.

EDIT: memoization is perhaps a better word than caching here. even if the linear list isn't calculated at container build time, state of a graph is easily snapshotted (just hash the containing object's data); that hash can serve as the basis for memoizing the linear tsl.

msonnabaum’s picture

Reminder that we've been able to do this with the graph code we've had since D6: https://drupal.org/project/module_order

msonnabaum’s picture

Also, while it is an expansion of scope, I think before/after is a good idea that could be done in a followup.

sun’s picture

@catch / @sdboyer: I was told that we have 1-2 use-cases for dynamically added event subscribers/listeners in core right now, but those are extreme edge-cases and do not represent the 99%.

It should be fine to only support dependency-sorted listeners if they're registered at container compile time.

At runtime, after compilation, your possibilities are reduced to first/last (append/last being the default), and if that is not sufficient, then you need to manually introspect the full stack of listeners anyway.


In addition, I forgot to mention one major remark:

A proper before/after resolution for event listeners is essentially a newborn incarnation/resolution for the epic, unresolved idea of a "hook registry."

The only reason for why hook_module_implements_alter() exists today is the lack of an event listener architecture that has relative dependencies natively built-in.

Properly resolving dependencies requires to have a (pre-dependency-resolved) registry readily at hand at runtime. Instead of module_implements cache, the information is precomputed and preloaded instead. There are lots of listeners + hooks, so that requires more memory, and thus negatively affects performance.

Performance was and is the only reason for why the module hook system still exists.

→ Resolve relative dependencies. → Kill hook_module_implements_alter() → Kill hooks.

Crell’s picture

Scope Creep

Sorry, someone had to say it. Fancy graph ordering of listeners is severely out of scope here. Let's also recall that we've managed with module weights for 12 years. The actual needed use cases of hook_module_implements_alter() are astonishingly few. Even listeners that don't care about order can just do nothing. Those that do already have more flexibility, today, than hooks do.

Also sun, no, the primary reason hooks still exist is we've not had anywhere close to the time to refactor them away properly.

dsnopek’s picture

So, I'll concede that this is probably scope creep and probably not something we can do for D8, given that we need to get to -beta and stop changing things so much. Also, I don't know how wide-spread the use cases for hook_module_implements_alter() (and hence fancy graph ordering) are, but...

I can say that I constantly run into hook ordering issues in work on Drupal distributions (I'm one of the maintainers of Panopoly and do a lot of Open Atrium work) and seem to use hook_module_implements_alter() all the time. So, maybe this wouldn't help most module developers in their daily lives, but it would help me in mine. :-)

dawehner’s picture

Yeah this issue is orthogonal to the potential ordering issue.

dawehner’s picture

Rerolled the latest patch, before the scope creeping began.

Status: Needs review » Needs work

The last submitted patch, 129: event_subscriber-2023613-129.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
102.21 KB
546 bytes

Removed the non existing listener.

Status: Needs review » Needs work

The last submitted patch, 131: event_subscriber-2023613-131.patch, failed testing.

dawehner’s picture

I still like the bit of the patch that it removes some bits of the dependency to the EventDispatcher component for your code.

I doubt though this will happen as it is too much of an API change. Anyone wants to disagree or we can just move this to 9.x directly?

Wim Leers’s picture

Issue tags: +beta deadline
xjm’s picture

Tagging as a priority for the beta sprint, and removing redundant tag. Agreed that this is a pretty big API change.

Crell’s picture

Issue summary: View changes

Note that Symfony fullstack supports adding event listeners via a subscriber class, as we do currently, or by putting the event-priority-method linkage into tags:

http://symfony.com/doc/current/components/event_dispatcher/introduction....

I don't think we support that currently, but we could. (I'm not sure of the performance implications on when the class loads, though.) That would mean we support multiple first-class ways to register an event, but does then let us use the services.yml file as an all-in-one file for event registration for those who want it.

dawehner’s picture

@crell

Note that Symfony fullstack supports adding event listeners via a subscriber class, as we do currently, or by putting the event-priority-method linkage into tags:

Just in case you want to know that we discussed around that idea, see #2023613-61: Move event registration from services.yml to events.yml, this didn't seemed to be a really liked idea.
Afaik symfony just supports it for kernel listeners. They have separate dispatchers for different concerns, one thing we don't do at the moment.

Crell’s picture

AFAIK there are only 3: The main dispatcher (used for most things), the form dispatcher (which is form-instance-specific, I think), and Doctrine (which doesn't even use the EventDispatcher component as its a stand-alone system). It's actually rather annoying that Doctrine doesn't use the same dispatcher.

And ha, yes we did. And my preference for that approach over yet another YAML file still stands. :)

xjm’s picture

@dawehner and @effulgentsia suggested this is too much of an API change at this point. We could add support for events.yml in 8.1.x or another minor version so long as we retained BC for registration in the services.yml.

So removing from the sprint list. Not actually bumping to 8.1.x yet, but we should consider doing so.

valthebald’s picture

Issue summary: View changes

Added beta evaluation

Crell’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

Per #139.

MustangGB’s picture

Issue tags: -beta deadline
dawehner’s picture

Status: Postponed » Active

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.

fgm’s picture

One issue msonnabaum touched on is the ability to directly declare event listeners, not just subscribers.

This is a problem I encountered recently when porting Heisencache to D8: needing configurable services, the static getSubscriberEvents() wasn't usable: I needed these service instances (with their dynamic configuration) and I had to add a specific CompilationPass just to be able to register them as EventListener : a mechanism allowing a dynamic discovery beyond what EventsubscriberInterface allows would have been a boon.

joachim’s picture

> $events[RdfMappingEvents::MAP_TYPES_FROM_INPUT] = 'mapTypesFromInput';

The issue summary doesn't say how the yml will declare the priority on subscribers, or how to declare multiple methods for the same event.

Wim Leers’s picture

Issue summary: View changes

This was not my proposal, but I amended the proposal to address #150's legitimate concerns.

dawehner’s picture

I think the change of the proposal in #151 is a bit weird.
We should rather do one step at a time

a) Allow to define things in a yml file
b) Allow to define before and after, which is discussed in #2352351: Add before/after ordering to events

For sanity I would really suggest to not mix up both things.

Wim Leers’s picture

Then how do you want to solve

The issue summary doesn't say how the yml will declare the priority on subscribers

?

pounard’s picture

I'm sorry to flame here, but regarding:

To register event subscribers currently, you have to add a service with the "event_subscriber" tag.

Considering that we don't use tagged services for discovery consistently in core, this is exposing an unnecessary implementation detail.

I still don't understand this. If you're using Symfony, you should embrace it: tags are really great to use. I can't understand why you would really want to do an overly useless Drupal-ism where Symfony does solve the problem in a very clean and efficient manner.

Moreover, container tags is a widely known and understood mecanism in the Symfony community, you're, in my opinion, once again stepping back from it by adding a new pure Drupal layer that will confuse newcomers.

I don't want to be rude, but I hope this external point of view might lead you to reconsider getting away from Symfony so much whereas Drupal should embrace it.

fgm’s picture

AIUI, the very real problem is the lack of discoverability of listeners/subscribers. However, seconding @pounard here, instead of adding a new Drupalism in these days where SF3/4 is becoming more and more widespread, we could just add discoverability for these, e.g. in devel.module, to avoid raising our entry barrier.

Here is a simple gist I just did which does just that as a Drush 9 command:

https://gist.github.com/FGM/136f68e3f30c2c2b9eb9687fd9ddcfc4

dawehner’s picture

Issue summary: View changes

Then how do you want to solve

The issue summary doesn't say how the yml will declare the priority on subscribers

It is a bit weird that you didn't had the idea to look at the patch itself, nevermind I updated it with the last patch :)

To be honest though there are a couple of issues (including the one @pounard and @fgm mention):

  • Yet another custom thing. Everyone should know about services already
  • It is not obvious how to have dependency injection for those. Do we want to have DependencyInjectionInterface for those subscribers as well?
  • Noone seemed to have identified the registration of event subscribers to be a major pine after the release, especially compared to other things.

Overall I think the impact of #2352351: Add before/after ordering to events would be higher, especially given that this will give additional safety.

pounard’s picture

And another note, historically, Drupal 8 did provide its own version of the EventDispatcherInterface (which is not compatible with Symfony's core ContainerAwareEventDistpatcher - now just EventDispatcher in Symfony 4, this gets interesting but we'll see that later) - mainly for performance reasons. But as of today, Symfony did great improvements in that regard, and Symfony 3.4 / 4.0 even more. Good bye ContainerAwareEventDispatcher, it's now much more lazy.

Maybe at some point, as a side task (not this issue, but as we speak of it...) maybe it will be time to get rid of Drupal 8 custom implementation and rejoin the way Symfony does it - one problem would only be that they rely on a PHP-dumped compiled container, this is actually on what a lot of performance improvements on the container do rely - but I'm sure there's ways to adapt that to Drupal container.

andypost’s picture

pounard’s picture

@andypost nice thanks.

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.

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.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bradjones1’s picture

There are a number of other issues which are close to being "done" that enable utilizing Symfony's DI constructs more natively. Specifically apropos here is #3021803: Document and add tests for service autowiring, which enables autowiring; I believe a natural next step would be #3108565: Update the Yaml file Loader functionality to simplify service configuration, which incorporates from upstream more DI functionality.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture