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:

<?php
namespace Drupal\rdf;
final class
RdfMappingEvents {
  const
MAP_TYPES_FROM_INPUT = 'rdf.map_types_from_input';
}
?>

<?php
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

For dependency injection we can use the ContainerInjectionInterface

Remaining tasks

User interface changes

API changes

Original report by @username

Files: 
CommentFileSizeAuthor
#115 events-2023613-110_0.patch101.5 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,549 pass(es).
[ View ]
#110 events-2023613-110.patch101.63 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,527 pass(es).
[ View ]
#108 event_subscriber-2023613-108.patch86.75 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 56,369 pass(es), 392 fail(s), and 13,619 exception(s).
[ View ]
#105 interdiff.txt1.25 KBdawehner
#105 event_subscriber-2023613-105.patch88.03 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch event_subscriber-2023613-105.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#103 event_subscriber-2023613-103.patch87.08 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,610 pass(es), 22 fail(s), and 0 exception(s).
[ View ]
#100 interdiff.txt1.89 KBdawehner
#99 event_subscriber-2023613-95.patch88.04 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,055 pass(es), 30 fail(s), and 0 exception(s).
[ View ]
#92 event_dispatcher-2023613-62.patch87.57 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,094 pass(es).
[ View ]
#92 interdiff.txt2.07 KBdawehner
#90 interdiff.txt16.77 KBdawehner
#90 event_subscriber-2023613-90.patch86.89 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,064 pass(es), 0 fail(s), and 149 exception(s).
[ View ]
#87 event_subscriber-2023613-87.patch79.24 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,765 pass(es), 366 fail(s), and 212 exception(s).
[ View ]
#84 interdiff.txt9.88 KBdawehner
#84 event_subscriber-2023613-84.patch79.12 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch event_subscriber-2023613-84.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#82 event-2023613-82.patch72.82 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 60,069 pass(es), 5,528 fail(s), and 1,260 exception(s).
[ View ]
#45 interdiff.txt11.34 KBdawehner
#45 events-2023613.patch71.48 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch events-2023613.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 interdiff.txt1.81 KBdawehner
#37 event-2023613.patch65.43 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#35 interdiff.txt533 bytesdawehner
#35 event-2023613.patch65.65 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,705 pass(es), 356 fail(s), and 382 exception(s).
[ View ]
#33 event-2023613.patch65.62 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#31 event-2023613.patch65.62 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,169 pass(es), 335 fail(s), and 16,490 exception(s).
[ View ]
#26 event-2023613.patch24.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 41,955 pass(es), 4,426 fail(s), and 3,368 exception(s).
[ View ]
#22 drupal8.base-system.2023613-22.patch22.78 KBneclimdul
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.base-system.2023613-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 eventyaml-2023613-12.patch25.1 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 52,814 pass(es), 2,606 fail(s), and 1,824 exception(s).
[ View ]
#9 eventyaml-2023613-9.patch22.99 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 event_subscription-2023613-6.patch5.74 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 56,475 pass(es).
[ View ]

Comments

Assigned:Unassigned» tsphethean

I can take a look at this.

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

I dont see how the current way discourages that.

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.

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

StatusFileSize
new5.74 KB
PASSED: [[SimpleTest]]: [MySQL] 56,475 pass(es).
[ View ]

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.

Status:Active» Needs review

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.

Assigned:tsphethean» msonnabaum
StatusFileSize
new22.99 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new25.1 KB
FAILED: [[SimpleTest]]: [MySQL] 52,814 pass(es), 2,606 fail(s), and 1,824 exception(s).
[ View ]

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.

@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.

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.

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.

Sure, I dont care if we leave it in.

+++ 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?

Issue tags:+API addition

Fixing tags

StatusFileSize
new22.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.base-system.2023613-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Priority:Normal» Major

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

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.

Status:Needs work» Needs review
StatusFileSize
new24.63 KB
FAILED: [[SimpleTest]]: [MySQL] 41,955 pass(es), 4,426 fail(s), and 3,368 exception(s).
[ View ]

Started some rerole.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new65.62 KB
FAILED: [[SimpleTest]]: [MySQL] 59,169 pass(es), 335 fail(s), and 16,490 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new65.62 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

A space can change a lot.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new65.65 KB
FAILED: [[SimpleTest]]: [MySQL] 59,705 pass(es), 356 fail(s), and 382 exception(s).
[ View ]
new533 bytes

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

StatusFileSize
new65.43 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.81 KB

.

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.

Title:Consider moving event registration from services.yml to events.ymlMove 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.

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 }

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

#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 }

Issue summary:View changes

StatusFileSize
new71.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch events-2023613.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new11.34 KB

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

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.

@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.

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.

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

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.

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.

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?

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.

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.

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.

@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.

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...

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.

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

<?php
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 }

<?php
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

<?php
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

<?php
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']

<?php
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.

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.

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 }

<?php
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
 
}
}
?>

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.

@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.

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.

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.

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.

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.

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 :)

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

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

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

@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.

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

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.

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?

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.

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?

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new72.82 KB
FAILED: [[SimpleTest]]: [MySQL] 60,069 pass(es), 5,528 fail(s), and 1,260 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new79.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch event_subscriber-2023613-84.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new9.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.

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.

Status:Needs work» Needs review
StatusFileSize
new79.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,765 pass(es), 366 fail(s), and 212 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new86.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,064 pass(es), 0 fail(s), and 149 exception(s).
[ View ]
new16.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.

Status:Needs work» Needs review
StatusFileSize
new2.07 KB
new87.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,094 pass(es).
[ View ]

.

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...

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.

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?

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.

--- /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?

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.

Status:Needs work» Needs review
StatusFileSize
new88.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,055 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

+++ 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

StatusFileSize
new1.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.

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.

Status:Needs work» Needs review
StatusFileSize
new87.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,610 pass(es), 22 fail(s), and 0 exception(s).
[ View ]

.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new88.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch event_subscriber-2023613-105.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.25 KB

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new86.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 56,369 pass(es), 392 fail(s), and 13,619 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new101.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,527 pass(es).
[ View ]

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.

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.

StatusFileSize
new101.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,549 pass(es).
[ View ]

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