Support #[AsEventListener] attribute on classes and methods

I took a look at this issue and gave an attempt to try to work with what we have, by supplementing ContainerAwareEventDispatcher with changes, and keeping our existing tag. But I think its going to be too much of a hassle, especially considering we also need to add a compiler pass to reflect on tagged services.

So I agree that It might just be more straightforward to switch from \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher to \Symfony\Component\EventDispatcher\EventDispatcher and utilize Symfony's\Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass here.

Since the associated MR came to the same conclusion, that swapping to Symfonys EventDispatcher is necessary, I think we should postpone this issue on our main EventDispatcher issue @ #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher

It'd be valuable to also rely on #3422359: Directory based automatic service creation to do automatic service creation for us.

#2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher was merged. Changing status per the most recent comments. Please update to a different status as needed.

Rebased MR 4438. One thing that should be noted: using the AsEventListener attribute without the event property only works if the dispatched event name matches the event object FQCN, or if an alias mapping between the event name and the FQCN is set in the event_dispatcher.event_aliases container parameter. These examples (from Symfony docs) only work if the the dispatched event name is CustomEvent::class, or if the alias is set:

namespace App\EventListener;

use Symfony\Component\EventDispatcher\Attribute\AsEventListener;

#[AsEventListener]
final class MyListener
{
    public function __invoke(CustomEvent $event): void
    {
        // ...
    }
}

namespace App\EventListener;

use Symfony\Component\EventDispatcher\Attribute\AsEventListener;

final class MyMultiListener
{
    #[AsEventListener]
    public function onCustomEvent(CustomEvent $event): void
    {
        // ...
    }
}

For convenience, I've add a commit to the MR to register all the Symfony kernel event aliases. We can probably add follow up tasks to convert core event subscribers to use the AsEventListener attribute, as well as add aliases for other core events.

Added a CR: Event listeners can be defined with the #[AsEventListener] attribute.

Is there least 1 listener that can be converted to show this change works?

Is there least 1 listener that can be converted to show this change works?

I mentioned converting core subscribers in a follow up, but I had a thought of whether there are benefits to doing it? I think having this feature is nice for DX going forward, but I don't know if there's an advantage to convert existing core code. I'll look into finding one to convert regardless, and the decision can be made from there.

OK, converted MaintenanceModeSubscriber. One thing I ran into is that AutowireTest was exempting MaintenanceModeSubscriber from needing a service alias declared, because it was one of many services implementing EventSubscriberInterface. However, with EventSubscriberInterface removed, needed to add an alias for MaintenanceModeSubscriber. I'm not sure that adding aliases for event subscriber services is worth it, but gonna push this forward for review.

Awesome to see the replacement worked though!

I reviewed the code and the draft CR. LGTM. Nice work.

Actually, I am wondering why we have both:

1. A service is declared in core.services.yml for maintenance_mode_subscriber
2. Various #[EventListener] appear on various methods on the same file. Wouldn't the nicer usage be to omit the yml declared service and rely on Attribute based creation of Listeners? The MR suggests that all we are saving is the getSubscribedEvents() method. I had higher hopes than that :). Maybe we need more plumbing elsewhere in core for what I describe.

The MR suggests that all we are saving is the getSubscribedEvents() method.

Pretty much, yes.

Automatic discovery of services without entry in a services.yml needs something like #3422359: Directory based automatic service creation. There's no discovery of services in this MR, and it's probably out of scope for this issue.

Since there are multiple handlers in MaintenanceModeSubscriber, either each method needs to have an attribute as done in the MR, or there needs to be multiple AsEventListener attributes on the class, each including the method property.

Thanks. Once we have #3422359: Directory based automatic service creation, we could add the RegisterListenersPass compiler pass and then get #[AsListener] classes registered as services and listeners. I'm not sure the intermediate step in this MR provides much value. If others think so, feel free to mark this RTBC.

The RegisterListenersPass already occurs within Drupal's Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass

That pass doesn't handle the #[AsEventListener] attribute, though. That is what this MR introduces, and the code to do so is copied from here: https://github.com/symfony/symfony/blob/9d89e05279631430f9037844fc4cc2ea...

So even if or when #3422359: Directory based automatic service creation gets in, the work here still needs to be done to support #[AsEventListener].

