Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[EventDispatcher] Addevents attribute of thekernel.event_listener tag#61222
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
[EventDispatcher] Addevents attribute of thekernel.event_listener tag#61222
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6513db5 tof801995Comparef801995 to61e8043Compare| "php":">=8.2", | ||
| "symfony/event-dispatcher-contracts":"^2.5|^3" | ||
| "symfony/event-dispatcher-contracts":"^2.5|^3", | ||
| "symfony/deprecation-contracts":"^2.5|^3" |
santysisiJul 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added this dependency to the component because I'm using thetrigger_deprecation() function.
| "symfony/deprecation-contracts":"^2.5|^3", | ||
| "symfony/error-handler":"^7.3|^8.0", | ||
| "symfony/event-dispatcher":"^6.4|^7.0|^8.0", | ||
| "symfony/event-dispatcher":"^7.4|^8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm bumping the version to 7.4 in order to use the newevents attribute instead ofevent.
Please let me know if you'd prefer a different strategy or approach. I'm happy to adjust and avoid the version bump if needed 😄
| "symfony/dependency-injection":"^6.4.11|^7.1.4|^8.0", | ||
| "symfony/deprecation-contracts":"^2.5|^3", | ||
| "symfony/event-dispatcher":"^6.4|^7.0|^8.0", | ||
| "symfony/event-dispatcher":"^7.4|^8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here
GromNaN commentedJul 24, 2025
Deprecating and renaming the "event" parameter of the tag has too much impact (as shown by the diff in tests). That will cause a lot of maintenance burden for projects that define event listeners in configuration directly. Instead, I think the event listener tag can be repeated for each listened event. |
nicolas-grekas commentedJul 24, 2025
I'm not sure that's the right approach. Can't we add multiple kernel.event_listener tags instead? That should be already supported. |
| EventDispatcher | ||
| --------------- | ||
| * Deprecate attribute`event` of the`kernel.event_listener` tag in favor of`events` attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| * Deprecate attribute`event` of the`kernel.event_listener` tag in favor of`events` attribute | |
| * Deprecate attribute`event` of the`kernel.event_listener` tag in favor of`events` attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are also two spaces between "tag" and "in".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This deprecation must be reverted.
| 7.4 | ||
| --- | ||
| * Add`events` attribute of the`kernel.event_listener` tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| * Add`events` attribute of the`kernel.event_listener` tag | |
| * Add`events` attribute of the`kernel.event_listener` tag |
OskarStark commentedJul 24, 2025
We should keep tests using also the old |
alex-dev left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking at how much changes this implementation for a minor QoL improvement bring, it is clear we should consider a less invasive approach.
I see no reason to go with supporting multiple events in the same tag. Considering a service can already have multiple kernel.event_listener targetting the same method, keeping the nesting flat and having a single event per tag ensure compatibility with everything without any breaking change.
It even matches better with the final runtime implementation: the listener method is registered multiple time in the dispatcher.
santysisi commentedJul 26, 2025
Hi everyone 👋, Thank you all for taking the time to review this PR and explain why this approach might not be ideal. I understand and agree with your points, so I’ll go ahead and close this PR. Once again, I really appreciate you all for sharing your perspectives ❤️ |
…EventListener]` (Fan2Shrek)This PR was merged into the 7.4 branch.Discussion----------[FrameworkBundle] Add support for union types on `#[AsEventListener]`| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | no| New feature? | yes| Deprecations? | no| Issues |Fix#61218| License | MITThis is an alternative implementation to support union types, related to issue#61218 and PR#61222.It duplicates `kernel.event_listener` tags.I'm not sure how to add tests for this — could you advise?Commits-------5d7b14f Add support for union types on AsEventListener
Added
eventsattribute in thekernel.event_listenertag, enabling registration of a listener for multiple events at once.Deprecated
eventattribute inkernel.event_listeneris now deprecated and will be removed in Symfony 8.0.eventattribute will trigger a deprecation warning.eventandeventswill throw anInvalidArgumentExceptionto avoid configuration conflicts.