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

Use HTTP status code 303 instead of 307 for redirects #2097

Merged
merged 2 commits into from
Aug 6, 2020

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Aug 5, 2020

See #2096

@leofeyer leofeyer added the bug label Aug 5, 2020
@leofeyer leofeyer added this to the 4.9 milestone Aug 5, 2020
@leofeyer leofeyer requested a review from a team August 5, 2020 15:53
@leofeyer leofeyer self-assigned this Aug 5, 2020
@leofeyer leofeyer merged commit a522876 into 4.9 Aug 6, 2020
@leofeyer leofeyer deleted the fix/redirect-status-code-4.9 branch August 6, 2020 07:09
@fritzmg
Copy link
Contributor

fritzmg commented Aug 6, 2020

Why the switch back to 302 for any redirects that use RedirectResponse directly? Now we have a mixture of 302 and 303 (again).

@leofeyer
Copy link
Member Author

leofeyer commented Aug 6, 2020

We just stick to the Symfony defaults. See symfony/symfony#10766 for the discussion.

@fritzmg
Copy link
Contributor

fritzmg commented Aug 6, 2020

We just stick to the Symfony defaults.

But shouldn't we use it for Controller::redirect too then? Now some parts of Contao's code base return 302 and some return 303. That does not make much sense to me, nor does it make sense to stick with 302 imho.

And the Symfony discussion is from over 6 years ago. As discussed elsewhere, browser support is not an issue any more.

@leofeyer
Copy link
Member Author

leofeyer commented Aug 6, 2020

The bottom line from the Symfony discussion (and https://stackoverflow.com/questions/5129076/after-a-post-should-i-do-a-302-or-a-303-redirect/5129148#5129148) is that is does not matter whether you send 302 or 303. We have updated our own methods to use 303, Symfony has decided to stick with 302. It still does not matter. 🤷🏻‍♂️

(If it really bothers you, you should create a PR to adjust the Symfony default status code.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants