adjust to session listener changes in symfony 4.1 #438

dbu · 2018-03-27T06:55:04Z

The SessionListener in Symfony 4.1 is changed and does the $session->save() itself.

dbu · 2018-03-27T06:57:58Z

@nicolas-grekas could you maybe have a look at this? for the user context caching, we explicitly want to have cache headers on responses that have a session. with the changes in symfony 4.1, this seems like the only way of achieving this.

Toflar · 2018-03-27T06:57:31Z

src/EventListener/SessionListener.php

     * @param BaseSessionListener $inner
     * @param string              $userHashHeader        Must be lower-cased
     * @param array               $userIdentifierHeaders Must be lower-cased
     */
-    public function __construct(BaseSessionListener $inner, string $userHashHeader, array $userIdentifierHeaders)


You will have to adjust the extension as the argument indexes do not match anymore now.

yep, wanted some feedback on the general approach before i adjust DI and the tests.

Toflar · 2018-03-27T06:57:37Z

src/EventListener/SessionListener.php

 * of your application.
 *
+ * @internal
+ *
 * @author Yanick Witschi <yanick.witschi@terminal42.ch>


Your name is missing :)

Toflar · 2018-03-27T06:58:33Z

src/EventListener/SessionListener.php

@@ -67,15 +82,30 @@ public function onKernelResponse(FilterResponseEvent $event)
            return;
        }

+        // copied from the Symfony SessionListener, to avoid breaking the lazyness


Tests are missing on purpose I suppose?

emodric · 2018-03-27T07:16:53Z

src/EventListener/SessionListener.php

+        ) {
+            $session->save();
+
+            if ($session instanceof Session && $session->hasBeenStarted()) {


Shouldn't we return always if Vary header check is statisfied? It seems to me that with this, if the check is satisfied, but for anomymous users without session, the original listener still gets called, right?

that is on purpose. i want to interfere as little as possible with the default behaviour. the only case where calling the parent after having done $session->save() would result in a problem is if the hasBeenStarted flag is present - that seems to try to work around the save and still detect if a session was open and has been saved...

emodric · 2018-03-27T07:19:24Z

I guess we should check for Sf 4.1 and save the sesion only in that case?

Toflar · 2018-03-27T07:20:26Z

I guess we should check for Sf 4.1 and save the sesion only in that case?

Doesn't hurt. In 3.4 and 4.0 there's a SessionSaveListener that does nothing else than just saving the session which does nothing in case it was already saved :)

Toflar · 2018-03-27T08:00:30Z

As much as I agree with @nicolas-grekas that Symfony should set the response to private by default if the session is started, I think it's way too hard to disable this behaviour. Maybe a simple second boolean parameter in that listener that is set to true by default would be enough so we could just set that to false instead of decorating etc.?

emodric · 2018-03-27T08:06:50Z

@Toflar Yep, agreed. This already looks like a wild goose chase. They change something, we fix it, and so on and so on.

Toflar · 2018-03-27T08:25:33Z

Or even more flexible like this:

if ($session->isStarted() || ($session instanceof Session && $session->hasBeenStarted())) {
    $response = $event->getResponse();

    if ($response->headers->has('Symfony-NoPrivateIfSessionStarted')) {
        $response->headers->remove('Symfony-NoPrivateIfSessionStarted');
    } else {
        $response
            ->setPrivate()
            ->setMaxAge(0)
            ->headers->addCacheControlDirective('must-revalidate');
     }
}

Because there are multiple setups out there that wish to be able to disable this feature without having to do heavy lifting code. That one would easily solve all the issues.

Toflar · 2018-03-27T09:02:23Z

PR is ready at symfony/symfony#26681

nicolas-grekas · 2018-03-27T11:46:59Z

But any new feature on Symfony would not be applicable to this bundle, unless you plan to make it support Symfony 4.1 only.

Toflar · 2018-03-27T11:48:10Z

User context would be 4.1+ only. And my PR only disables the auto-private-thing. It leaves everything else as is.

dbu · 2018-03-27T15:16:17Z

The solution we merged and tagged a few days ago works for symfony 3.4 and 4.0, but will break session saving with symfony 4.1 because the SaveSessionListener is no longer a separate step. We did not call the inner listener because it used to only overwrite the cache headers to private - now we need a different solution and Session::hasBeenStarted does not make it easier...

But lets discuss on symfony/symfony#26681 first and then see what remains to be done here.

Toflar · 2018-03-27T15:18:08Z

Yeah this listener is really causing trouble. A per response option to disable it would solve all our problems.

…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

adjust to session listener changes in symfony 4.1

3c4c232

dbu mentioned this pull request Mar 27, 2018

Decorate default Symfony session listener for Symfony 3.4+ #435

Merged

Toflar requested changes Mar 27, 2018

View reviewed changes

emodric reviewed Mar 27, 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

dbu mentioned this pull request Apr 5, 2018

adjust session listener to symfony 4.1 #439

Merged

dbu closed this Apr 14, 2018

dbu deleted the session-listener-4.1 branch April 14, 2018 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adjust to session listener changes in symfony 4.1 #438

adjust to session listener changes in symfony 4.1 #438

dbu commented Mar 27, 2018 •

edited

Loading

dbu commented Mar 27, 2018

Toflar Mar 27, 2018

dbu Mar 27, 2018

Toflar Mar 27, 2018

Toflar Mar 27, 2018

emodric Mar 27, 2018

dbu Mar 27, 2018

emodric commented Mar 27, 2018

Toflar commented Mar 27, 2018 •

edited

Loading

Toflar commented Mar 27, 2018

emodric commented Mar 27, 2018

Toflar commented Mar 27, 2018

Toflar commented Mar 27, 2018

nicolas-grekas commented Mar 27, 2018

Toflar commented Mar 27, 2018

dbu commented Mar 27, 2018

Toflar commented Mar 27, 2018

adjust to session listener changes in symfony 4.1 #438

adjust to session listener changes in symfony 4.1 #438

Conversation

dbu commented Mar 27, 2018 • edited Loading

dbu commented Mar 27, 2018

Toflar Mar 27, 2018

Choose a reason for hiding this comment

dbu Mar 27, 2018

Choose a reason for hiding this comment

Toflar Mar 27, 2018

Choose a reason for hiding this comment

Toflar Mar 27, 2018

Choose a reason for hiding this comment

emodric Mar 27, 2018

Choose a reason for hiding this comment

dbu Mar 27, 2018

Choose a reason for hiding this comment

emodric commented Mar 27, 2018

Toflar commented Mar 27, 2018 • edited Loading

Toflar commented Mar 27, 2018

emodric commented Mar 27, 2018

Toflar commented Mar 27, 2018

Toflar commented Mar 27, 2018

nicolas-grekas commented Mar 27, 2018

Toflar commented Mar 27, 2018

dbu commented Mar 27, 2018

Toflar commented Mar 27, 2018

dbu commented Mar 27, 2018 •

edited

Loading

Toflar commented Mar 27, 2018 •

edited

Loading