[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699
Conversation
9dffde8 to
29e4128
Compare
| /** | ||
| * @internal | ||
| */ | ||
| public function setSessionFactory(callable $factory) |
There was a problem hiding this comment.
This provides extra laziness, needed to account for symfony/recipes#333.
9a1e7a0 to
f3a8cc3
Compare
| if (null === $session || $request->hasSession()) { | ||
| return; | ||
| if (!$request->hasSession()) { | ||
| $request->setSessionFactory(function () { return $this->getSession(); }); |
There was a problem hiding this comment.
Wouldn't it be more secure to isolate the listener in the closure by injecting it via a use instead of using $this that can be changed before executing the closure? Maybe I'm being paranoid, so just asking 😄
There was a problem hiding this comment.
Wow we never closed that far. Not sure it makes sense.
Here particularly there is nothing to close: the closure cannot be accessed.
| @@ -44,19 +45,22 @@ | |||
| */ | |||
| class SaveSessionListener implements EventSubscriberInterface | |||
| { | |||
| private $container; | |||
There was a problem hiding this comment.
please add explicit typehint for this property, typehint for constructor parameter is not the same as typehint for property
There was a problem hiding this comment.
We actually removed such duplicate comments from the code base.
There was a problem hiding this comment.
👎 on that, when one would do sth later with this code, he need to remember that perhaps he need to add that typehinting (eg, removing obtaining the parameter via constructor, changing visibility to public and so). I agree that for private it is no big deal, yet still.
And we do had already issues with field that was assumed to be type X, but in the end it was null|X, just not documented...
There was a problem hiding this comment.
I respect your opinion, but this is not the proper place to discuss this. Right now, this is just consistent with the code base.
There was a problem hiding this comment.
I treat it same as with params, as long as we cannot typehint it on lang level, we do it in docblock ;)
Would be great if we would be able to do it with any var, like here with the property, but we are not there yet ;)
Well, I'm bet I just saw too many fuckups so I'm extra defensive...
|
I'm going to split the extra-laziness part for 4.1 Status: needs work |
f3a8cc3 to
8d52152
Compare
|
Extra-laziness removed, PR is now strictly a bug fix. Status: needs review |
There was a problem hiding this comment.
Wouldn't calling the migrate and save methods on the Session object also start the session? i.e. we would need to set hasStarted to true ?
It's also don't really get the difference between isStarted and hasStarted now 🤔Isn't it having some logic of the storage down to the Session object?
8d52152 to
e81e3a5
Compare
612bb6a to
266e156
Compare
|
I finally figured out a decent way to implement this, here we are. |
018ee58 to
1eaeef6
Compare
0cf2c68 to
bd5ebac
Compare
that's exactly what the
feel free to submit an alternate PR (but keep in mind this should remain a bug fix, no time for refactorizations.) |
That's probably a good reason not to refactor 👍 |
daca567 to
294202e
Compare
|
and now green |
294202e to
bf41b23
Compare
| return; | ||
| } | ||
|
|
||
| if ($session->isStarted() || ($session instanceof Session && ((\method_exists(Session::class, 'hasBeenStarted') && $session->hasBeenStarted()) || (\method_exists(Session::class, 'isEmpty') && !$session->isEmpty())))) { |
There was a problem hiding this comment.
Why does it need to make the response uncachable when the session has been started but there is no data?
There was a problem hiding this comment.
Because this is the semantic of the cache control header in HTTP: if the session is used, whatever it contains, then it means the page can be different for another user, and thus the page cannot be cached.
That's not new at all.
There was a problem hiding this comment.
Hum, your question might be related to the isEmpty call on this line.
In fact this call is only required for maximum compatibility with soon-legacy versions of HttpKernel, where hasBeenStarted is not available.
We could also bump the minimum version requirement from 3.3 to 3.4.4.
I'll do that :)
97847c0 to
f0bd401
Compare
f0bd401 to
f8727b8
Compare
|
Minimum version of http-foundation bumped. |
|
Thank you @nicolas-grekas. |
…tting response "private" (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] Fix session handling: decouple "save" from setting response "private" | 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 | - Fixes #25583 (comment) from @Tobion, and provides extra laziness for the "session" service, related to symfony/recipes#333. (deps=high failure will be fixed by merging to upper branches.) Commits ------- f8727b8 [HttpKernel] Fix session handling: decouple "save" from setting response "private"
…rivate automatically (Toflar) This PR was squashed before being merged into the 4.1-dev branch (closes #26681). Discussion ---------- Allow to easily ask Symfony not to set a response to private automatically | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This PR is related to the many discussions going on about the latest (Symfony 3.4+) changes regarding the way Symfony handles the session. I think we're almost there now, Symfony 4.1 automatically turns responses into `private` responses once the session is started and it's all done in the `AbstractSessionListener` where the session is also saved. In other issues/PRs (symfony/symfony#25583, symfony/symfony#25699, symfony/symfony#24988) it was agreed that setting the response to `private` if the session is started is a good default for Symfony. It was also agreed that setting it to `private` does not always make sense because you **can share a response** across sessions, it just requires a more complex caching setup with shared user context etc. So there must be an easy way to disable this behaviour. Right now it's very hard to do so because what you end up doing is basically decorating the `session_listener` which is very hard because you have to keep track on that over different Symfony versions as the base listener might get additional features etc. The [FOSCacheBundle](FriendsOfSymfony/FOSHttpCacheBundle#438) is already having this problem, [Contao](contao/core-bundle#1388) has the same issue and there will be probably more. Basically everyone that wants to share a response cache across the session will have to decorate the default listener. That's just too hard, so I came up with this solution. The header is easy. Every project can add that easily. It does not require any extension, configuration or adjustment of any service. It's clean, transparent and has absolutely no impact on "default" Symfony setups. Would be happy to have some feedback before 4.1 freeze. Commits ------- 0f36710 Allow to easily ask Symfony not to set a response to private automatically
…rivate automatically (Toflar) This PR was squashed before being merged into the 4.1-dev branch (closes #26681). Discussion ---------- Allow to easily ask Symfony not to set a response to private automatically | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This PR is related to the many discussions going on about the latest (Symfony 3.4+) changes regarding the way Symfony handles the session. I think we're almost there now, Symfony 4.1 automatically turns responses into `private` responses once the session is started and it's all done in the `AbstractSessionListener` where the session is also saved. In other issues/PRs (#25583, #25699, #24988) it was agreed that setting the response to `private` if the session is started is a good default for Symfony. It was also agreed that setting it to `private` does not always make sense because you **can share a response** across sessions, it just requires a more complex caching setup with shared user context etc. So there must be an easy way to disable this behaviour. Right now it's very hard to do so because what you end up doing is basically decorating the `session_listener` which is very hard because you have to keep track on that over different Symfony versions as the base listener might get additional features etc. The [FOSCacheBundle](FriendsOfSymfony/FOSHttpCacheBundle#438) is already having this problem, [Contao](contao/core-bundle#1388) has the same issue and there will be probably more. Basically everyone that wants to share a response cache across the session will have to decorate the default listener. That's just too hard, so I came up with this solution. The header is easy. Every project can add that easily. It does not require any extension, configuration or adjustment of any service. It's clean, transparent and has absolutely no impact on "default" Symfony setups. Would be happy to have some feedback before 4.1 freeze. Commits ------- 0f36710 Allow to easily ask Symfony not to set a response to private automatically
Fixes #25583 (comment) from @Tobion, and provides extra laziness for the "session" service, related to symfony/recipes#333.
(deps=high failure will be fixed by merging to upper branches.)