NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Event listener from attribute should not have a default priority#43757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

aschempp wants to merge1 commit intosymfony:5.3fromaschempp:bugfix/attribute-priority

Closed

Event listener from attribute should not have a default priority#43757

aschempp wants to merge1 commit intosymfony:5.3fromaschempp:bugfix/attribute-priority

Conversation

Copy link

Contributor

aschempp commentedOct 26, 2021

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	~
License	MIT
Doc PR	~

There is a difference between a service not having a priority and a priority of being0. If there is no priority (or it isnull), the event listener/service can provide a method to return the priority. This would be disabled by a default value of0.

seehttps://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php#L68-L72

Event listener from attribute should not have a default priority

0fb5a00

carsonbot added Status: Needs Review Bug labels

Oct 26, 2021

carsonbot added this to the5.3 milestone

Oct 26, 2021

Copy link

Member

nicolas-grekas commentedOct 26, 2021

Nice catch. Can you please add a test case so that we're reminded about that behavior?

Copy link

ContributorAuthor

aschempp commentedOct 28, 2021

Reviewing this feature, I realized that theRegisterListenersPass which registers event listener does not actually implementPriorityTaggedServiceTrait!https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php#L94

Now this

obviously makes my PR obsolete
begs the question whether the listener should be changed to use the trait? 😅

Copy link

Member

derrabus commentedOct 28, 2021

I don't quite get why I would configure an event listener by attribute but set the priority via method call. If I use the attribute, why don't I use the attribute for everything?

Copy link

Member

nicolas-grekas commentedOct 28, 2021

I realized that the RegisterListenersPass which registers event listener does not actually implement PriorityTaggedServiceTrait!

Oh indeed, priority is managed by the EventDispatcher at runtime.

nicolas-grekas closed this

Oct 28, 2021

Copy link

ContributorAuthor

aschempp commentedOct 28, 2021

I don't quite get why I would configure an event listener by attribute but set the priority via method call. If I use the attribute, why don't I use the attribute for everything?

Yeah thats a good question, and maybe why this has never been added to event listeners. As far as I understand, this was added in#33628 to support priority for services that were tagged through (interface) autoconfiguration. I guess this has never become relevant for event listeners because one could use theEventSubscriber pattern in that case (which is not available for e.g. security voters).