[EventDispatcher] add a way to call a listener before or after another one (WIP)

[nikophil]

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc PR	will be done if feature gets accepted

When hooking into the event system, it's a common need to define the priority compared to another existing listener:

// should occur BEFORE "router_listener"
#[AsEventListener(event: KernelEvents::REQUEST, priority: 33)]

// OR

// should occur AFTER "router_listener"
#[AsEventListener(event: KernelEvents::REQUEST, priority: 31)]

This is not optimal: without the comment it is not clear why the listener has the given priority. Moreover the order is not enforced by code and the priority of the other listener can change.

I'd like to propose another approach, and introduce a new way to handle priorities between listeners:

// the listener will occur before "router_listener::onKernelRequest()"
#[AsEventListener(event: KernelEvents::REQUEST, before: RouterListener::class)] 

 // the listener will occur after "router_listener::onKernelRequest()"
#[AsEventListener(event: KernelEvents::REQUEST, after: RouterListener::class)]

before and after priority should follow these rules:

• it can be declared as a simple string:
  • another listener's serviceId, if it only have one method listening to the same event
  • a listener's class name if a service is declared for this class AND if it has only one method listening to the same event
• it can be declared as an array:
  • {0: 'some_service_id', 1: 'someMethodName'}
  • {0: SomeListenerClass::class, 1: 'someMethodName'} a class name is accepted if a service is declared with this class

The idea is to rely on the priority system and to guess the priority of the listener based on its "before" or "after" attributes (the priority computation would be recursive if the given before/after listener should also occur before/after another one).

Here are some rules:

• if a listener uses before/after, it must listen the same event than the listener which is referenced
• a listener cannot have a priority configured and use before/after
• a listener cannot declare at the same time "before" and "after"
• we should detect circular references

Here is a first draft PR of the implementation, it's missing a lot of stuff:

• tests
• ability to define before/after in php, xml, yaml
• doing the same thing with subscribers
• certainly some edge cases are not handled correctly here

at this point, I'd like to have early feedback about the idea and if it has some chances to be accepted. and if the given implementation is the way to go?

I think if this feature is accepted, it would be nice to commit it to all tags using priorities, and then add it to PriorityTaggedServiceTrait

thanks for your feedbacks!

[kbond]

I like this. Didn't look to deep into the code but it would fail during compile if the before/after service id changed?

