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

[8.x] When following redirects, terminate each test request in proper order #35604

Merged

Conversation

inxilpro
Copy link
Contributor

The current followRedirects() implementation applies both a while loop and recursion, causing Kernel::terminate() to be called in the incorrect order. 99% of the time this is not an issue, but in the case of terminating middleware or callbacks set up with App::terminating(), the termination callbacks will be executed in reverse order.

For example, with two routes:

Route Middleware Response
/from LogRequest Redirect to /to
/to LogRequest return "OK"

And the following middleware:

class LogRequest
{
    public function handle($request, $next)
    {
        return $next($request);
    }

    public function terminate($request, $response)
    {
        // Log request path and response status code
    }
}

Given the following pseudo-test:

$this->followingRedirects()->get('from');
$this->assertEquals(['from', 'to'], get_logged_request_paths()); // <-- This fails before the PR

@GrahamCampbell
Copy link
Member

I think we should probably regard this as a breaking change, to make the behaviour better, rather than a bug fix. In which case, master is the right target branch for this.

@inxilpro
Copy link
Contributor Author

I was originally going to submit it to master, but this is definitely a bug and the current implementation still triggers all the same code, it just does it in the incorrect order.

I'm OK submitting it for 9.x if that's your call, but I'm fairly sure that this is more a bug fix than a breaking change.

@taylorotwell taylorotwell merged commit 2fb0f00 into laravel:8.x Dec 15, 2020
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