Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Add session cookie handling in cli context#41390
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
[HttpKernel] Add session cookie handling in cli context#41390
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5d51c41 tobe9fc75Comparesrc/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
carsonbot commentedMay 24, 2021
Hey! I think@Simperfit has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
f5a49e5 to4cc89f7CompareUh oh!
There was an error while loading.Please reload this page.
aac0ccb toab83e8aCompareab83e8a tofbfc9cfComparefbfc9cf to647a4e0Compare
Nyholm 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.
Thank you.
I like this. Is it ready for a review (ie, not draft)?
If so, I would be happy if@jderusse gave his opinion on this solution.
Uh oh!
There was an error while loading.Please reload this page.
alexander-schranz commentedJul 8, 2021
@Nyholm Yes the PR itself is ready for review, I want still provide some tests for the Listener logic. |
Nyholm 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.
I hope you don't mind that I pusheda small commit to fix the tests.
| $this->registerSessionConfiguration($config['session'],$container,$loader); | ||
| if (!empty($config['test'])) { | ||
| $container->getDefinition('test.session.listener')->setArgument(1,'%session.storage.options%'); | ||
| $container->getDefinition('test.session.listener')->setArgument(2,'%session.storage.options%'); |
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 is because we switched fromTestSessionListener toSessionListener
| $sessionCookiePath =$this->sessionOptions['cookie_path'] ??'/'; | ||
| $sessionCookieDomain =$this->sessionOptions['cookie_domain'] ??null; | ||
| $sessionCookieSecure =$this->sessionOptions['cookie_secure'] ??null; | ||
| $sessionCookieSecure =$this->sessionOptions['cookie_secure'] ??false; |
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.
src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
8bf60c5 tob3e4f66Comparefabpot commentedSep 6, 2021
Thank you@alexander-schranz. |
Nyholm commentedSep 6, 2021
Awesome. |
alexander-schranz commentedSep 6, 2021
Nice to see this merged. Thank you all! |
GrahamCampbell commentedSep 6, 2021
🎉 |
It is now well handled by the framework itself, seesymfony/symfony#41390
It is now well handled by the framework itself, seesymfony/symfony#41390
It is now well handled by the framework itself, seesymfony/symfony#41390
…onListener (nicolas-grekas)This PR was merged into the 5.4 branch.Discussion----------[HttpKernel] Fix advertizing deprecations for *TestSessionListener| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Looks like we missed that during the review of#41390Commits-------3e209c3 [HttpKernel] Fix advertizing deprecations for *TestSessionListener
Uh oh!
There was an error while loading.Please reload this page.
Currently the session cookie is never set on the
requestobject. In a normal webserver setup this is not needed as a session is in php-fpm or apache fastcgi written by php itself when the response is outputted in:symfony/src/Symfony/Component/HttpFoundation/Response.php
Line 393 in744b901
When use a runner like
Roadrunnerthesessioncookie is never set on theRequestobject, as this kind of appserver never really outputs something and is running in theclicontext.Actually there is no way from outside to know if
$session->save()was called or not because when the code inRoadrunnerexecutes the session is actually written and so closed here:symfony/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
Line 279 in744b901
The best way so to fix this issue is that in CLI context symfony writes the session cookie on the
Responseobject when the session is really saved.This fixes issues which we have in the current roadrunner implementation of php runtime:
Beside Roadrunner there is also Swoole implementation which would require the same here:php-runtime/runtime#60
With this changes the Runners can be simplified a lot:https://github.com/php-runtime/runtime/pull/62/files#
TODO
Reset session variables after successfully response. The following code currently lives also in the runtime implementation but is requires also by all runners and so we should find a place where we put it:
EDIT: Added this now to the
AbstractSessionListenerservice viaResetInterface.