Skip to content
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.

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

Breaking change with 301 redirects and Symfony 3.2 #969

Closed
lstrojny opened this issue Aug 3, 2017 · 12 comments
Closed

Breaking change with 301 redirects and Symfony 3.2 #969

lstrojny opened this issue Aug 3, 2017 · 12 comments
Labels
Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality. State: Reviewing This item is being reviewed to determine if it should be accepted.
Milestone

Comments

@lstrojny
Copy link
Contributor

lstrojny commented Aug 3, 2017

Q A
Bug Report? yes
Feature Request? no
BC Break Report? yes
RFC? no
Imagine Bundle Version 1.7

Symfony 3.2 changes the default behavior for 301 redirects to no longer set cache headers (symfony/http-foundation@6802c88). Chrome for example caches 301 redirects when no specific cache header is specified leading to infinite loops.

Changing the redirect from 301 to 302, which is arguably also semantically more correct, fixes it.

@robfrawley robfrawley added Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality. State: Reviewing This item is being reviewed to determine if it should be accepted. labels Aug 3, 2017
@robfrawley robfrawley added this to the 1.9.0 milestone Aug 3, 2017
@makasim
Copy link
Collaborator

makasim commented Aug 3, 2017

I dont think it is correct. The cache image is generated and the new url must be used at all times. that's what 301 is for isn't it?

@makasim
Copy link
Collaborator

makasim commented Aug 3, 2017

the resolve controller should never ever be called once the cached image is created.

@robfrawley
Copy link
Collaborator

The use of a 301 has always been a widely discussed topic; is there a reason we haven't simply allowed people to configure their preferred redirect type (a 301 or 302)? There are use cases for calling the resolve controller every time.

That said, back to the original issue, I'm not sure a 302 is how we want to handle the default implementation; as stated, we don't want the resolve controller to be called once the cache is created. Do we need to assign additional headers now due to the Symfony change? I don't have time to check out the referenced PR ATM; I'll further investigate it when I have some time later.

@lstrojny
Copy link
Contributor Author

lstrojny commented Aug 4, 2017

@robfrawley one alternative would be to set no cache headers for the 301 redirect. The issue is that the redirect goes to the same URL and therefore Chrome users will have 301s cached and run into infinite redirect loops.

@makasim
Copy link
Collaborator

makasim commented Aug 4, 2017

Do I miss something? The urls are not the same. The controller's url has /resolve/ in its path.

@lstrojny
Copy link
Contributor Author

lstrojny commented Aug 4, 2017

@makasim depends on the configuration. We use the same URLs, which escalates the whole thing. We configure it so that /image/foo.jpg will use nginx to look up the format in a folder and if it’s not there fall back to the controller. We keep the URLs the same so that once the proper image is generated it can nicely live in the CDN under the proper URL and we can shortcut the whole controller dance once it is generated.

@makasim
Copy link
Collaborator

makasim commented Aug 4, 2017

@lstrojny I see. it has to be configurable than.

@lstrojny
Copy link
Contributor Author

lstrojny commented Aug 4, 2017

@makasim I wonder if configuring the status code is enough or if a ResponseFactory would be a good idea so that people can do crazy stuff if they need to.

@makasim
Copy link
Collaborator

makasim commented Aug 4, 2017

I've just realized that this is already possible. One can register a kernel response listener and do what ever they want there from changing the status only to replacing the whole response object with the new one.

If so, we need a tutorial that helps people.

@robfrawley
Copy link
Collaborator

Makes sense; we should add an example of doing so to the RST documentation. In the meantime, why not introduce a configuration option for the redirect response type: many others have asked about using one redirect over the other for ages, and adding this option would solve a simple and common use-case. The custom event listener/subscriber shouldn't be required to change the redirect type, IMHO, but it is a very powerful alternative for those that require even most advanced customization.

@lstrojny
Copy link
Contributor Author

lstrojny commented Aug 4, 2017

Made it a simple setting for now. If a user wants more one can still overload the controller action

@robfrawley
Copy link
Collaborator

Resolved via #970.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: Bug 🐞 This item involves a legitimate regression (bug) to existing functionality. State: Reviewing This item is being reviewed to determine if it should be accepted.
Projects
None yet
Development

No branches or pull requests

3 participants