[Kernel] ensure session is saved before sending response #12341
Conversation
| namespace Symfony\Component\HttpKernel\EventListener; | ||
|
|
||
| use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
| use Symfony\Component\HttpKernel\Event\FinishRequestEvent; |
There was a problem hiding this comment.
There is not such class in version 2.3, it was introduced in version 2.4.
There was a problem hiding this comment.
@stloyd thanks. I fixed it.
bb0ffc4 to
0e677eb
Compare
|
@Tobion So, I think you need to also submit a PR for 2.5. |
|
instead of doing it on master kernel.response with a low priority, which can still break for people streaming responses for instance, I think it would be better to do it on kernel.terminate with a high priority (the goal being to save the session before doing the heavy termination logic) |
|
@stof that's useless. Again the point, as documented, is to save the session before it is send(). Terminate would be too late. |
|
@Tobion the issue is that this will also save before the processing of streamed responses. The right time to save is actually when finishing the send() call (but before the termination logic), not before the send() call |
|
@stof what exactly is the problem with streamed responses? |
|
@fabpot After this is merged into 2.3 and 2.4, I can make a PR on 2.4 to change the listener to listen on |
|
Don't you fear getting bug reports from users changing session state after the event? |
|
This might actually solve some issues I have with a database session storage and a Captcha throwing 404s because it has no code in the session (yet). |
|
@nicolas-grekas no because symfony session starts on demand. So writing to the session after it closed, still works as expected. |
|
@stof streamed responses are sent via https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/StreamedResponseListener.php aren't they? And we want the session to be saved before the headers are sent. So the SaveSessionListener should have a higher priority that the StreamedResponseListener. |
|
👍 |
0e677eb to
b7bfef0
Compare
|
Thank you @Tobion. |
…Tobion) This PR was merged into the 2.3 branch. Discussion ---------- [Kernel] ensure session is saved before sending response | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #6417, #7885 | License | MIT | Doc PR | n/a Saves the session, in case it is still open, before sending the response. This ensures several things in case the developer did not save the session explicitly: - If a session save handler without locking is used, it ensures the data is available on the next request, e.g. after a redirect. PHPs auto-save at script end via session_register_shutdown is executed after fastcgi_finish_request. So in this case the data could be missing the next request because it might not be saved the moment the new request is processed. - A locking save handler (e.g. the native 'files') circumvents concurrency problems like the one above. By saving the session before long-running things in the terminate event, we ensure the session is not blocked longer than needed. - When regenerating the session ID no locking is involved in PHPs session design. See https://bugs.php.net/bug.php?id=61470 for a discussion. So in this case, the session must be saved anyway before sending the headers with the new session ID. Otherwise session data could get lost again for concurrent requests with the new ID. One result could be that you get logged out after just logging in. This listener should be executed as one of the last listeners, so that previous listeners can still operate on the open session. This prevents the overhead of restarting it. Listeners after closing the session can still work with the session as usual because Symfonys session implementation starts the session on demand. So writing to it after it is saved will just restart it. Commits ------- b7bfef0 [Kernel] ensure session is saved before sending response
This PR was merged into the 2.3 branch. Discussion ---------- [Session] remove invalid hack in session regenerate | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The original issue #7380 was just caused because the developer missed to save the session before doing the redirect. That's all. Such mistakes won't happen anymore with #12341 This reverts #8270 and following. Also it makes absolutely no sense to do this only for the `files` save handler which creates huge inconsistencies. All save handlers are affected and it's more a documentation thing. Commits ------- 703d906 [Session] remove invalid workaround in session regenerate
Saves the session, in case it is still open, before sending the response.
This ensures several things in case the developer did not save the session explicitly:
on the next request, e.g. after a redirect. PHPs auto-save at script end via
session_register_shutdown is executed after fastcgi_finish_request. So in this case
the data could be missing the next request because it might not be saved the moment
the new request is processed.
the one above. By saving the session before long-running things in the terminate event,
we ensure the session is not blocked longer than needed.
https://bugs.php.net/bug.php?id=61470 for a discussion. So in this case, the session must
be saved anyway before sending the headers with the new session ID. Otherwise session
data could get lost again for concurrent requests with the new ID. One result could be
that you get logged out after just logging in.
This listener should be executed as one of the last listeners, so that previous listeners
can still operate on the open session. This prevents the overhead of restarting it.
Listeners after closing the session can still work with the session as usual because
Symfonys session implementation starts the session on demand. So writing to it after
it is saved will just restart it.