Also see: https://stackoverflow.com/a/78338989

Yes, I think this is still useful as it reduces boilerplate and makes the definition of listeners simpler. Going to set back to RTBC.

   maintenance_mode_subscriber:
     class: Drupal\Core\EventSubscriber\MaintenanceModeSubscriber
     arguments: ['@maintenance_mode', '@config.factory', '@string_translation', '@url_generator', '@current_user', '@bare_html_page_renderer', '@messenger', '@event_dispatcher']
+  Drupal\Core\EventSubscriber\MaintenanceModeSubscriber: '@maintenance_mode_subscriber'

While this is annoying I think in the long run event subscribers should just be named after their class, ie.

   Drupal\Core\EventSubscriber\MaintenanceModeSubscriber:
     arguments: ['@maintenance_mode', '@config.factory', '@string_translation', '@url_generator', '@current_user', '@bare_html_page_renderer', '@messenger', '@event_dispatcher']

Service names come with a BC promise (although I'm not sure how useful that is for event subscribers) so to make this switch we will have to alias the old name and deprecate it, which is out of scope of this issue, but worth considering if we decide to swap all event subscriber services to use this attribute in a followup.

If we also use autowiring then this just becomes:

   Drupal\Core\EventSubscriber\MaintenanceModeSubscriber: ~

Also I assume if we do #3422359: Directory based automatic service creation this will have to happen anyway, because the definition will be gone entirely from services.yml.

Alternatively we do this in AutowireTest:

      // Ignore proxy classes for autowiring purposes.
      if (str_contains($class, '\\ProxyClass\\')) {
        continue;
      }

We could also ignore classes with EventSubscriber in the path in a similar way? Then we can deal with everything in #22 separately from applying this pattern to all event subscribers.

Added commit per #23 to ignore classes with '\\EventSubscriber\\' in the path.

Appears to have MR conflicts. Surprised the bot didn't catch it

Rebased.

Sweet thanks for the quick turn around. Since was previously RTBC and previous feedback appears to be addressed restoring that.

It occurs to me that since #3442009: OOP hooks using attributes and event dispatcher introduced a container compiler pass that essentially iterates through module files to find classes or methods with the Drupal\Core\Hook\Attribute\Hook attribute and turn those into event listener services, if we:

1. Made the Hook attribute a subclass of the Symfony\Component\EventDispatcher\Attribute\AsEventListener attribute class
2. Changed the compiler pass to look for the AsEventListener attribute,

we could have event listener services generally discovered in the same pass. And those event listeners wouldn't need to be declared in services.yml files.

This wouldn't work for core event subscribers outside of modules, but it might be worth looking into?

Created a new draft MR 10119 for a PoC of the approach I mentioned in #28. It leverages a lot of what HookCollectorPass is already doing to also find module class with the AsEventListener attribute and registers them as event listener services. No entry in the module .services.yml is required. Note that discovery is limited to the EventSubscriber directory in a module. Also, these services are autowired, so any arguments in class constructors need to be able to be autowired or have the Autowire attribute applied to them.

Putting back to NR for feedback on the PoC MR. I can also move this issue back to RTBC if this new work is better done in a followup.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Not sure the approach to combine event subscriber discovery with hook discovery makes that much sense anymore, since they've diverged, so closing MR 10119.

Haven't revisited MR 4438 in a while, and I'm not sure there's much momentum to do this anymore? But if there is, that might be a good starting point to revisit.

I'm currently working on a new MR that uses a different compiler pass for the event listeners (see AsEventListener module). Probably opening a can of worms here (especially because of naming) but I thing it's worth it :)
Hooks has changed drastically since MR 10119 has been built (hint: OrderInterface) so I couldn't find a solution right now to translate the way hooks are ordered now into event listener priority.

We explicitly don't have priority on hooks beyond module weight but there is an issue iirc to add it.

Fundamentally I think this would be a bad idea at the moment for a couple reasons.

This attribute is too close to hooks and will likely be very confusing when to use which.

Also events can't scale properly to how Drupal sites are built, that's why we had to decouple hooks and events again.

@stborchert I closed MR 10119 because aligning hooks and events doesn't make sense anymore.

