[HttpKernel] Make AbstractTestSessionListener compatible with CookieClearingLogoutHandler #27659
Conversation
|
ping @rpkamp I suppose. |
| @@ -71,6 +71,13 @@ public function onKernelResponse(FilterResponseEvent $event) | |||
|
|
|||
| if ($session instanceof Session ? !$session->isEmpty() || (null !== $this->sessionId && $session->getId() !== $this->sessionId) : $wasStarted) { | |||
| $params = session_get_cookie_params(); | |||
|
|
|||
| foreach ($event->getResponse()->headers->getCookies() as $cookie) { | |||
| if ($session->getName() === $cookie->getName() && $params['path'] == $cookie->getPath() && $params['domain'] == $cookie->getDomain()) { | |||
There was a problem hiding this comment.
Please use === instead of ==
There was a problem hiding this comment.
They could be null or empty strings.
There was a problem hiding this comment.
can you add a test case where this would fail when the first == is turned into a === please?
There was a problem hiding this comment.
Looking again, the path will always be a string and at least /, so can make that comparison strict. The domain, however, is null in Cookie but an empty string in $params['domain'] (so the existing test cases already fail if that's made strict).
There was a problem hiding this comment.
You can always cast the value from Cookie to a string and then use strict comparison. I think that would still be better than loose comparison.
$params['domain'] === (string) $cookie->getDomain()
There was a problem hiding this comment.
both sides could need the cast.
but since this is tested, that's not really needed anyway.
|
|
||
| $response = $this->filterResponse(new Request(), HttpKernelInterface::MASTER_REQUEST, $response); | ||
|
|
||
| $this->assertSame($expected, $response->headers->get('Set-Cookie', null, false)); |
There was a problem hiding this comment.
It's not really clear to me what this PR for. You state that it is to make it compatible with the CookieClearingLogoutHandler, but then I would expect a test that shows that no cookies are sent. However, you've added no tests that show this.
Furthermore, I doubt if this fix is the correct way to do it. It feels like it's battling symptoms instead of the root problem, but I can't quite lay my finger on it.
There was a problem hiding this comment.
I still expect the original cookie to be sent (ie the one set by CookieClearingLogoutHandler).
There was a problem hiding this comment.
But the CookieClearingLogoutHandler doesn't set cookies, it removes cookies. That's the whole point.
There was a problem hiding this comment.
To 'remove' a cookie you have to set a new expired one, hence:
symfony/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php
Lines 248 to 251 in 6064cfe
There was a problem hiding this comment.
aaah, check, I get it now.
In that case, LGTM
|
Should be back as 'Needs Review'. |
| @@ -71,6 +71,13 @@ public function onKernelResponse(FilterResponseEvent $event) | |||
|
|
|||
| if ($session instanceof Session ? !$session->isEmpty() || (null !== $this->sessionId && $session->getId() !== $this->sessionId) : $wasStarted) { | |||
| $params = session_get_cookie_params(); | |||
|
|
|||
| foreach ($event->getResponse()->headers->getCookies() as $cookie) { | |||
| if ($session->getName() === $cookie->getName() && $params['path'] == $cookie->getPath() && $params['domain'] == $cookie->getDomain()) { | |||
There was a problem hiding this comment.
can you add a test case where this would fail when the first == is turned into a === please?
…learingLogoutHandler
2725364 to
f54d969
Compare
|
Thank you @thewilkybarkid. |
…ith CookieClearingLogoutHandler (thewilkybarkid) This PR was squashed before being merged into the 3.4 branch (closes #27659). Discussion ---------- [HttpKernel] Make AbstractTestSessionListener compatible with CookieClearingLogoutHandler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | #26157 started to send a new cookie in `AbstractTestSessionListener`, but is incompatible with `CookieClearingLogoutHandler` as it overrides its `Set-Cookie` by setting a new cookie (breaking my test that checked to see that the cookie was removed after a log out). Commits ------- f54d969 [HttpKernel] Make AbstractTestSessionListener compatible with CookieClearingLogoutHandler
#26157 started to send a new cookie in
AbstractTestSessionListener, but is incompatible withCookieClearingLogoutHandleras it overrides itsSet-Cookieby setting a new cookie (breaking my test that checked to see that the cookie was removed after a log out).