8000 PR #25583 and #25699 BC Break · Issue #26237 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

PR #25583 and #25699 BC Break #26237

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
smurfy opened this issue Feb 19, 2018 · 3 comments
Closed

PR #25583 and #25699 BC Break #26237

smurfy opened this issue Feb 19, 2018 · 3 comments

Comments

@smurfy
Copy link
smurfy commented Feb 19, 2018
Q A
Bug report? yes
Feature request? no
BC Break report? yes
Symfony version 3.4.3+

PR #25583 and #25699 overwrites cache-control even if i manually set them to public. This breaks compatibility. version 3.4.2 works.

I use varnish to cache and have several actions with explicit @Cache annotation to cache them by varnish, regardless if the user is logged in or not. This worked since 2.7 and till 3.4.2.

With the PR #25583 this broke, but was relatively easy to fix by overwriting the savesession listener.
With #25699 the change got moved to the abstract class which makes it even harder to overwrite.
Probably by modifying the session object but still, in my opinion its a BC Break.
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Feb 19, 2018 •
edited
Loading

Hello, thank you for your report.
This has been discussed previously in eg #25736: this is not a BC break, but a bug fix.
I invite you to review this discussion, it should give you the reasoning, and a way to force your logic, if you really want to.

@smurfy
Copy link
Author
smurfy commented Feb 19, 2018

@nicolas-grekas thanks for the info, i still see it somewhat as BC break, because it worked and then it broke :)

But i fixed it for myself by defining all urls which need the user object in my security.yml so the cache-control will be only changed for these urls, which is fine and wanted.

@nicolas-grekas
Copy link
Member

OK, all good then. Any bug fix is a BC break :)

@nicolas-grekas nicolas-grekas mentioned this issue Feb 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nicolas-grekas @smurfy
0