[10.x] Run route middleware before controller instantiation #44192

rodrigopedra · 2022-09-18T05:57:00Z

Currently a controller based route resolves its associated controller instance before running its middleware, regardless if the middleware were defined in the route definition (let's refer to those as route middleware), or in the controller's constructor (let's refer to those as controller middleware).

For some users this behavior is not intuitive, as they expect the controller to be resolved after middleware are run, at least after route middleware are run.

This is particularly an issue when a controller's dependencies are injected through its constructor, which is a common OOP pattern.

If a user writes a middleware where they expect to tweak the container to affect a controller's dependency resolution, these changes are not applied as expected due to the controller instance was already resolved from the container and bound to the route.

For example assume this controller:

class AController {
    public function __constructor(
        private readonly ApiClient $client,
    ) {}

    public function __invoke() {
        return $this->client->run();
    }
}

with this dependency:

class ApiClient {
    public function __constructor(
        private readonly string $apiKey,
    ) {}

    // ...
}

And this middleware:

class AMiddleware {
    public function ($request, $next) {
        app()->when(ApiClient::class)
            ->needs('$apiKey')
            ->give(function () use ($request) {
                if ($request->is('admin/*') return '1234';
                else return '4567';
            });

        return $next($request);
    }
}

and finally these route definition:

Route::get('/dashboard', AController::class)->middleware(AMiddleware::class);
Route::get('/admin/dashboard', AController::class)->middleware(AMiddleware::class);

When running these routes, as the controller is resolved before the middleware are run, the container would not be able to resolve the ApiClient instance, or at least it could give a wrong $apiKey value, in case a default value was bound in a service provider.

Prior attempts

There were some prior attempts to handle this, some recent are:

[9.x] Add resolveMiddleware() method to Router #40165 (which during review narrowed scope)
[9.x] Do not instantiate the controller just for fetching the middleware #40306
[9.x] Do not instantiate the controller just for fetching the middleware #40397 (reverted in [9.x] Revert changes to controller middleware #40675)

The difference in this PR implementation is that it tries to keep changes needed by an application to a minimum. Prior PRs attempted to add a dedicated static method to a controller to gather middleware, which would require code changes for many apps that declare middleware on their constructor.

Also a static method won't allow closure-based middleware to interact with the controller instance. Although I don't follow this practice, this was one of the reasons closure-based middleware were introduced back in 2016 to accommodate the fact the session would not be available to the controller's constructor anymore.

Actually, I believe most apps won't need to change nothing, as I believe (total personal opinion, from personal experience) it is uncommon to mix route middleware with controller middleware.

Desired side affects

If any authentication, authorization or other "safeguard"/"firewall" middleware are defined as route middleware, the request will fail before any controller's dependency is resolved.

This PR:

Changes how the route pipeline is handled, by first handling route middleware, and then resolving the controller and handling its controller middleware
Resets the route's controller instance after the route is ran, to address a concern on controller reuse between requests in tests or octane, as explained here: [9.x] Revert changes to controller middleware #40675 (comment)
Adds relevant tests, most would not run without this PR proposed changes, and one (testMiddlewareRunsOnce) was added to ensure behavior won't change by mistake in the future

Breaking Changes

This PR was sent to master due to middleware priority order not being fully respected anymore.

Currently as all middleware are gathered and pre-processed together, the middleware are sorted, and deduplicated, to respect the middleware priority as a single stack.

This PR breaks middleware processing in two steps: route middleware (including global and group middleware) are gathered together, sorted, deduplicated before the request is dispatched.

Then the controller middleware are gathered, sorted, deduplicated (including any middleware already processed) and the request is ran trough these remaining middleware.

By breaking this process into two steps, a middleware defined in a controller's constructor that should have a higher priority will be executed after any middleware defined globally, as a group or as route middleware that should have a lower priority.

This is shown in the testControllerMiddlewareIsRanAfterRouteMiddleware test.

I believe this can be documented in the upgrade guide. This would be, as far as I know, the only downside of the approach proposed by this PR.

One last note: the pipeline is broke into two pipelines, but the controller instantiation happens on the end of the first pipeline inside its ->then() method. So even by breaking into two pipelines all middleware that depends on the response will be handled as usual.

Epilogue

I want to thanks @christhomas for the suggestion on handling middleware by sets on this comment #44177 (comment), @X-Coder264 for the discussion on issue #44177 that inspired me writing this PR besides both of us having different opinion regarding this, and to @lorisleiva for the comments on previous attempts.

I would really appreciate if any of them could review or add any suggestions or critics to improve this PR.

Although I don't think there is an issue to be fixed -- as all the issues described above could be solved by using method injection on a controller's method -- I understand different people might have different views on how software should be built and I thought on this proposal as a middle ground solution to accommodate both views.

X-Coder264

@rodrigopedra Thank you for investing your time into this. I really like this proposed solution. As far as I'm aware the BC break should be minimal (if even that in practice) and what's even better with this new "controller middleware" concept is that it gives more power to users as now the middleware feature is more modular so users can more precisely target when they want their middleware to execute.

Before this PR there was no "hook" that would run after the route middleware and before the controller action method and now this PR brings exactly that. This also means that in a standard Laravel application users could now always reliably access session data in their "controller middleware" as the start session middleware is a route middleware that would now run before the controller gets instantiated and the controller middleware is run.

rodrigopedra · 2022-09-19T01:37:11Z

Thanks @X-Coder264 for looking into this, really appreciate.

I didn't think on those benefits you mentioned at the time, but this is actually something that had bitten me before, specially when we expect the middleware to be run in a particular order and due to middleware priority they are ran in a different order.

Let's hope for the best, and if you have any suggestions to improve it, let me know =)

rodrigopedra · 2022-09-19T09:01:16Z

From the current middleware priority stack, the most likely point of struggle for users - with this PR implementation - would be the auth and throttle middleware behavior.

If they are defined on a controller's constructor, they will be executed after any of the other middleware defined as a route middleware, regardless of their priority:

framework/src/Illuminate/Foundation/Http/Kernel.php

Lines 93 to 103 in 3bbc0e7

    
           protected $middlewarePriority = [ 
        
               \Illuminate\Cookie\Middleware\EncryptCookies::class, 
        
               \Illuminate\Session\Middleware\StartSession::class, 
        
               \Illuminate\View\Middleware\ShareErrorsFromSession::class, 
        
               \Illuminate\Contracts\Auth\Middleware\AuthenticatesRequests::class, 
        
               \Illuminate\Routing\Middleware\ThrottleRequests::class, 
        
               \Illuminate\Routing\Middleware\ThrottleRequestsWithRedis::class, 
        
               \Illuminate\Contracts\Session\Middleware\AuthenticatesSessions::class, 
        
               \Illuminate\Routing\Middleware\SubstituteBindings::class, 
        
               \Illuminate\Auth\Middleware\Authorize::class, 
        
           ];

This can break applications using the AuthenticatesSessions - as it is generally added to the web stack on the HTTP Kernel - which would run before the auth middleware if the later is defined in a controller's constructor.

If we want to tackle this issue - allowing dependency injection on a controller's constructor - I can see these alternatives:

Document on the upgrade guide that any auth and throttle middleware should be moved to a route definition, in the case a user defines them on a controller's constructor to keep the current behavior.
Ditch this PR and consider an static method alternative - as proposed on previous PRs - that keeps the middleware stack as a single stack, so priority is not an issue.
This will require users to migrate from defining middleware on a constructor to this dedicated method, and potentially having to deal with any closure based middleware that access the controller instance.
And finally - the most radical one - disallowing defining middleware on a controller's constructor.
I personally prefer to define middleware on a controller's constructor - as I explained in issue Controller is constructed before route middleware are executed #44177 - but I now see the value on allowing constructor dependency injection to work as users might expect due to the Principle of least astonishment.
But this alternative would require the largest changes on apps that defines middleware in a controller's constructor extensively.

If this issue ends up not being worth the hassle, I suggest we should at least add to the docs a note warning that constructor dependency injection on a controller class might not work as expected.

rodrigopedra · 2022-09-19T16:55:13Z

Document on the upgrade guide that any auth and throttle middleware should be moved to a route definition, in the case a user defines them on a controller's constructor to keep the current behavior.

If this ends up being chosen, we can add some mechanism like the lazy loading prevention, which errors out when auth or throttle are defined as controller middleware, than can be disabled in production.

We can also consider an interface to mark such middleware and assess the middleware stack before the second pipeline for those kind of middleware.

christhomas · 2022-09-19T21:56:18Z

Wow, this is exactly what I wanted. I think it also makes more sense to split these two because really it's how you'd expect them to be, so it was confusing to me when I was looking at this to see them all lumped together. Thanks for this!!

DarkGhostHunter · 2022-09-20T00:31:39Z

As I understand:

Route middleware
Controller instanced
Controller middleware
Controller action hit.

deleugpn · 2022-09-21T07:41:35Z

As someone that has been side-stepping controller DI for years, I really dig this. It might be obvious, but I wanted to add a short quesiton just to make sure this is it: If this were to be merged, we would be able to declare \Illuminate\Http\Request as a dependency of the controller and it would be resolved after being filled with data, correct?

christhomas · 2022-09-21T12:51:53Z

@deleugpn from what I've seen in the laravel framework code, there is a request build before the middleware and constructor are called. So I think you can inject the Request into the constructor. But I wonder what parts of the request are not "ready" yet? Perhaps it's not a complete request until it's gone all the way through all the middlewares. I suppose you'd run into a problem if a middleware adjusted the request parameters, or did something else, then maybe you'd run into my problem, that I would have a correct object. But it was misconfigured because I asked for the object before I was truly ready for it.

rodrigopedra · 2022-09-21T13:00:32Z

as a dependency of the controller and it would be resolved after being filled with data, correct?

Correct. And any modifications -- such as adding attributes or merging input to the request instance -- done before the controller instantiation, i.e. on global, stack or route middleware, should be available to the injected instance within the controller's constructor, as the instance will be resolved after these middleware are run.

rodrigopedra · 2022-09-21T13:03:08Z

Modifications done by middleware declared in a constructor, will not be available yet. I mean inside the controller's constructor. If you assign the instance to a controller property, they might be available to a controller's method later as objects are saved as reference and the instance piped to the remaining middleware is the same.

rodrigopedra · 2022-09-21T13:19:16Z

For example, assume these middleware:

class FirstMiddleware
{
    public function handle(Request $request, \Closure $next)
    {
        $request->attributes->set('foo', 'bar');
        $request->merge(['baz' => true]);

        return $next($request);
    }
}

and

class SecondMiddleware
{
    public function handle(Request $request, \Closure $next)
    {
        $request->attributes->set('foo', 'qux');
        $request->merge(['baz' => false]);

        return $next($request);
    }
}

also this route definition:

Route::get('/a-route', MyController::class)->middleware(FirstMiddleware::class);

And finally this controller:

class MyController extends Controller
{
    private Request $request;
    private LoggerInterface $logger;

    public function __construct(Request $request, LoggerInterface $logger)
    {
        $this->middleware(SecondMiddleware::class);

        $this->request = $request;
        $this->logger = $logger;

        $logger->info('foo@construct', [$request->attributes->get('foo')]);
        $logger->info('baz@construct', [$request->input('baz')]);
    }

    public function __invoke()
    {
        $this->logger->info('foo@invoke', [$this->request->attributes->get('foo')]);
        $this->logger->info('baz@invoke', [$this->request->input('baz')]);

        return 'done';
    }
}

Log output before this PR:

[2022-09-21 13:12:53] local.INFO: foo@construct [null] 
[2022-09-21 13:12:53] local.INFO: baz@construct [null] 
[2022-09-21 13:12:53] local.INFO: foo@invoke ["qux"] 
[2022-09-21 13:12:53] local.INFO: baz@invoke [false]

Currently, as the constructor is resolved before any middleware are run, no modifications to the request instance are available within the controller's constructor.

While at __invoke only the modifications from the second middleware are seen as they overwrite the modifications done by the first middleware.

Log output after this PR (tested locally):

[2022-09-21 13:16:28] local.INFO: foo@construct ["bar"] 
[2022-09-21 13:16:28] local.INFO: baz@construct [true] 
[2022-09-21 13:16:28] local.INFO: foo@invoke ["qux"] 
[2022-09-21 13:16:28] local.INFO: baz@invoke [false]

If this PR is accepted, within the constructor any middleware ran before the controller is resolved are now reflected within its constructor. I guess this is what @christhomas expected it to be.

And as the request instance is assigned to a controller's property as a reference, the modifications done by the middleware defined at the constructor are reflected when finally executing the route within the __invoke method.

lorisleiva · 2022-09-21T17:22:47Z

Hi there 👋

I think this is a good attempt but, I'm afraid I have to disagree with this approach.

Route middleware should just be another way of registering middleware. I struggle to see why we should add cognitive resonance to developers by resolving route middleware differently than any other middleware.

I can foresee developers using route middleware with DI on their controller's constructors and then doing some refactoring so that one route middleware is registered in a more convenient place and struggling to understand why, suddenly, their controllers are failing.

As I said in the past, I disagree with the premise that there is even an issue with the current way middleware are resolved but, moving past that, I wouldn't tackle it this way.

I think you're right about selecting a subset of middleware that would behave that way but I think this should be a lot more explicit than that and should not depend on how the middleware is registered.

One solution could be to add a flag on the Middleware itself that decides whether or not it should resolve before the controller is instantiated. Making that flag default to false — i.e. resolving after the controller is instantiated — would ensure no breaking changes are being introduced as you'd need to opt-in for that new behaviour.

I hope this helps. 🍀

DarkGhostHunter · 2022-09-21T17:28:02Z

One solution could be to add a flag on the Middleware itself that decides whether or not it should resolve before the controller is instantiated.

This. I often fixed this problem by allowing a dependency to be resolved and let the middleware to set it up, or attach that dependency to the Request object itself. I can see myself doing this:

Route::get('important/thing', function () {
    // ...
})->unresolvedController();

This way the Router is instructed to not resolve the controller, and run the controller middleware last once resolved.

deleugpn · 2022-09-21T17:59:47Z

Route middleware should just be another way of registering middleware. I struggle to see why we should add cognitive resonance to developers by resolving route middleware differently than any other middleware.

As someone that never uses Controller Middleware, the issue this solve for me is the availability of Request Data at Controller construction time.

christhomas · 2022-09-21T18:42:07Z

@lorisleiva

If I attach middleware to the route, then I expect them to trigger when the route is executed and before the controller is executed. Hence it's "in the middle", between the router and the controller.

If I attach a middleware inside the controllers constructor (or as I've seen suggested, by php attributes), then I expect them to trigger after the controller is constructed, but before any work is done on the endpoint, so between the constructor and the method which handles the request.

So for me this is about managing expectations. My expectations for route middleware are that the middleware apply to the route, before the controller handles them. Because if I wanted the controller to handle them, I would just define them in the controller. Except that would become problematic if two controller methods are targetted by the different routes, but require different middleware.

What you said about there being a flag that you can add to middleware, well wouldn't attaching middleware to my route or controller be providing you with that information in a simple and easy to understand way. You don't need a flag to see whether I execute before or after the controller construction, if I simply just attach middleware to the router or put them inside the controller constructor, since where I define them, is effectively the flag.

What about a middleware which checks auth credentials, then rejects the request because there is not the correct credentials. Then I construct a controller with all it's dependencies, that could be a pretty heavy set of object constructions in large projects and then you reject the request and throw all those objects away because you abort 401. That doesn't make sense. Why waste resources?

christhomas · 2022-09-21T18:47:15Z

One solution could be to add a flag on the Middleware itself that decides whether or not it should resolve before the controller is instantiated.

This. I often fixed this problem by allowing a dependency to be resolved and let the middleware to set it up, or attach that dependency to the Request object itself. I can see myself doing this:
Route::get('important/thing', function () {
    // ...
})->unresolvedController();
This way the Router is instructed to not resolve the controller, and run the controller middleware last once resolved.

But then you run into the problem of "what does unresolvedController mean?" and why do I add middleware to the route, then tell the framework I want to trigger them before the constructor, when I added them to the route, which is a sure sign that I want them executed on the route, not the controller.

if I wanted them executed after the controller, I would just add them to the controller constructor.

So the problem with this solution is that it appears to say the same thing twice. You define the route on the middleware, then you tell the route an extra thing it already knows by having the middleware on the route.

So this idea of adding a flag, or an extra method call, just seems like you're adding complexity to a simple solution and not gaining anything from it.

I can't think of a single problem triggering router middleware before the constructor would cause, @lorisleiva said there was one they could imagine, but honestly, I'm trying to think of one and I can't. But there are tangible negatives to not running middleware before the constructor, as is mentioned and demonstrated very simply in the first comment of the PR.

So, considering the problems, this solves them, without any perceivable downside

rodrigopedra · 2022-09-21T22:00:45Z

@lorisleiva I actually share the same opinion. But after discussion on issue #44177 I see how a developer can be surprised for a controller's constructor not working as expected.

This PR's proposal is kind of a middle-ground solution.

~~I think I found a solution for the middleware priority issue, but it involves using reflection. I'll try to commit it later when I have some time to work on it.~~

~~It could even allow for "bubbling up" the middleware stack any middleware that should run before the controller is instantiated but is declared within a controller's constructor.~~

~~I might send it as an alternative PR, so both approaches can be assessed.~~

EDIT

I don't think it is worth it, or that I can solve it. I though on reflecting a constructor's parameters and providing instances with ReflectionClass@newInstanceWithoutConstructor and default primitives.

So I could create a "shallow" controller, grab its declared middleware, and discard this "shallow" instance.

But, if a middleware's options are defined as depending of a dependency's property or method return value, these "mock" dependencies would not provide the expected value.

And that is like making things worse than better.

An alternative -- I think it used to work like this before -- is instantiate the controller, grab all the middleware, drop that instance, and build it again as the final step when dispatching.

From memory I think this "double-instantiation" approach was removed as some users had costly to instantiate dependencies, or that performed side-affects such as logging, and they did not expect them to run twice.... But I am not sure, this was a long time ago.

At the end I think this is a good middle-ground solution that can be explained well in the docs. Middleware declared in the outer/definition layers (Kernel, route definition) are executed before reaching a controller. Middleware declared close to the execution layer (within a controller's constructor) are executed after a controller instance is resolved.

christhomas · 2022-09-22T18:29:54Z

In my case, grabbing a shallow controller to grab the middleware and then throw away wouldn't work, because if the object can't be constructed with the correct data, it'll correctly throw an exception because the object cannot be used without the configuration in place. I think the idea of instantiating the controller just to grab the middleware is in itself a red flag that you're doing something wrong.

Really, the correct approach is the one we're doing now. If I attach middleware to the router, then they execute before the router and after the correct route is known. That's the absolute correct approach. I can't imagine why you'd run into any problems, so I'm not sure what these potential issues could be, perhaps you can demonstrate a potential problem which would make it understandable?

rodrigopedra · 2022-09-22T21:55:41Z

I can't imagine why you'd run into any problems, so I'm not sure what these potential issues could be

The problems are the same as you are having now: things not working as one expect, or how it used to behave before the changes.

See I understand your surprise when you realized a controller's constructor didn't work as expected. I fully understand, that is why I sent this PR.

But on the other hand by changing how a middleware stack is prioritized can be an unexpected surprise. For example, for people used to declare middleware at a controller's constructor, they might declare the auth middleware there as well.

Before, due to middleware prioritization happening in a single stack, the auth middleware would "bubble up" the queue and be executed earlier in the queue. And always be execute before -- for example -- the SubstituteBindings middleware that potentially performs database queries to fetch model dependencies. If the auth fails early, then no database query would need to be performed.

Now with breaking the queue into two, this same auth middleware would be executed after SubstituteBindings which would allow database queries to be executed even for unauthenticated queries. This would surely break people expectations.

I understand your argument that one could expect middleware declared in a route to be executed at route level, and those declared in a controller to be executed in a later stage. And, again, this is why I proposed this PR.

But we have to take into account what people could be used too as this was the default behavior for many years. It is not a matter on what should be correct or not correct, but a matter on breaking behavior people are used too even if we think this behavior is "wrong" or should be corrected somehow. We have to take this into account.

I saw many PRs being reverted due to breaking how apps used to work. I believe this could be solved by documenting these changes on the upgrade guide. But nonetheless we have to bring this change into the discussion table, so we avoid a revert and a smooth transition.

if the object can't be constructed with the correct data, it'll correctly throw an exception because the object cannot be used without the configuration in place

This true. What I tried doing was creating a custom container's subclass that resolved dependencies with ReflectionClass@newinstancewithoutconstructor. Then changing the error and exception handlers while building a giving class (i.e. the controller).

This would allow the Controller@middleware method to be called without error.

As Controller@middleware accepts either a string, an array of strings, or a closure as its first parameter, that is all I needed -- at first thought -- to grab the middleware's names and use them when prioritizing the middleware queue. Closure based middleware would always be considered a controller middleware, as they can only be defined there.

The problem is when middleware options, or middleware parameters, are dependent from a constructor's dependency, then the issue you describe -- object can't be constructed with the correct data -- takes place, even if errors are temporarily suppressed, we won't be able to grab the correct options nor parameters.

An idea could be keeping the dependency dependent middleware always as an constructor middleware. But I think this would add another layer of overhead.

Documenting the potential behavior change this PR presents is yet a better "middle-ground" solution, is my opinion.

christhomas · 2022-09-23T07:30:13Z

Ah, i see what you mean now, it’s about peoples expectations they had before. Yeah you’re right, documenting this change and making people aware of this is the best solution! Thanks again for the PR, I really hope it’s going to be merged

…

On 22. Sep 2022 at 23:56 +0200, Rodrigo Pedra Brum ***@***.***>, wrote: > I can't imagine why you'd run into any problems, so I'm not sure what these potential issues could be The problems are the same as you are having now: things not working as one expect, or how it used to behave before the changes. See I understand your surprise when you realized a controller's constructor didn't work as expected. I fully understand, that is why I sent this PR. But on the other hand by changing how a middleware stack is prioritized can be an unexpected surprise. For example, for people used to declare middleware at a controller's constructor, they might declare the auth middleware there as well. Before, due to middleware prioritization happening in a single stack, the auth middleware would "bubble up" the queue and be executed earlier in the queue. And always be execute before -- for example -- the SubstituteBindings middleware that potentially performs database queries to fetch model dependencies. If the auth fails early, then no database query would need to be performed. Now with breaking the queue into two, this same auth middleware would be executed after SubstituteBindings which would allow database queries to be executed even for unauthenticated queries. This would surely break people expectations. I understand your argument that one could expect middleware declared in a route to be executed at route level, and those declared in a controller to be executed in a later stage. And, again, this is why I proposed this PR. But we have to take into account what people could be used too as this was the default behavior for many years. It is not a matter on what should be correct or not correct, but a matter on breaking behavior people are used too even if we think this behavior is "wrong" or should be corrected somehow. We have to take this into account. I saw many PRs being reverted due to breaking how apps used to work. I believe this could be solved by documenting these changes on the upgrade guide. But nonetheless we have to bring this change into the discussion table, so we avoid a revert and a smooth transition. > if the object can't be constructed with the correct data, it'll correctly throw an exception because the object cannot be used without the configuration in place This true. What I was trying to do was creating a custom container's dependency that resolved dependencies with ***@***.*** Then changing the error and exception handlers while building a giving class (i.e. the controller). This would allow the ***@***.*** method to be called without error. As ***@***.*** accepts either a string, an array of strings, or a closure as its first parameter, that is all I needed -- at first thought -- to grab the middleware's names and use them when prioritizing the middleware queue. The problem is when middleware options, or middleware parameters, are dependent from a constructor's dependency, then the issue you describe -- object can't be constructed with the correct data -- takes place, even if errors are temporarily suppressed, we won't be able to grab the correct options nor parameters. An idea could be keeping the dependency dependent middleware always as an constructor middleware. But I think this would add another layer of overhead. Documenting the potential behavior change this PR presents is yet a better "middle-ground" solution, is my opinion. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

taylorotwell · 2022-10-04T17:13:09Z

I personally am still not compelled to change this behavior. Laravel is 11 years old - and I just feel like a change in fundamental behavior at this point needs to be extremely warranted. And, right now, we have 3 thumbs up and 1 thumbs down on this PR.

It also seems trivial to work around - for example... you could inject a factory:

public function __construct(protected ApiClientFactory $factory)
{
}

public function __invoke(Request $request)
{
    return $this->factory->make($request)->handle();
}

Most applications seem to be getting by fine without this PR? I feel like we need much more compelling reasons to change the routing / middleware behavior.

X-Coder264 · 2022-10-04T17:30:46Z

#40306 got 17 thumbs up if that matters 😛 That PR tried to address the same thing but in a different way and was closed because no transitive way forward was provided. If that's still the case and if it would be merged with a transitive way forward I could try to invest some time into it. Otherwise this seems like a good compromise/middle ground solution IMO.

Creating factories does not seem like good DX tbh because it's additional work for the developer and because sometime you don't even know that you need a factory as not every user knows what all middleware do and which "side effects" they cause (for example in the guard scenario where the auth middleware is the one that is setting the guard and you don't know that you got the wrong guard in the controller).

christhomas · 2022-10-04T18:33:42Z

I don't think the factory method is a good suggestion, it means you inject an unrelated dependency just to acquire the dependency you want afterwards in a second step. So it's not expressive as a constructor dependency would be.

Fixing the problem of when middlewares are executed would instantly fix constructor injection without really giving any real world effects, I've yet to see a good example of how moving the execution of route middleware to before the constructor is called causing any problems.

I would just say that constructor injection is our ultimate best practice for dependency injection in php and there are no examples of side effects that anybody can think of, so why wouldn't we fix this issue? We fix bugs all the time. This one is very easy to document and very easy to grasp in terms of what it potentially changes for anybody who might run across a future issue.

So I think it's a good path forward.

The alternatives to this PR are all worse, they inject factories, or mysterious functions that need to be called on the routes, weird little triggers or tags that can be or can't be attached to affect some behaviour in a non-clear fashion, all to sidestep what is actually quite an understandable change.

Just execute the router middleware at the router stage of execution, and the controller middleware, at the controller stage of execution.

In the meanwhile, what I had to do was add dependencies to the controller methods themselves, which gets around the issue of needing to fix this issue, but this leads to all the controller methods full of dependencies. Which is also pretty ugly in practice.

JayBizzle · 2022-10-04T20:14:06Z

We've hit this issue loads of times over the years, always feels like a hack to get it working as expected. Would like to see this merged 🤞🏻

deleugpn · 2022-10-05T23:58:01Z

I feel like we could try to acomplish this by making a special interface that controllers implement to opt-in to the new behavior.

X-Coder264 · 2022-10-06T00:38:05Z

I feel like we could try to acomplish this by making a special interface that controllers implement to opt-in to the new behavior.

I was thinking more along the lines of a config option to be able to disable the "define middleware in the constructor" feature (the default being that the feature is enabled to preserve backwards compatibility). That way we would ensure that the controller is not instantiated before all middleware is run and devs who would want to opt-in to this behavior would not need to go and add a marker interface to every controller in the project. For new projects (e.g. in the Laravel skeleton app repo) we could set it as disabled by default immediately (the auth scaffolding controllers would probably need some adjustments first).

christhomas · 2022-10-06T08:13:46Z

I kind of think we're talking ourselves away from a clean and effective solution to the behaviour towards one which is muddy and possibly more complex and strange. Adding interfaces to controllers so the router knows to trigger middleware ahead of controller construction does sound like the cart before the horse.

I guess we are doing that because we are not sure what changing this behaviour would cause, but I think we need to test this in various applications to see what is happening and whether test suites are passing or not then knowing more information about what this change would make.

But I would recommend we try this out and see before we go into other solutions which don't really solve the problem, but work around it.

taylorotwell · 2022-10-06T15:49:56Z

@christhomas every time we have tried this it has had to be reverted.

moisish · 2022-10-07T06:21:22Z

I agree 100% on this statement @christhomas

“If I attach middleware to the route, then I expect them to trigger when the route is executed and before the controller is executed. Hence it's "in the middle", between the router and the controller.”

garygreen · 2022-10-07T18:04:03Z

I think a different PR attempted to fix this better: #40306

It's really an issue with HOW the middlewares are defined. By defining the middlewares in the construct Laravel HAS to construct the controller for it to know about those middlewares. So changing the way that works seems most logical to me.

This PR (#44192) feels like a halve-fix. It doesn't really solve the problem for anyone using controller middlewares, but does for those using route middlewares.

taylorotwell · 2022-10-08T02:25:45Z

Can I get feedback on this alternative that is similar to the idea the PR referenced by @garygreen takes.

#44516

I do not think it contains any breaking changes or changes to middleware priority / run order.

rodrigopedra · 2022-10-08T20:38:34Z

Closed in favor of #44516

run route middleware before controller instantiation

039718f

rodrigopedra mentioned this pull request Sep 18, 2022

Controller is constructed before route middleware are executed #44177

Closed

rodrigopedra added 2 commits September 18, 2022 03:03

styleci fixes

0e786f9

styleci fixes

d0732d1

X-Coder264 reviewed Sep 19, 2022

View reviewed changes

christhomas mentioned this pull request Sep 19, 2022

[9.x] Do not instantiate the controller just for fetching the middleware #40306

Closed

taylorotwell marked this pull request as draft October 4, 2022 17:14

taylorotwell mentioned this pull request Oct 8, 2022

[9.x] Controller middleware without resolving controller #44516

Merged

rodrigopedra closed this Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.x] Run route middleware before controller instantiation #44192

[10.x] Run route middleware before controller instantiation #44192

rodrigopedra commented Sep 18, 2022 •

edited

Loading

X-Coder264 left a comment

rodrigopedra commented Sep 19, 2022

rodrigopedra commented Sep 19, 2022

rodrigopedra commented Sep 19, 2022

christhomas commented Sep 19, 2022

DarkGhostHunter commented Sep 20, 2022 •

edited

Loading

deleugpn commented Sep 21, 2022

christhomas commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

lorisleiva commented Sep 21, 2022

DarkGhostHunter commented Sep 21, 2022 •

edited

Loading

deleugpn commented Sep 21, 2022

christhomas commented Sep 21, 2022

christhomas commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022 •

edited

Loading

christhomas commented Sep 22, 2022

rodrigopedra commented Sep 22, 2022 •

edited

Loading

christhomas commented Sep 23, 2022 via email

taylorotwell commented Oct 4, 2022 •

edited

Loading

X-Coder264 commented Oct 4, 2022

christhomas commented Oct 4, 2022 •

edited

Loading

JayBizzle commented Oct 4, 2022

deleugpn commented Oct 5, 2022

X-Coder264 commented Oct 6, 2022

christhomas commented Oct 6, 2022

taylorotwell commented Oct 6, 2022

moisish commented Oct 7, 2022

garygreen commented Oct 7, 2022

taylorotwell commented Oct 8, 2022 •

edited

Loading

rodrigopedra commented Oct 8, 2022

[10.x] Run route middleware before controller instantiation #44192

[10.x] Run route middleware before controller instantiation #44192

Conversation

rodrigopedra commented Sep 18, 2022 • edited Loading

Prior attempts

Desired side affects

This PR:

Breaking Changes

Epilogue

X-Coder264 left a comment

Choose a reason for hiding this comment

rodrigopedra commented Sep 19, 2022

rodrigopedra commented Sep 19, 2022

rodrigopedra commented Sep 19, 2022

christhomas commented Sep 19, 2022

DarkGhostHunter commented Sep 20, 2022 • edited Loading

deleugpn commented Sep 21, 2022

christhomas commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022

lorisleiva commented Sep 21, 2022

DarkGhostHunter commented Sep 21, 2022 • edited Loading

deleugpn commented Sep 21, 2022

christhomas commented Sep 21, 2022

christhomas commented Sep 21, 2022

rodrigopedra commented Sep 21, 2022 • edited Loading

christhomas commented Sep 22, 2022

rodrigopedra commented Sep 22, 2022 • edited Loading

christhomas commented Sep 23, 2022 via email

taylorotwell commented Oct 4, 2022 • edited Loading

X-Coder264 commented Oct 4, 2022

christhomas commented Oct 4, 2022 • edited Loading

JayBizzle commented Oct 4, 2022

deleugpn commented Oct 5, 2022

X-Coder264 commented Oct 6, 2022

christhomas commented Oct 6, 2022

taylorotwell commented Oct 6, 2022

moisish commented Oct 7, 2022

garygreen commented Oct 7, 2022

taylorotwell commented Oct 8, 2022 • edited Loading

rodrigopedra commented Oct 8, 2022

rodrigopedra commented Sep 18, 2022 •

edited

Loading

DarkGhostHunter commented Sep 20, 2022 •

edited

Loading

DarkGhostHunter commented Sep 21, 2022 •

edited

Loading

rodrigopedra commented Sep 21, 2022 •

edited

Loading

rodrigopedra commented Sep 22, 2022 •

edited

Loading

taylorotwell commented Oct 4, 2022 •

edited

Loading

christhomas commented Oct 4, 2022 •

edited

Loading

taylorotwell commented Oct 8, 2022 •

edited

Loading