-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Set session cookie only when not empty #44657
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
Conversation
43c83f5
to
3e4a8df
Compare
3e4a8df
to
e89eb3a
Compare
@@ -151,7 +151,8 @@ public function onKernelResponse(ResponseEvent $event) | |||
$request = $event->getRequest(); | |||
$requestSessionCookieId = $request->cookies->get($sessionName); | |||
|
|||
if ($requestSessionCookieId && $session->isEmpty()) { | |||
$isSessionEmpty = $session->isEmpty(); |
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.
Need in 6.0 be replaced with the new logic:
symfony/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
Line 149 in b2e7fcd
if ($requestSessionCookieId && ($session instanceof Session ? $session->isEmpty() : empty($session->all()))) { |
Hey! I think @mtarld has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
@@ -160,7 +161,7 @@ public function onKernelResponse(ResponseEvent $event) | |||
$sessionCookieHttpOnly, | |||
$sessionCookieSameSite | |||
); | |||
} elseif ($sessionId !== $requestSessionCookieId) { | |||
} elseif ($sessionId !== $requestSessionCookieId && !$isSessionEmpty) { |
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.
maybe we should even not write the session in the backend when it's empty?
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 a value is removed we need to write it to remove that value from the session bag.
for making things simpler for me I moving this changes to #44634 |
A session is normally not written when empty so we need to check for empty before set the session cookie.
TODO: