Skip to content

Removed unused $event param of onKernelFinishRequest from RouterListener #47472

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
Merged
merged 1 commit into from
Sep 13, 2022
Merged

Removed unused $event param of onKernelFinishRequest from RouterListener #47472

merged 1 commit into from
Sep 13, 2022

Conversation

githubfromgui
Copy link
Contributor

@githubfromgui githubfromgui commented Sep 2, 2022 •
edited by nicolas-grekas
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
License MIT

The $event parameter in the onKernelFinishRequest for the RouterListener class seems to not be used. Other listeners such as LocaleAwareListener, Firewall and FirewallListener all takes advantage of this $event parameter.

Symfony\Component\HttpKernel\EventListener\LocaleAwareListener:
image

Symfony\Component\Security\Http\Firewall:
image

Symfony\Bundle\SecurityBundle\EventListener\FirewallListener:
image

Plus, I could not find any Interface that ensures any contract over this method to all these classes above.

@carsonbot carsonbot added this to the 4.4 milestone Sep 2, 2022
@ghost
Copy link

ghost commented Sep 3, 2022

Syfony 4.4 receives only bug fixes and your change ain't one. What's more, removing a required parameter on public method in a non-final class is a BC break.

@artyuum
Copy link
Contributor

artyuum commented Sep 3, 2022

@javaDeveloperKid The PR removes an unused method argument and when I look into that file (RouterListener), I see that it implements the EventSubscriberInterface and this interface does not require the modified method to have this argument.

So how is that a BR break?

(Sorry for potentially polluting this PR, I'm just trying to understand things about Symfony better for my future contributions.)

@chalasr
Copy link
Member

chalasr commented Sep 4, 2022

If a class extends it, that child class would be broken because of the method signature mismatch. See https://symfony.com/doc/current/contributing/code/bc.html.
But this class is marked as @final since Symfony 4.3 so it is considered as really final since v5, thus this change is fine from a BC pov as of v5.
Finally, as this change does not fix a bug, it should target 6.2.

@chalasr chalasr modified the milestones: 4.4, 6.2 Sep 4, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to 6.2 September 12, 2022 16:21
@nicolas-grekas nicolas-grekas force-pushed the event-param-not-used-router-listener branch from ae1f568 to 3ff9706 Compare September 12, 2022 16:21
nicolas-grekas
nicolas-grekas approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I rebased for 6.2)

@fabpot
Copy link
Member

fabpot commented Sep 13, 2022

Thank you @githubfromgui.

@fabpot fabpot merged commit f7556e7 into symfony:6.2 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
6 participants
@githubfromgui @artyuum @chalasr @fabpot @nicolas-grekas @carsonbot