Skip to content

[HttpKernel] Fix CacheAttributeListener priority #48524

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
Dec 13, 2022
Merged

[HttpKernel] Fix CacheAttributeListener priority #48524

merged 1 commit into from
Dec 13, 2022

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Dec 6, 2022 •
edited
Loading

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

Currently the CacheAttributeListener & the IsGrantedAttributeListener have the same priority:

Registered Listeners for "kernel.controller_arguments" Event
============================================================

 ------- --------------------------------------------------------------------------------------------------------- ---------- 
  Order   Callable                                                                                                  Priority  
 ------- --------------------------------------------------------------------------------------------------------- ---------- 
  #1      Symfony\Component\HttpKernel\EventListener\CacheAttributeListener::onKernelControllerArguments()          10        
  #2      Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener::onKernelControllerArguments()   10        
  #3      Symfony\Component\HttpKernel\EventListener\ErrorListener::onControllerArguments()                         0         
 ------- --------------------------------------------------------------------------------------------------------- ----------

Since the CacheAttributeListener is alphabetically first, it's first to get triggered. This can cause an unauthenticated user to receive a 304 Not modified instead of a 302 Redirect, resulting in the user seeing some stale content from when they were authenticated instead of getting redirected to the login page.

This PR changes the priority of the CacheAttributeListener to be lower than that of the IsGrantedAttributeListener.

@carsonbot carsonbot added this to the 6.2 milestone Dec 6, 2022
@HypeMC HypeMC changed the title [HttpKernel] Fix CacheAttributeListener priority [HttpKernel] Fix CacheAttributeListener priority Dec 6, 2022
@nicolas-grekas
Copy link
Member

Did you consider setting IsGrantedAttributeListener's priority to 20 instead?

@HypeMC HypeMC force-pushed the cacheattributelistener-priority-fix branch from 282eef4 to 90eb89f Compare December 13, 2022 15:05
@HypeMC
Copy link
Contributor Author

HypeMC commented Dec 13, 2022

@nicolas-grekas Actually, come to think of it, I'd prefer that option, done.

@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 0f5a556 into symfony:6.2 Dec 13, 2022
@HypeMC HypeMC deleted the cacheattributelistener-priority-fix branch December 13, 2022 15:54
@fabpot fabpot mentioned this pull request Dec 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
3 participants
@HypeMC @nicolas-grekas @carsonbot