Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Fix session handling: decouple "save" from setting response "private"#25699
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * @internal | ||
| */ | ||
| publicfunctionsetSessionFactory(callable$factory) |
nicolas-grekasJan 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This provides extra laziness, needed to account forsymfony/recipes#333.
9a1e7a0 tof3a8cc3Compare| if (null ===$session ||$request->hasSession()) { | ||
| return; | ||
| if (!$request->hasSession()) { | ||
| $request->setSessionFactory(function () {return$this->getSession(); }); |
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.
Wouldn't it be more secure to isolate the listener in the closure by injecting it via ause 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wow we never closed that far. Not sure it makes sense.
Here particularly there is nothing to close: the closure cannot be accessed.
| */ | ||
| class SaveSessionListenerimplements EventSubscriberInterface | ||
| { | ||
| private$container; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We actually removed such duplicate comments from the code base.
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.
👎 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 wasnull|X, just not documented...
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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...
nicolas-grekas commentedJan 8, 2018
I'm going to split the extra-laziness part for 4.1 Status: needs work |
nicolas-grekas commentedJan 8, 2018
Extra-laziness removed, PR is now strictly a bug fix. Status: needs review |
sroze left a comment
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.
Wouldn't calling themigrate andsave methods on theSession object also start the session? i.e. we would need to sethasStarted totrue ?
It's also don't really get the difference betweenisStarted andhasStarted now 🤔Isn't it having some logic of the storage down to the Session object?
612bb6a to266e156Comparenicolas-grekas commentedJan 9, 2018
I finally figured out a decent way to implement this, here we are. |
018ee58 to1eaeef6Comparenicolas-grekas commentedJan 9, 2018
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.) |
sroze commentedJan 9, 2018
That's probably a good reason not to refactor 👍 |
daca567 to294202eComparenicolas-grekas commentedJan 9, 2018
and now green |
| return; | ||
| } | ||
| if ($session->isStarted() || ($sessioninstanceof Session && ((\method_exists(Session::class,'hasBeenStarted') &&$session->hasBeenStarted()) || (\method_exists(Session::class,'isEmpty') && !$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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tof0bd401Comparenicolas-grekas commentedJan 10, 2018
Minimum version of http-foundation bumped. |
fabpot commentedJan 10, 2018
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 tosymfony/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
Uh oh!
There was an error while loading.Please reload this page.
Fixes#25583 (comment) from@Tobion, and provides extra laziness for the "session" service, related tosymfony/recipes#333.
(deps=high failure will be fixed by merging to upper branches.)