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

[12.x] Fix the issue of HasMiddleware compatibility #51058

Closed

Conversation

mahmoudmohamedramadan
Copy link
Contributor

@mahmoudmohamedramadan mahmoudmohamedramadan commented Apr 14, 2024

I found a Tip related to #44516 PR that introduced the HasMiddleware interface by @taylorotwell. I always prefer trying the code by myself and when I tried that interface I found no issue when we did not extend the BaseController but, if we do, we will hit a compatibility issue, therefore, both BaseController and HasMiddleware define the same middleware method name.

I love to use an expressive name, so I will suggest other names I think about too that may help (glueMiddleware, and injectMiddleware).

Important

The Laravel v.11 users will not feel that issue because every newly created controller does not extend the BaseController therefore, they will not hit that compatibility issue (Unless they do).

@driesvints driesvints changed the title Fix the issue of HasMiddleware compatibility [12.x] Fix the issue of HasMiddleware compatibility Apr 15, 2024
@taylorotwell
Copy link
Member

Honestly no idea what this is doing. 🙃

@mahmoudmohamedramadan
Copy link
Contributor Author

mahmoudmohamedramadan commented Apr 15, 2024

@taylorotwell I have updated the PR comment, I hope that clarifies it!

If you try to implement that interface that, you will hit a compatibility issue because the HasMiddleware interface and BaseController class have the middleware method with different type and parameters (Overloading) so, when you send an HTTP request you will get the next error :(

middleware-not-compatible

http-request-issue

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.

2 participants