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

laravel/framework 9.35+ action middleware not being resolved #199

Closed
jaalt007 opened this issue Oct 13, 2022 · 13 comments · Fixed by #203
Closed

laravel/framework 9.35+ action middleware not being resolved #199

jaalt007 opened this issue Oct 13, 2022 · 13 comments · Fixed by #203

Comments

@jaalt007
Copy link

Hello!

From laravel/framework 9.35+ actions run asController have a regression in behaviour.

Controller middleware is not being resolved properly, leading to things like route model binding not working.

Due to:
laravel/framework@6cdf03e#diff-efe2a07687b9b821bbd99ae38a9329253f032e6df04bdea02bba9303a2b2beadL1084-R1103

Current workaround: make actions extend Illuminate\Routing\Controller.

Not sure how best to proceed, I'm happy using the workaround mentioned above at the moment.

@JasperTey
Copy link

I can confirm the same on one of my projects where I'm using a few actions asController. Thanks for the suggested workaround in the meantime, it saved me some time hunting down the root cause.

@lorisleiva
Copy link
Owner

😢 Thanks for raising this! I don't think there's anything Laravel Actions can do aside from making a PR to Laravel to relax that last if statement.

@pm-consultancy
Copy link

Was issue created at laravel/framework ?

@lorisleiva
Copy link
Owner

Yes, in this PR: laravel/framework#44516

@pm-consultancy
Copy link

No, what I meant to say was. Was an issue logged with Laravel about this issue.
Something like 'Can use invokable classes without extending it with Controller'
or how to use HasMiddleware

@lorisleiva
Copy link
Owner

Oh sorry. No, not yet, I've been trying to fix it on Laravel Actions first.

I've been trying to hijack the middleware logic but I sadly end up running route middleware twice. See #200.

@lorisleiva
Copy link
Owner

I'm writing a PR for laravel/framework right now.

@pm-consultancy
Copy link

I've done some debugging on my own.

My conclusions

  1. Middleware will be detected with/without the extends Controller or implements hasMiddleware
  2. using the current __invoke method will fail the SubstituteBindings Middleware, it will see ...$arguments which it can't bind
  3. using the Route::get('route', [CLASS, 'asController]` will make everything work nicely

In short the __invoke is the problem

@lorisleiva
Copy link
Owner

That's not quite true. The ControllerDecorator updates the action property of the route so that SubstituteBindings can work. The issue introduced by Laravel 9.35 has nothing to do with this.

@pm-consultancy
Copy link

And I was so proud for finding this.
Anyway, for some reason with the laravel 9.35 the route paramters in asController don't work anymore

@lorisleiva
Copy link
Owner

I've created a PR in laravel/framework that would fix this if it is merged. 🤞

laravel/framework#44590

@lorisleiva
Copy link
Owner

PR merged on laravel/framework! 🎉 I still need to update the AsController trait so it includes an empty getMiddleware method for that second if statement to trigger. I'll do that as soon as I have a moment. Bear with me. 🌺

lorisleiva added a commit that referenced this issue Oct 20, 2022
* add more middleware tests

* fix AsJobSerializedTest

* add empty getMiddleware to trigger controller middleware

* Update composer.json

* Fix tests

* Try to downgrade pest

* Try to downgrade orchestra

* Update run-tests.yml

* Update composer.json

* Update run-tests.yml

* Update run-tests.yml

* Update composer.json
@lorisleiva
Copy link
Owner

Fix merged and released as v2.4.1. 🚀

Let me know if you have any more issues. 🙏

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 a pull request may close this issue.

4 participants