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

set correct http status code in redirect response #22497

Merged
merged 2 commits into from
Feb 18, 2016

Conversation

v1r0x
Copy link
Contributor

@v1r0x v1r0x commented Feb 18, 2016

Was intended to respond with a 303 (See other) status code instead of 307 (Temporary Redirect).

cc @BernhardPosselt (btw: thanks for the fix! ;))

Was intended to respond with a 303 (See other) status code instead of 307 (Temporary Redirect).
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @DeepDiver1975, @MorrisJobke and @BernhardPosselt to be potential reviewers

@nickvergessen
Copy link
Contributor

OCP\AppFramework\Http\RedirectResponseTest::testHeaders
Failed asserting that 303 matches expected 307.
/ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/tests/lib/appframework/http/RedirectResponseTest.php:47

You need to adjust the unit test as well,

when you did that, please set the label "To Review"

@BernhardPosselt
Copy link
Contributor

As discussed yesterday on IRC 👍 if unit tests are fixed.

Background: This response was intended to implement the Post/Redirect/Get pattern: https://en.wikipedia.org/wiki/Post/Redirect/Get

@DeepDiver1975 DeepDiver1975 added this to the 9.0-current milestone Feb 18, 2016
@nickvergessen
Copy link
Contributor

👍

@rullzer
Copy link
Contributor

rullzer commented Feb 18, 2016

Fine by me. 👍

DeepDiver1975 added a commit that referenced this pull request Feb 18, 2016
set correct http status code in redirect response
@DeepDiver1975 DeepDiver1975 merged commit 99d6a6b into master Feb 18, 2016
@DeepDiver1975 DeepDiver1975 deleted the fix-redirect-response branch February 18, 2016 13:24
@oparoz
Copy link
Contributor

oparoz commented Feb 21, 2016

This kind of breaking changes has to be advertised on the devel mailing lists.
It breaks tests for all apps which use the AppFramework and which have unit tests.

@nickvergessen
Copy link
Contributor

Well basically can happen with any function that is changed.

@oparoz
Copy link
Contributor

oparoz commented Feb 22, 2016

Yes and all such changes should be advertised. We're not talking about private APIs here.

@lock
Copy link

lock bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants