[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

nicolas-grekas · 2018-01-06T07:59:18Z

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.)

nicolas-grekas · 2018-01-06T09:06:40Z

src/Symfony/Component/HttpFoundation/Request.php

+    /**
+     * @internal
+     */
+    public function setSessionFactory(callable $factory)


This provides extra laziness, needed to account for symfony/recipes#333.

Pierstoval · 2018-01-06T16:58:18Z

src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php

-        if (null === $session || $request->hasSession()) {
-            return;
+        if (!$request->hasSession()) {
+            $request->setSessionFactory(function () { return $this->getSession(); });


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 😄

Wow we never closed that far. Not sure it makes sense.
Here particularly there is nothing to close: the closure cannot be accessed.

keradus · 2018-01-06T22:22:23Z

src/Symfony/Component/HttpKernel/EventListener/SaveSessionListener.php

@@ -44,19 +45,22 @@
 */
 class SaveSessionListener implements EventSubscriberInterface
 {
+    private $container;


please add explicit typehint for this property, typehint for constructor parameter is not the same as typehint for property

We actually removed such duplicate comments from the code base.

👎 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...

I respect your opinion, but this is not the proper place to discuss this. Right now, this is just consistent with the code base.

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 · 2018-01-08T19:42:33Z

I'm going to split the extra-laziness part for 4.1

Status: needs work

nicolas-grekas · 2018-01-08T20:40:37Z

Extra-laziness removed, PR is now strictly a bug fix.

Status: needs review

sroze

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?

nicolas-grekas · 2018-01-09T07:57:50Z

I finally figured out a decent way to implement this, here we are.

nicolas-grekas · 2018-01-09T11:09:37Z

wasStarted argument on the SessionBagProxy as a sort of hidden observable

that's exactly what the SessionBagProxy is for (and why it's internal also)

I'd properly decouple your logic

feel free to submit an alternate PR (but keep in mind this should remain a bug fix, no time for refactorizations.)

sroze · 2018-01-09T11:23:54Z

that's exactly what the SessionBagProxy is for (and why it's internal also)

That's probably a good reason not to refactor 👍

nicolas-grekas · 2018-01-09T12:34:03Z

and now green

Tobion · 2018-01-10T01:59:18Z

src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php

+            return;
+        }
+
+        if ($session->isStarted() || ($session instanceof Session && ((\method_exists(Session::class, 'hasBeenStarted') && $session->hasBeenStarted()) || (\method_exists(Session::class, 'isEmpty') && !$session->isEmpty())))) {


Why does it need to make the response uncachable when the session has been started but there is no data?

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.

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 :)

…nse "private"

nicolas-grekas · 2018-01-10T08:25:00Z

Minimum version of http-foundation bumped.

fabpot · 2018-01-10T09:23:09Z

Thank you @nicolas-grekas.

@Tobion

…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

nicolas-grekas added this to the 3.4 milestone Jan 6, 2018

carsonbot added Status: Needs Review HttpKernel Bug labels Jan 6, 2018

nicolas-grekas mentioned this pull request Jan 6, 2018

[HttpKernel] Call Response->setPrivate() instead of sending raw header() when session is started #25583

Merged

nicolas-grekas force-pushed the fix-session branch from 9dffde8 to 29e4128 Compare January 6, 2018 09:01

nicolas-grekas mentioned this pull request Jan 6, 2018

[symfony/framework-bundle] Enable session support by default symfony/recipes#333

Merged

nicolas-grekas commented Jan 6, 2018

View reviewed changes

nicolas-grekas force-pushed the fix-session branch 2 times, most recently from 9a1e7a0 to f3a8cc3 Compare January 6, 2018 15:01

Pierstoval reviewed Jan 6, 2018

View reviewed changes

keradus reviewed Jan 6, 2018

View reviewed changes

nicolas-grekas mentioned this pull request Jan 7, 2018

[FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded #25508

Merged

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 8, 2018

nicolas-grekas force-pushed the fix-session branch from f3a8cc3 to 8d52152 Compare January 8, 2018 20:40

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 8, 2018

sroze reviewed Jan 8, 2018

View reviewed changes

nicolas-grekas force-pushed the fix-session branch from 8d52152 to e81e3a5 Compare January 8, 2018 22:38

symfony deleted a comment from dmaicher Jan 8, 2018

nicolas-grekas force-pushed the fix-session branch 4 times, most recently from 612bb6a to 266e156 Compare January 9, 2018 07:57

nicolas-grekas force-pushed the fix-session branch 2 times, most recently from 018ee58 to 1eaeef6 Compare January 9, 2018 08:40

nicolas-grekas force-pushed the fix-session branch from 0cf2c68 to bd5ebac Compare January 9, 2018 11:06

nicolas-grekas force-pushed the fix-session branch 2 times, most recently from daca567 to 294202e Compare January 9, 2018 12:24

nicolas-grekas force-pushed the fix-session branch from 294202e to bf41b23 Compare January 9, 2018 12:59

Tobion reviewed Jan 10, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 10, 2018

fabpot approved these changes Jan 10, 2018

View reviewed changes

nicolas-grekas force-pushed the fix-session branch 2 times, most recently from 97847c0 to f0bd401 Compare January 10, 2018 08:22

[HttpKernel] Fix session handling: decouple "save" from setting respo…

f8727b8

…nse "private"

nicolas-grekas force-pushed the fix-session branch from f0bd401 to f8727b8 Compare January 10, 2018 08:23

fabpot merged commit f8727b8 into symfony:3.4 Jan 10, 2018

nicolas-grekas mentioned this pull request Jan 12, 2018

Auto add the cache-control headers? #25736

Closed

leofeyer mentioned this pull request Jan 22, 2018

Session handling in Contao 4.4 contao/core-bundle#1283

Closed

This was referenced Jan 29, 2018

Release v3.4.4 #25956

Merged

Release v4.0.4 #25957

Merged

thewilkybarkid mentioned this pull request Jan 29, 2018

Remove polyfills elifesciences/journal#834

Merged

nicolas-grekas deleted the fix-session branch February 19, 2018 10:35

smurfy mentioned this pull request Feb 19, 2018

PR #25583 and #25699 BC Break #26237

Closed

Toflar mentioned this pull request Mar 27, 2018

Allow to easily ask Symfony not to set a response to private automatically #26681

Merged

Spomky mentioned this pull request Jul 23, 2018

JWKSet Cache Control Header ignored web-token/jwt-framework#142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

nicolas-grekas commented Jan 6, 2018 •

edited

Loading

nicolas-grekas Jan 6, 2018 •

edited

Loading

Pierstoval Jan 6, 2018

nicolas-grekas Jan 6, 2018

keradus Jan 6, 2018

nicolas-grekas Jan 7, 2018

keradus Jan 7, 2018

nicolas-grekas Jan 7, 2018

keradus Jan 7, 2018

nicolas-grekas commented Jan 8, 2018

nicolas-grekas commented Jan 8, 2018

sroze left a comment

nicolas-grekas commented Jan 9, 2018

nicolas-grekas commented Jan 9, 2018

sroze commented Jan 9, 2018

nicolas-grekas commented Jan 9, 2018

Tobion Jan 10, 2018 •

edited

Loading

nicolas-grekas Jan 10, 2018

nicolas-grekas Jan 10, 2018

nicolas-grekas commented Jan 10, 2018

fabpot commented Jan 10, 2018

[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

[HttpKernel] Fix session handling: decouple "save" from setting response "private" #25699

Conversation

nicolas-grekas commented Jan 6, 2018 • edited Loading

nicolas-grekas Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 8, 2018

nicolas-grekas commented Jan 8, 2018

sroze left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 9, 2018

nicolas-grekas commented Jan 9, 2018

sroze commented Jan 9, 2018

nicolas-grekas commented Jan 9, 2018

Tobion Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 10, 2018

fabpot commented Jan 10, 2018

nicolas-grekas commented Jan 6, 2018 •

edited

Loading

nicolas-grekas Jan 6, 2018 •

edited

Loading

Tobion Jan 10, 2018 •

edited

Loading