[RTM] Do not make the response private when saving the session #1388

leofeyer · 2018-02-20T13:52:29Z

This PR fixes #1283.

Symfony has recently changed their session listener to always make the response private if the session has been started. Although we agree with the change, it renders the HTTP cache unusable, because Contao always starts a session (e.g. to store the user's language). This PR decorates the Symfony session listener and circumvents Symfony's changes by not making the response private if the request is a Contao front end request.

Of course, our long term goal should be to adjust Contao so it only starts a session if it really has to.

Toflar · 2018-03-01T08:25:31Z

src/EventListener/SessionListener.php

+use Symfony\Component\HttpKernel\EventListener\SessionListener as BaseSessionListener;
+
+/**
+ * Decorates the default session listener.


There's no explanation why this listener is present, what it does and when it will be removed again. I think that should be added because it's important information (it should also go into the PR description).

).

Toflar · 2018-03-23T12:25:52Z

src/EventListener/SessionListener.php

+            return;
+        }
+
+        $session = $event->getRequest()->getSession();


@leofeyer There's an issue here. Either we have to call $this->inner->onKernelResponse($event); at the end of the method again or we can leave out the whole save() part here because we're overriding the method.

No, we must not call $this->inner->onKernelResponse($event) here otherwise we have the same problem (private response) again. And saving is required to be forward compatible with Symfony 4.1.

This does not make sense to me. If you do not forward to the decorated service, why do you call save() then?

So the follow up listener, which checks $session->isStarted(), is not run.

Which follow up listener? I think you confuse stuff here. The one that checks for this is the one you decorate, no?

I'm not confusing anything. We have carefully analyzed the program flow and set up the existing solution during the developer's meeting. Which issues does it cause for you?

…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

Description ----------- Fixes #1388. Commits ------- 49bf3a28 Update preview_toolbar_base.html.twig b7a8e8f6 Update default.xlf 3710cd16 Use trans_default_domain

leofeyer added the bug label Feb 20, 2018

leofeyer added this to the 4.4.15 milestone Feb 20, 2018

leofeyer self-assigned this Feb 20, 2018

leofeyer requested a review from Toflar February 20, 2018 13:52

leofeyer changed the title ~~[RTM] Set the front end response cache headers in an event listener~~ [RTM] Set the front end response cache headers in a listener Feb 20, 2018

leofeyer mentioned this pull request Feb 20, 2018

Session handling in Contao 4.4 #1283

Closed

Toflar mentioned this pull request Feb 20, 2018

Tasks to fix and improve caching #1389

Closed

9 tasks

leofeyer changed the title ~~[RTM] Set the front end response cache headers in a listener~~ [RFC] Do not make the response private when saving the session Feb 20, 2018

leofeyer changed the title ~~[RFC] Do not make the response private when saving the session~~ [RTM] Do not make the response private when saving the session Feb 20, 2018

leofeyer mentioned this pull request Feb 20, 2018

Fix http_cache of issue 1246 #1368

Closed

leofeyer requested review from ausi, Toflar and aschempp and removed request for Toflar March 1, 2018 08:05

Toflar suggested changes Mar 1, 2018

View reviewed changes

leofeyer added 5 commits March 1, 2018 10:58

Set the front end response cache headers in an event listener (see #1283

6ecc514

).

Fix the copyright notice.

f937aaf

Do not make the response private when saving the session (see #1388).

99ad48d

Add the unit tests.

41580e2

Add a description to the session listener.

e967856

Toflar approved these changes Mar 1, 2018

View reviewed changes

leofeyer merged commit 980fdda into contao:4.4 Mar 1, 2018

leofeyer deleted the hotfix/http-cache branch March 1, 2018 10:16

dbu mentioned this pull request Mar 20, 2018

work around symfony 4 auto-adding cache control headers FriendsOfSymfony/FOSHttpCacheBundle#433

Closed

Toflar reviewed Mar 23, 2018

View reviewed changes

Toflar mentioned this pull request Mar 27, 2018

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

Merged

leofeyer modified the milestones: 4.4.15, 4.4 May 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Do not make the response private when saving the session #1388

[RTM] Do not make the response private when saving the session #1388

leofeyer commented Feb 20, 2018 •

edited

Loading

Toflar Mar 1, 2018

leofeyer Mar 1, 2018

Toflar Mar 23, 2018

leofeyer Mar 23, 2018 •

edited

Loading

Toflar Mar 23, 2018

leofeyer Mar 23, 2018

Toflar Mar 23, 2018

leofeyer Mar 23, 2018 •

edited

Loading

[RTM] Do not make the response private when saving the session #1388

[RTM] Do not make the response private when saving the session #1388

Conversation

leofeyer commented Feb 20, 2018 • edited Loading

Toflar Mar 1, 2018

Choose a reason for hiding this comment

leofeyer Mar 1, 2018

Choose a reason for hiding this comment

Toflar Mar 23, 2018

Choose a reason for hiding this comment

leofeyer Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Toflar Mar 23, 2018

Choose a reason for hiding this comment

leofeyer Mar 23, 2018

Choose a reason for hiding this comment

Toflar Mar 23, 2018

Choose a reason for hiding this comment

leofeyer Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

leofeyer commented Feb 20, 2018 •

edited

Loading

leofeyer Mar 23, 2018 •

edited

Loading

leofeyer Mar 23, 2018 •

edited

Loading