-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Remove sorting of security listeners at runtime from Firewall #43548
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for the PR. symfony/src/Symfony/Component/Security/Http/FirewallMapInterface.php Lines 36 to 38 in 732acf5
With the proposed patch, the logout listener returned from FirewallMap is not used anymore which looks like a BC break. |
Excellent 👍 I'll re-target the PR to 6.0. How should we move ahead with the deprecation? (How do you deprecate a value in a returned array? 🤔) |
29a751c
to
3ca2103
Compare
Triggering a notice from |
I can take care for a PR to add that deprecation. Could someone update the milestone on this PR to 6.0 (I can't). Thanks! 🙇 |
PR for deprecation is open. Once that's done, I'll rebase this one and include a clean-up of deprecations. |
Let me close here as I think that while cleanups are welcome, this one is hard (with BC implications) for little practical benefit. 3 years passed also :) |
This is follow-up of introducing the ability to sort security listeners, which was introduced in #37337. Part of the discussion at that time was whether we'd still need to hardcoded sorting algorithm in the
Firewall
class, which was previously needed to sort in theLogoutListener
at the correct position. With the ability to sort by a priority that was actually no longer needed.I had this change temporarily implemented in 1972786, though in the discussion it was decided against and instead keep the code for backwards compatibility reasons. Otherwise we'd need to declare a conflict with
security-bundle
in thecomposer.json
and it was argued by @chalasr against making thesecurity
component aware ofsecurity-bundle
(see #37337 (comment)).Though I've recognized, things have changed in the meantime, and
security
component is now declaring conflict forsecurity-bundle
(introduced by @nicolas-grekas in 314ef9f)So I want to give this another shot and clean up the
Firewall
class, targeting potentially the 5.4 branch, definitely for the 6.0 branch.Considerations
To make the clean-up work, the
LogoutListener
service needs to be added to the list of listeners generated bySecurityExtension
. Until now, it's intentionally (?) left out. In the 5.4 branch this could be considered a breaking change. Let me know what you think, if this is in fact considered as breaking, I'm happy to change the target to 6.0.