Skip to content

[5.6] Fix assertRedirect - #23176

Merged
taylorotwell merged 1 commit into
laravel:5.6from
adaroobi:fix-assertRedirect-retained-original-tests
Feb 15, 2018
Merged

taylorotwell merged 1 commit into
laravel:5.6from
adaroobi:fix-assertRedirect-retained-original-tests

Conversation

@adaroobi

Copy link
Copy Markdown
Contributor

This is a follow to #23170 :

assertRedirect doesn't behave correctly when asserting redirect routes since it retains the headers format while converting the given URI to URL if necessary.

but this retains the original tests.
However assertRedirect is operating an equal behavior to original tests and even better by asserting any redirect type, not just 301 one. Plus, accepting both urls and uris equally.

@Miguel-Serejo

Copy link
Copy Markdown

even better by asserting any redirect type, not just 301 one

Redirect routes are supposed to return a 301 code. Any other code would be an error.

@adaroobi

Copy link
Copy Markdown
Contributor Author

@36864 Sorry but, I disagree with you since 3xx codes are all redirect codes, and for example, default code for Illuminate\Routing\Redirector::to() method is 302.

@Miguel-Serejo

Copy link
Copy Markdown

Apologies, I should have been more explicit.

The tests are explicitly passing the status code 301 into the Route::redirect method. In this case, any status code other than the supplied status code should be considered an error.

@taylorotwell
taylorotwell merged commit a829ca7 into laravel:5.6 Feb 15, 2018
@adaroobi

Copy link
Copy Markdown
Contributor Author

@36864 No problem. Yes, exactly you're right.

@adaroobi
adaroobi deleted the fix-assertRedirect-retained-original-tests branch February 15, 2018 18:40
@GrahamCampbell GrahamCampbell changed the title Fix assertRedirect [5.6] Fix assertRedirect Feb 15, 2018
salmanarshad321 pushed a commit to salmanarshad321/framework that referenced this pull request Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants