adjust session listener to symfony 4.1 #439
Conversation
|
@Toflar would you have a moment to review this one? |
|
@emodric can you also check if this makes sense? |
|
@dbu Looks good to me 👍 Although, I was confused at first at why do you disable the original listener even on versions lower than Symfony 3.4 ( |
| } | ||
|
|
||
| // noop, see class description | ||
| // For Symfony 4.1+, we need to call the inner listener to save the session. Set the header to disable overwriting caching headers. | ||
| $event->getResponse()->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, 1); |
There was a problem hiding this comment.
I don't think this should be set here. I think we should not decorate the listener at all in Symfony 4.1+ but instead set that header where it belongs: The UserContextListener. Imho it can always be added, even for lower Symfony versions because the header doesn't harm at all if it is being sent to external but if you really want to omit that you can add the version_compare there.
So basically imho we should only register/decorate this listener when Symfony 3.4 or Symfony 4.0.
And then I think - for the sake of your time and stress level - you should really think about dropping < 4.1 support in the future 😄
There was a problem hiding this comment.
So basically imho we should only register/decorate this listener when Symfony 3.4 or Symfony 4.0.
Yep, this goes inline with my suggestion to keep the version check at one place. It can be further extended to only decorate on Sf 3.4 and 4.0.
And then I think - for the sake of your time and stress level - you should really think about dropping < 4.1 support in the future
I hope this won't happen before 3.0, though :)
|
thanks for the input. indeed, thats much better. i now add the header in the UserContextListener and the SessionListener is purely a workaround for SF 3.4 and 4.0. good like this, or any more input? |
|
Looks good! 👍 |
|
Perfect, yes 👍 Maybe we should add a unit test covering that case in the |
|
@dbu Is there a chance this might be backported to 1.x branch? Decorator for 3.4 and 4.0, and setting the header for 4.1, that is? |
|
got stuck trying to build that test... hope to have something soon. @emodric i'd rather not maintain the 1.x branch anymore. the 1.3 branch does not support symfony 4. if you really need the 1.3 branch with symfony 3.4 and can't upgrade to FOSHttpCacheBundle 2, i can merge a pull request on the 1.3 branch for this, if you port it. might also safe other people who are stuck on version 1 and update symfony to 3.4. |
|
Thanks! I'll create a PR with the decorator, I would need it for Symfony 3.4, yes. |
302673c to
e2943c9
Compare
e2943c9 to
2a95081
Compare
|
tagged as 2.2.1 |
Use symfony/symfony#26681 to make the session listener work with Symfony 4.1
Fix #437
Replaces #438