[HttpKernel] Let the storage manage the session starts #24774
Conversation
|
IMO that's not correct fix. Data collector should not start the session as the dev application behavior is then different from prod. |
|
Not, from the top of my head. But I know that I fixed something similar e.g DataCollector starting session somewhere in sf 2.0 times. As it was a great annoyance. |
|
@nicolas-grekas can you check this to see what is the impact of the changes you made to the session and how this should best be solved ? |
|
The impact is meeting the expectations set then, nothing more: if the session is empty, it isn't written nor started. This "isStarted" check is strange, as it binds a feature of the profiler to the behavior of the page being monitored. Maybe legitimate of course. This PR looks ok to me though. |
02bcd53 to
597cf34
Compare
|
Removed the test asserting that the session is not started, as obviously, this might be now... |
|
Thank you @sroze. |
…oze) This PR was squashed before being merged into the 3.4 branch (closes #24774). Discussion ---------- [HttpKernel] Let the storage manage the session starts | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24730 | License | MIT | Doc PR | ø HttpKernel's request collector should not really care if the session has started or not, be let the storage decide. Without the session, it is not possible to track the redirected pages. I don't think the consideration of "we should not start the session if not needed by the user's code" applies here as if this is running, that is very likely that the user is running the dev environment anyway. Commits ------- 95d0b72 [HttpKernel] Let the storage manage the session starts
|
This change is causing a functional test of mine to fail. I am testing that deleting the current session logs the user out. I am calling |
|
@kbond could you open an issue for this, explaining exactly what is wrong? As I'm not sure how this change would impact your test. Cheers! |
| @@ -315,7 +315,7 @@ public function onKernelController(FilterControllerEvent $event) | ||
|
|
||
| public function onKernelResponse(FilterResponseEvent $event) | ||
| { | ||
| if (!$event->isMasterRequest() || !$event->getRequest()->hasSession() || !$event->getRequest()->getSession()->isStarted()) { | ||
| if (!$event->isMasterRequest() || !$event->getRequest()->hasSession()) { |
There was a problem hiding this comment.
Specifically, this change is causing my problem.
…n (sroze) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] Uses cookies to track the requests redirection | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25698 | License | MIT | Doc PR | ø In order to track the redirections across requests, we need to have some state. So far, we've been using the session but some users have complained about it (#24774, #24730). The idea is that we don't actually need the session, we can use cookies. It's a tradeoff: using a cookie would mean that both the redirection and the target page will not be cachable (because of the Set-Cookie to set the sf_redirect and the one to clear it). As it's only on dev, it seems fair to say that having no cache (because of `Set-Cookie`s) is a better side effect than starting the session. Commits ------- 83f2579 Uses cookies to track the requests redirection
HttpKernel's request collector should not really care if the session has started or not, be let the storage decide. Without the session, it is not possible to track the redirected pages.
I don't think the consideration of "we should not start the session if not needed by the user's code" applies here as if this is running, that is very likely that the user is running the dev environment anyway.