Skip to content

[Security] Deprecate callable firewall listeners #60614

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from
Open

[Security] Deprecate callable firewall listeners #60614

wants to merge 1 commit into from
+132 −27

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jun 1, 2025 •
edited
Loading

Q A
Branch? 7.4
Bug fix? no
New feature? no
Deprecations? yes
Issues N/A
License MIT

After spending some time in the Security component it occurred to me callable firewall listeners are obsolete now that we got the FirewallListenerInterface. Their deprecation has already been suggested (like in #34627 (comment) or #38751 (review)), so this PR does it.

@MatTheCat MatTheCat requested a review from chalasr as a code owner June 1, 2025 14:01
@carsonbot carsonbot added this to the 7.4 milestone Jun 1, 2025
Spomky
Spomky reviewed Jun 1, 2025
View reviewed changes
Copy link
Contributor

@Spomky Spomky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one remark about the changlog.

chalasr
chalasr requested changes Jun 1, 2025
View reviewed changes
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch 2 times, most recently from f2b8c5e to 311c89f Compare June 5, 2025 08:40
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch from 311c89f to 253ed34 Compare June 11, 2025 08:34
chalasr
chalasr reviewed Jun 11, 2025
View reviewed changes
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch 3 times, most recently from 225e787 to fd98438 Compare June 12, 2025 21:11
@MatTheCat
Copy link
Contributor Author

Just noticed that the LazyFirewallContext can also run the firewall listeners so I added the deprecation in there. I also moved the changelog entry to the security-http’s instead of the security-bundle’s.

@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch from fd98438 to 2b96194 Compare June 12, 2025 21:24
@MatTheCat MatTheCat mentioned this pull request Jun 13, 2025
Merged
@xabbuh xabbuh reopened this Jun 15, 2025
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch 4 times, most recently from e900519 to c4d7fce Compare June 15, 2025 10:54
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch 3 times, most recently from 8cf25fb to 7dd75b6 Compare June 15, 2025 11:51
@MatTheCat MatTheCat force-pushed the deprecate-callable-firewall-listener branch from 7dd75b6 to 2b4d065 Compare June 15, 2025 11:59
@MatTheCat
Copy link
Contributor Author

With this PR AbstractListener::__invoke shouldn’t be called anymore, should I deprecate it as well? That would only leave its getPriority method so maybe the AbstractListener can be deprecated entirely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Spomky Spomky Spomky approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Deprecation Security Status: Reviewed
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
5 participants
@MatTheCat @Spomky @chalasr @xabbuh @carsonbot