To support AsEventListener independently, the approach in that contrib module you mentioned is a lot more complex than MR 4438, which was RTBC in #27 before I looked into MR 10119. You might want to investigate whether the complexity is needed.

As mentioned, I haven't looked at 4438 in a while, so there might be some performance issues with it, as well as the general idea @nicxvan noted about events not scaling (adding a lot of event listeners makes the container size grow significantly).

> I closed MR 10119 because aligning hooks and events doesn't make sense anymore.

Jup, that's what I thought, too.

> To support AsEventListener independently, the approach in that contrib module you mentioned is a lot more complex than MR 4438

A little :)
In the module I tried to follow the implementation from Symfony so you are able to do the same fancy things with the attribute (method name guessing from event name/class, __invoke, ...) and it's turned out to be a bit more extensive.
If I understand MR 4438 correct it only allows the attribute on methods and requires to set the event.

MR 4438 is basically directly lifted from Symfony: https://github.com/symfony/symfony/blame/2fc5fbe185e77ca8425334e3fd841da...

Requesting review for MR !15467.

I see ... but it's missing the part where the listeners are registered by adding the RegisterListenerPass: https://github.com/symfony/symfony/blame/2fc5fbe185e77ca8425334e3fd841da...

Anyway, it would be really great if we could get this in. I don't care which path we take. It's the result that counts :)

Some notes about MR !15467:

I use EventListener namespace instead of EventSubscriber because with #[AsEventListener] we do not need to create classes implementing EventSubscriberInterface and define a list of events they're interested in, along with corresponding handling methods.
Event subscribers handle multiple events and react to them, while listeners simply listen to the event they are bound to (yes, you are able to use one method for multiple events, but ...).

I see ... but it's missing the part where the listeners are registered by adding the RegisterListenerPass

That's already in core: https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

Oh, wow ... didn't see that.
Ok, I have to check how this behaves when trying to use method name guessing and the other features ...

Needs work for 38 and 45.

I really think we really need a strong case for how this won't be confused with hooks and won't become a performance issue.

IMO, this should be scoped to just making it possible for any registered service (i.e. defined in a .services.yml or ServiceProvider) to be an event listener using the attribute. (This means they would not need to implement EventSubscriberInterface or the getSubscribedEvents() method.)

It was worth seeing if we could get event subscriber discovery almost for free when hooks were event subscribers. Now, remaining autodiscovery of services probably can be left to #3422359: Directory based automatic service creation.

I did a few tests with MR !4438 (applied the changes manually on main) and it looked great. All usage variants of #[AsEventListener] are working.

The only change I would like to make is naming: the services will no longer be event subscribers, but rather event listeners.

I am closing MR 15467 because 4438 has everything we need.

I've created a new MR based on MR !4438 and main.

* added simple tests for #[AsEventListener] usage
* updated AutowireTest to also exclude classes in EventListener namespace

