Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Profiler respect stateless attribute#50218
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
MatTheCat commentedMay 3, 2023
#50198 was a new feature so it is only available on the 6.3 branch 😅 I looked for a way to test this and it seems we have to update |
stof 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.
This should be done in 6.3 IMO as it relies on the previous PR that is merged in 6.3
src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
a763930 to7820776Comparefabpot commentedMay 5, 2023
Thank you@alamirault. |
| $session =$request->hasSession() ?$request->getSession() :null; | ||
| $session =null; | ||
| if ($request->attributes->getBoolean('_stateless') &&$request->hasSession()) { |
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 know I am late, but I thought posting it here was less interruptive then creating a new issue or discussion for this question:
The PR touches 3 placed where it gets the session and adds checks for the_stateless request attribute.
Two checks check for!stateless && hasSession while one of them (insearchBarAction) checks forstateless && hasSession (without the!).
I found this while debugging for what startsSession was used while the request was declared stateless. when calling the profiler. As far as I can tell, the only place using the session there is the this spot.
This line looks incorret to me, I think its missing the! in front. But since I'm probably wrong (since this is like this for a year now, with no one noticing) I'd like to learn why I am wrong!
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.
Just tested and you’re not wrong. Maybe you really are the first to notice 😅
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.
If I understand the implications of this correct, then that means that for users that havestateless: false, restoring the search bar state from session should not work, as it would not use the session in those cases. It would only use the session ifstateless: true (e.g. when there should be no session). Correct?
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.
for users that have
stateless: false, restoring the search bar state from session should not work
Yes, that also includes users who did not set_stateless. Are you up for a PR? Looks like you already know the fix!
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 can give it a try:#57679
Hope thats correct so far.
Uh oh!
There was an error while loading.Please reload this page.
Now that#50198 forward
_statelessattribute in fragments, this PR check this attribute and make profiler stateless when is required.I don't know which tests I have to add because session is not checked. (I checked on reproducer and it's ok)