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] Use parents to resolve middleware priority #39647

Merged

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Nov 17, 2021

Currently when middlewares are ordered it uses the class name and its interfaces for sorting but not it's parents.

This is a problem for example in Jetstream because:

  • Jetstream extends the AuthenticateSession middleware with it's own.
  • Since it doesn't have a contract it will run before any Authenticate middleware
  • Resulting in it's always being initialized via the default guard instead of the authenticated one
  • This prevents using two guards simultaniously (for example a user and an admin guard)

This PR fixes this behaviour by using parent classes for ordering so the Jetstream AuthenticateSession middleware will be in the correct place because it extends the Laravel AuthenticateSession middleware.

I also added test for the parent sorting and the already present interface sorting.

@taylorotwell taylorotwell merged commit cfab148 into laravel:8.x Nov 17, 2021
@driesvints
Copy link
Member

driesvints commented Nov 19, 2021

We're going to have to revert this: 404labfr/laravel-impersonate#145 & #39705

@netpok
Copy link
Contributor Author

netpok commented Nov 22, 2021

@driesvints I'm pretty sure the package has some bugs in it but probably it's best to revert it and I will target 9.x later if I have some time

@driesvints
Copy link
Member

@netpok as said in the other thread: it would be good if we could identify the bug itself so we know how this will impact others.

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