Maybe before/after could be configured as a quasi callable (attributes can't accept real callables) to help with IDE click throughs and refactoring?

/** @var string|class-string|callable|null */
public string|array|null $before = null,

before: [MyListener::class, 'myMethod']

before: MyListener::class // uses __invoke

[nikophil]

it would fail during compile if the before/after service id changed?

Yes, you'll have a \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException with the following message: Given "before" "some_service::some_method" for listener "your_listener::your_method" does not listen the same event.

But maybe I should fail before, and directly say Given "before" "some_service::some_method" for listener "your_listener::your_method" does not exists.

Maybe before/after could be configured as a quasi callable (attributes can't accept real callables) to help with IDE click throughs and refactoring?

this is a bit tricky since we're working with service ids and not classes...

[kbond]

this is a bit tricky since we're working with service ids and not classes...

For internal event listeners, yes. I'm thinking about ones in your app that you probably have autowired as the class name.

[nikophil]

yes ok, I think I definitely need to be more flexible how we can name the listeners:

• "quasi callable" as you said
• maybe "first class callable": MyListener::myMethod(...) (not sure the syntax is allowed in the attributes 🤔 )
• also, if the class only have one public method, we may allow to only pass its service id
• maybe also allow to pass the classes FQCN instead of the service id? I mean: before: RequestListener::class. .'::onKernelRequest()' but I'm not sure if I really can rely on this?

[kbond]

maybe "first class callable": MyListener::myMethod(...) (not sure the syntax is allowed in the attributes thinking )

I'd prefer this but yeah, don't know if it's possible.

maybe also allow to pass the classes FQCN instead of the service id? I mean: before: RequestListener::class. .'::onKernelRequest()' but I'm not sure if I really can rely on this?

This would be nice but we'd need to throw an ambiguous exception if there are multiple service ids for this class.

[nikophil]

I'd prefer this but yeah, don't know if it's possible.

not possible: https://3v4l.org/9HYHJ#v8.2.7

This would be nice but we'd need to throw an ambiguous exception if there are multiple service ids for this class.

ok, I'm gonna do that!

[mtarld]

Nice addition!

[GromNaN]

Usually, classes have only 1 method that listen to a given event. Instead of assuming no-method is __invoke, the behavior should be to find the method of the class that have a listener for this event. Or throw an exception if there is not exactly 1.
This would solve en "pseudo-closure" issue by just setting the class name in the AsEventListener attribute.

[nikophil]

Yes, I'm perfectly fine with this, but this code was extracted from the original compiler pass,
So changing this would change the main behavior of how the listener method is resolved.

By the way, I've just noticed what I thing is a bug while playing around with this:

#[AsEventListener(event: KernelEvents::REQUEST, priority: 5)]
class MyListenerNotInvokable
{
    public function someMethod(RequestEvent $event): {}
}

It registers a listener on MyListenerNotInvokable::onKernelRequest() and then an error is dispatched at runtime: Error: Call to undefined method App\EventListener\MyListenerNotInvokable::onKernelRequest(). Whereas the correct behavior would be to resolve the listener method to the only public method in the class (or throw an exception if several public methods exist)

I think I'll fix this in another PR before finishing the work here

[GromNaN]

I understand that you've extracted the existing code, but these are 2 different things.

• the existing code finds the method to call when declaring a listener by analyzing the class (or with a fallback to a default method name). I think it's dangerous to use the public method if it's unique. There's a risk that the code will break if you have to add a new public method.
• the new code must be based on existing listeners only. It's then no longer a question of detecting public methods, but only of searching among the methods attached to a listener on the event concerned.

[nikophil]

yes I totally agree but don't you think that registering a listener on method MyListenerNotInvokable::onKernelRequest() (see example above) which does not exist is a mistake? which by the way throws a runtime exception as soon as the event is dispatched, although we could have detected the problem at compile-time. See this code which seems buggy: if the computed method name does not exist, and if __invoke does not exist, then we still keep the wrong method name.

What I have in mind is to remove the automatic fallback on __invoke here and here and replace it with a more fine-grained method resolving (would be done in another PR)

Or throw an exception if there is not exactly 1.

this would be done exclusively for listeners declaring before/after, when resolving their priority: if not enough information is provided (ie: only class/serviceId is provided and the given class contains multiple methods which listen to the same event) then we'd throw an exception because before/after declaration is ambiguous

maybe these are two different things, but the latter will benefit from the former because I've reused all the code in RegisterListenersPass::process() to iterate on listeners definitions

WDYT?

[GromNaN]

yes I totally agree but don't you think that registering a listener on method MyListenerNotInvokable::onKernelRequest() (see example above) which does not exist is a mistake? which by the way throws a runtime exception as soon as the event is dispatched, although we could have detected the problem at compile-time. See this code which seems buggy: if the computed method name does not exist, and if __invoke does not exist, then we still keep the wrong method name.

This is indeed a bug. Would you mind opening a PR or an issue?

maybe these are two different things, but the latter will benefit from the former because I've reused all the code in RegisterListenersPass::process() to iterate on listeners definitions

I'm still not convinced that it can be the same mechanism for both. For the before/after feature, we know the actual listeners of the given event, which restrict the list of possible method names. I would not modify in this PR the way the listener method are guessed when they are register.

[nikophil]

This is indeed a bug. Would you mind opening a PR or an issue?

yep I'll open a PR soon, I'll ping you there

I'm still not convinced that it can be the same mechanism for both. For the before/after feature, we know the actual listeners of the given event, which restrict the list of possible method names. I would not modify in this PR the way the listener method are guessed when they are register.

I've almost finished the work on the compiler pass with the definition you suggested:

class AsEventListener
{
    public function __construct(
        public ?string $event = null,
        public ?string $method = null,
        public int $priority = 0,
        public ?string $dispatcher = null,
        /** @var string|array{class?: class-string, service?: string, method?: string} */
        public string|array|null $before = null,
        /** @var string|array{class?: class-string, service?: string, method?: string} */
        public string|array|null $after = null,
    ) {
    }
}

I'll push the code soon, the discussion will be easier then :)

[nikophil]

just a word about this:

I would not modify in this PR the way the listener method are guessed when they are register.

even if I've refactored the compiler pass, I've touched nothing about the logic and the way listener methods are guessed.
All I do is iterating on the listener definitions, and recomputing the priority of the listeners which have before/after properties

[alexislefebvre]

If 2 listeners subscribe to the same “after X”, which one will be called first? Could we define the priority of each listener?

[kbond]

If 2 listeners subscribe to the same “after X”, which one will be called first? Could we define the priority of each listener?

I would say the order shouldn't matter here. If it does, you'd need to set one to be called after the other.

[alexislefebvre]

If 2 listeners subscribe to the same “after X”, which one will be called first? Could we define the priority of each listener?

I would say the order shouldn't matter here. If it does, you'd need to set one to be called after the other.

Like chaining them? Is it possible with the changes from this PR? Or is it a special case that require to define priorities and is out of scope for this PR?

[nikophil]

Hi @alexislefebvre

yes there is no problem with the current implementation to do something like

#[AsEventListener(event: SomeEvent::class, priority: 10)]
class SomeListener{}

// will have priority 11
#[AsEventListener(event: SomeEvent::class, before: SomeListener::class)]
class SomeOtherListener{}

// will have priority 12
#[AsEventListener(event: SomeEvent::class, before: SomeOtherListener::class)]
class AnotherListener{}

// will have priority 10 (notice "after")
#[AsEventListener(event: SomeEvent::class, after: SomeOtherListener::class)]
class YetAnotherListener{}

[nikophil]

not class-string|array{class?: class-string, service?: string, method?: string} as suggested because we accept service id as well as class names

[nikophil]

I've updated the PR.

before/after can be defined as: a class name, a service name or a pair of [class, method] or [service, method].
If the service or the class is defined alone, then we can resolve it if and only if it defines one listener for the given event.

we have to create 2 "maps" of the listener definitions:

• one which indexes all the listeners/subscribers definitions
• one based on the classes FQCN where we can easily find listener/subscriber definitions based on a class name

Thus, we need to loop over all listeners/subscribers (indistinctly: a listener can be defined before/after a subscriber and the other way round) before actually registering them in the event dispatcher. I needed to create a normalized representation of all listeners/subscribers definitions, with their method and event resolved, so I extracted the logic from RegisterListenersPass::process() which now leverage this new representation. I actually hesitated to introduce a value object for ListenerDefinition, but for now I've used a psalm pseudo-type.

Because before/after logic was getting bigger and bigger, I decided to split the compiler pass, hence the trait ListenersPassTrait because we need to share the logic of the listeners definitions extraction.

I tried to limit the "array hell" effect, but it's kinda hard since we need a hash map which is indexed by event, service id and method.

I hope that the implementation is OK and that I did not over-engineered all the things 😅
I'd love to have your feedbacks @GromNaN @kbond @chalasr

Once we've found a satisfying solution, I'll provide the missing parts of the PR (tests, xml/yaml/php configuration, implement before/after for subscribers)

[TemirkhanN]

Looks good if it is for the symfony internal usage.
Arranging the priority on the application level might bring some complications since one listener can't know how another listener is implemented and thus unable to guarantee that there is no async process initiated underneath (which basically nullifies the idea behind the priority).

[nicolas-grekas]

I'm seeing a lot of activity here but I'm also seeing a lot code, and that scares me a bit.
Priorities are so much easier to manage from a maintainer pov.

I think we can simplify a lot the logic by turning before/after attributes into pure verification steps: priorities work as usual and they're still the only way to specify the order, but then we throw an exception if the before/after attributes don't work.

WDYT?

[nikophil]

Hi @nicolas-grekas

I understand your concerns about the big amount of code in the PR.

if we keep before/after definition like we imagined (ie: could be a class, a service id or an array [class|serviceId, method]) I'm not really sure we could save that much code: we'll still need to compute both $listenersMap and $listenerClassesMap and we'll still need the big method RegisterListenersPass::normalizeBeforeAfter()

Priorities are so much easier to manage from a maintainer pov.

Not sure to understand what you mean here 🤔

[nicolas-grekas]

I mean the current code is fine in terms of complexity.

if we keep before/after definition like we imagined

I think we should take the simplest option: we handle only [serviceId, method] and we check if the required order is respected. If not, we throw. An array_search can do it (to get the resolved order after priorities have been used).

[nicolas-grekas]

the current code => the one in Symfony (not this PR :) )

[nikophil]

Hi,

I've tried a rewrite of the PR with an object oriented approach. I think the code is really cleaner and simpler, any thought before I change before/after attributes into pure verification steps?

[fabpot]

I agree with @nicolas-grekas that it adds a lot of complexity to the code for a problem that was never reported before.
Priorities for built-in listeners have not changed for years, and using numbers works well enough.
Maybe we can try to implement Nicolas' suggestion?

[diimpp]

for a problem that was never reported before.

For me it's mainly useful if vendor code like sylius/sylius defines multiple listeners with non-zero priority and it's important to execute my code before or after. As result App listener definitions becomes ridden with "magic numbers" like priority: 47, which can be solved with "before" or "after".

Though it's pretty much edge case.

[fabpot]

Closing as I still don't think this is something we need to support like implemented here. Thank you for proposing.