search

lorisleiva / laravel-actions

⚡️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.52k stars 125 forks source link

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

Closed jaalt007 closed 2 years ago

jaalt007 commented 2 years ago

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: https://github.com/laravel/framework/commit/6cdf03e661a2a8b9b4ea2cc7d503e1d6a09bf5dc#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 commented 2 years ago

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 commented 2 years ago

😢 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 commented 2 years ago

Was issue created at laravel/framework ?

lorisleiva commented 2 years ago

Yes, in this PR: https://github.com/laravel/framework/pull/44516

pm-consultancy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

pm-consultancy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/laravel/framework/pull/44590

lorisleiva commented 2 years ago

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 commented 2 years ago

Fix merged and released as v2.4.1. 🚀

Let me know if you have any more issues. 🙏