I really would like to re-label current event subscribers to event listeners, but this would be a massive change with probably no backwards compatibility (maybe not? https://www.drupal.org/node/3509577).

Example core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php -> core/lib/Drupal/Core/EventListener/MaintenanceModeListener.php

38 and 46 really need to be answered please.

What does this give is that hooks don't already?
How will this not be confused with the hook attribute?
How will we prevent container bloat and performance issues?

I can only speak from my perspective and with my limited knowledge. If I'm wrong about anything I've written here, I apologize. There are people with much more expertise in these areas who can answer your questions much better.

From my understanding, hooks are a special kind of event subscribers/listeners. In theory (maybe Drupal 13 or 14 and maybe I'm completely wrong at this point) we could remove support for procedural hooks and #[Hook] completely and use event listeners for hooks only.

> What does this give is that hooks don't already?

The question should be the other way round ... What does a hook do we cannot achieve using events?

> How will this not be confused with the hook attribute?

I don't see why anyone should confuse a hook with an event listener. Event listeners are a basic feature of Symfony used by Drupal in various places (see MaintenanceModeSubscriber as an example). Hooks are - well - hooks, a unique feature of Drupal ...

No need to apologize!

hooks are a special kind of event subscribers/listeners

Sort of, they are kind of inverted from event subscribers.

we could remove support for procedural hooks and #[Hook] completely and use event listeners for hooks only.

Procedural hooks will likely be deprecated in 13, The hook attribute I never see being removed outright.
The big issue is symfony doesn't handle listeners at the scale the Drupal needs them. The new Hook attribute in 11.1 made them event listeners under the hood. It was so bad performance wise we stripped that out by 11.3 https://www.drupal.org/node/3550627

The one thing AsEventListener does that hooks don't do yet is it can be on an arbitrary service. I think we just work on the issue to add hooks to arbitrary services though: #3481903: Support hooks (Hook attribute) in any registered service

The question should be the other way round ... What does a hook do we cannot achieve using events?

They scale far better than events do is one huge thing. Relative ordering is another.

> Procedural hooks will likely be deprecated in 13

Wohoo :)

> The new Hook attribute in 11.1 made them event listeners under the hood. It was so bad performance wise we stripped that out by 11.3 https://www.drupal.org/node/3550627

Wow, wasn't aware of that. Ok, this crashes my theory about removing the attribute :D.

> They scale far better than events do is one huge thing. Relative ordering is another.

Got it.
What I don't get right now: using the AsEventListener attribute does not add any new services to the container. Instead of using a service implementing EventSubscriberInterface, it simply allows developers to register the event listener (and the corresponding service) simply by using the attribute. Is there any performance decrease by using the attribute instead of implementing a class with EventSubscriberInterface?

Doesn't it register them as services for autowiring?

> Doesn't it register them as services for autowiring?

Yes, but the service is registered either way, if you use a event subscriber or a event listener using the attribute. At least I think that there are no services created in addition to the services we've already got.

I don't know if we should actually do this but an idea I've had for a while is that we should scan every class in every module (maybe excluding Plugin, Entity and some other directories), and register it as a private service. The final container build excludes private services unless they are actually injected into something else (such as the event dispatcher). Then you can just write classes in almost any namespace, and only need to use an attribute like this to automatically get your service registered in the correct way. Given we're already scanning src/Controller and src/Hook, and soon src/Form, perhaps scanning and registering everything is the most efficient way.

> [...] we should scan every class in every module (maybe excluding Plugin, Entity and some other directories), and register it as a private service.

You mean something like this and adding <?php $definition->setPublic(FALSE); to process()?

I think so, but I also vaguely recall something being broken with that bit of functionality (auto dropping private services with no usages) with Drupal's container overrides.

Also I think that is one of the directions explored or being explored here #3481903: Support hooks (Hook attribute) in any registered service

I don't know if we should actually do this but an idea I've had for a while is that we should scan every class in every module (maybe excluding Plugin, Entity and some other directories), and register it as a private service.

I have some minor DX concerns about this, because sometimes I'll have uncommitted POC/WIP PHP classes sitting around in the src/ directory, and discovery will break unless I fix them up enough so that there aren't reflection errors. This is a minor annoyance because I can change the extension to not be .php or move them out of src or something. But I think it's something to consider that if we do that, there'd be no such thing as "dormant" code anymore, since everything will get processed by PHP via reflection.

But I think that's a scope topic for #3422359: Directory based automatic service creation.

I think so, but I also vaguely recall something being broken with that bit of functionality (auto dropping private services with no usages) with Drupal's container overrides.

Someone reminded me that it's actually that private services don't work in many cases like plugins and controllers.

Also @longwave the work you are doing on the event subscriber private issue will likely break once #3538212: Reintroduce the container aware dispatcher for a 10% speedup lands which I think we should prioritize for 11.4 if we can.

Yes private services don't work in ContainerInjectionInterface things (including AutowireTrait), by design. The idea would be that you explicitly declare your public services in services.yml as now, but that we can also look for other things and register them so they can be privately injected into things that do support that (such as the event dispatcher) without needing to declare them twice.

The idea would be that you explicitly declare your public services in services.yml as now

As we found out in #3295751: Autowire core services that do not require explicit configuration, we have to declare the services that can't be autowired as well.

I think this issue is worth doing without automatic discovery. Per #21, it reduces boilerplate (don't need classes to implement EventSubscriberInterface or getSubscribedEvents), and it means that an event listener can be easily implemented in any service.

While for a lot of things Drupal, hooks will stay the best way to do things, there are still many aspects that rely on event listeners/subscribers (the routing system, for example), and it'd be good to make DX a bit easier.

I think we can move this out of postponed.