-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy 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
Conversation
5d51c41
to
be9fc75
Compare
src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
Outdated
Show resolved
Hide resolved
f5a49e5
to
4cc89f7
Compare
src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml
Outdated
Show resolved
Hide resolved
aac0ccb
to
ab83e8a
Compare
ab83e8a
to
fbfc9cf
Compare
fbfc9cf
to
647a4e0
Compare
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.
@Nyholm Yes the PR itself is ready for review, I want still provide some tests for the Listener logic. |
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 pushed a small commit to fix the tests.
@@ -325,7 +325,7 @@ public function load(array $configs, ContainerBuilder $container) | |||
$this->sessionConfigEnabled = true; | |||
$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 from TestSessionListener
to SessionListener
@@ -142,7 +142,7 @@ public function onKernelResponse(ResponseEvent $event) | |||
$sessionId = $session->getId(); | |||
$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.php
Show resolved
Hide resolved
8bf60c5
to
b3e4f66
Compare
Thank you @alexander-schranz. |
Awesome. |
Nice to see this merged. Thank you all! |
🎉 |
It is now well handled by the framework itself, see symfony/symfony#41390
It is now well handled by the framework itself, see symfony/symfony#41390
It is now well handled by the framework itself, see symfony/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 #41390 Commits ------- 3e209c3 [HttpKernel] Fix advertizing deprecations for *TestSessionListener
Currently the session cookie is never set on the
request
object. 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 in 744b901
When use a runner like
Roadrunner
thesession
cookie is never set on theRequest
object, as this kind of appserver never really outputs something and is running in thecli
context.Actually there is no way from outside to know if
$session->save()
was called or not because when the code inRoadrunner
executes the session is actually written and so closed here:symfony/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php
Line 279 in 744b901
The best way so to fix this issue is that in CLI context symfony writes the session cookie on the
Response
object 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
AbstractSessionListener
service viaResetInterface
.