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

[koenhoeijmakers]

This PR is a less intrusive submission of #40306

Summary of what this intends to fix:

Laravel offers the option to have middleware definitions in the controller constructor via the ->middleware() method.
This feature constructs the controller before the middleware are ran and we are therefore unable to inject classes that are bound in a middleware or of which the driver is selected in the middleware (Guard implementation for example See an example here).

Previous PR was super intrusive as it changed the current behavior for everyone, this PR aims to keep current behavior as is, but fixes the issue that me and others are running into.

The only downside is a new interface method, but i would argue that this is an interface that is rarely overridden.

How it works

Previously a controller was constructed and then passed to the ControllerDispatcher@getMiddleware() method to retrieve the middleware defined in the dispatched controller. In there a method_exists resides to make sure the given Controller has a getMiddleware method.
This PR moves the method_exists to a method on the ControllerDispatcher and verifies if middlewares should be collected. If so it does what it always did. If not then the instantiation is delayed until controller method is ran, because of this the middlewares are constructed first, and then the Controller. Instead of the other way around.

ping @X-Coder264

[taylorotwell]

Can you describe what this actually does? You talk a lot about the previous PR but not much about this one.

[koenhoeijmakers]

Can you describe what this actually does? You talk a lot about the previous PR but not much about this one.

Sure, updated the description.

[taylorotwell]

I guess I don't understand what this is solving. What does this allow people to do? If I don't want my controller to define middleware after being constructed, what do I actually do in my code? Can you give examples of real code please?

[koenhoeijmakers]

Alright I see, it all boils down to the following:

Take this example, it has a Guard injected in the constructor and a Guard injected in the method which is __invoke in our case.

Route::get('foo', ApiController::class)->middleware('auth:api'); // where the `api` Guard is the Passport guard

use Illuminate\Contracts\Auth\Guard;

class ApiController
{
    public function __construct(private Guard $guard)
    {
    }

    public function __invoke(Guard $methodInjectionGuard)
    {
        dd($this->guard); // unexpected behavior -> you get the 'web' guard
        dd($methodInjectionGuard); // you get the 'api' guard 
    }
}

With current behavior the constructor injected Guard is different then the method injected Guard. This is because the controller is instantiated before the middleware are ran. My change gives enables the option to prevent middleware collection from the controller by not having a getMiddleware method (which is usually defined by a base Controller).

Thus with this change both Guard instances would be the same, as the controller is only instantiated after the middlewares are ran.

[imanghafoori1]

1- It aims to the situations in which people do lazy container bindings app()->bind(...) in the handle method of their middleware and do __construct injection in their controllers. (to "Optimize" their application)

In other words, they do not want to bind in the register method, which is run for ALL requests.
They want to bind only when they are sure it is going to be needed down the road.

2- It also gives the opportunity to use multiple implementations for a single controller.

Example:

• Middleware binder_11:

public function handle($request, Closure $next)   // <------- the 'binder_11' middleware
{
    app()->bind(SomeInterface::class, MyClass11::class)

    return $next($request);
}

• other middleware:

public function handle($request, Closure $next)   // <------- the 'binder_22' middleware
{
    app()->bind(SomeInterface::class, MyClass22::class)

    return $next($request);
}

The Controller:

class ApiController
{
    function __construct(SomeInterface $obj) // <---- errors out, since app()->bind() is in middleware
    {
          // ...
    }

     // ...
}

Now, middlewares determine the injected class:

Route::get('/fooo', ApiController::class)->middleware('binder_11'); 
Route::get('/baar', ApiController::class)->middleware('binder_22'); 

Here if the user visits /fooo the MyClass11 is injected.

[X-Coder264]

The problems that this PR solves are described in #40306 (comment)

The difference with this PR as opposed to the last one is that the end users wouldn't have to change their controllers because the middleware registration syntax remains the same.

If users extend Illuminate\Routing\Controller they won't get the benefits of this fix (so basically for those users nothing changes compared to Laravel 8), but users who do not extend that controller won't have their controller instantiated anymore just for getting the middleware list so all problems described in that comment will be fixed for them. So it's basically a win-win situation, Laravel 8 users won't have to change their syntax, but they will have an option of switching the middleware registration from the controller constructor to the route definition so that they get the benefits of this PR.

[lorisleiva]

I know this has been merged but I am a bit concerned about this PR and there are a few points I'd like to mention.

• This PR makes the controller resolution change based on whether on not the getMiddleware method is implemented on that controller. I agree this shouldn't be a breaking change for users extending Illuminate\Routing\Controller but it makes understanding controller resolution confusing and unpredictable.
• My understanding of controllers was always that we need to use __construct for the registration of the controller and handle or __invoke for its actual logic. This is because once resolved, the controller is cached inside the Route it belongs to. So if we were to call that same endpoint twice within the same application lifetime, the __construct method would not be called again and the middleware would be resolved/executed after the controller was resolved. The changes in this PR scare me a bit because we now have a mixture of "registration logic" and "application logic" within the __construct method of controllers.
• It leads me to wonder if this PR break Octane applications as they will not rebuild the application from scratch at every request.
• Finally, I'd like to add that all of the issues addressed in the PR can be fixed if you simply inject in the controller handle or __invoke method instead of the __construct method whose purpose is to register the controller before it gets cached to its route.

[X-Coder264]

@lorisleiva Yes, there's now that condition upon which it depends whether the controller will be instantiated immediately during the fetching middleware phase or only when it's actually needed. This solution was accepted in the end because it does not have any breaking change as opposed to #40306.

If you are aware of any concrete scenario where this would break something please describe it in more details, I currently do not see such a case, the controller route caching logic remained the same as before.

* Finally, I'd like to add that all of the issues addressed in the PR can be fixed if you simply inject in the controller `handle` or `__invoke` method instead of the `__construct` method whose purpose is to register the controller before it gets cached to its route.

That is just the thing, __construct should be usable in the same way as __invoke can be used in regards to injecting dependencies. That's what users expect because that's how it also works in other frameworks (e.g. Symfony). Before this PR constructor injection would sometime work, sometimes not which is not good which is why this PR was made (since constructor injection was a Laravel feature even before this PR was merged so it had to be fixed to always work reliably - saying to use __invoke feels more like a bug workaround than an actual bug fix).

[koenhoeijmakers]

This PR makes the controller resolution change based on whether on not the getMiddleware method is implemented on that controller. I agree this shouldn't be a breaking change for users extending Illuminate\Routing\Controller but it makes understanding controller resolution confusing and unpredictable.

It is quite unfortunate that its based on the getMiddleware method indeed, but it was the easiest and least intrusive way of implementing, maybe an interface that defines getMiddleware which is added to the default Controller would make this difference more clear?

My understanding of controllers was always that we need to use __construct for the registration of the controller and handle or __invoke for its actual logic. This is because once resolved, the controller is cached inside the Route it belongs to. So if we were to call that same endpoint twice within the same application lifetime, the __construct method would not be called again and the middleware would be resolved/executed after the controller was resolved. The changes in this PR scare me a bit because we now have a mixture of "registration logic" and "application logic" within the __construct method of controllers.

I fail to see how a controller can be constructed twice in a single request lifecycle? And the controller was already being cached before, thus your notion of "the __construct not being called again" doesn't seem valid as it was only being called once before this PR anyways, it was just cached before middleware were ran VS after middleware being ran.

It leads me to wonder if this PR break Octane applications as they will not rebuild the application from scratch at every request.

As this follows up on the point above, i don't believe this will break any Octane applications as they would have already been broken in such case. (not 100% versed in octane so i could just as well be wrong here)

Finally, I'd like to add that all of the issues addressed in the PR can be fixed if you simply inject in the controller handle or __invoke method instead of the __construct method whose purpose is to register the controller before it gets cached to its route.

You could, indeed, except me and many others have been feeling weirdly about that since we're expecting injection to work like it does for any other class.

[lorisleiva]

Thanks for your reply guys.

I'm not saying that we were constructing the controller twice before the PR nor that we should. What I'm saying is that, by allowing us to write application logic within the __construct method, we create a framework in which running the same endpoint twice will potentially cause unpredictable behaviour.

Take a look at the following diagram.

Before, it was clear that __construct would only be executed once and would be used to register the controller in the framework before being cached on its route. Additionally, no matter how many time we call this endpoint, the behaviour stays the same — i.e. the controller is resolved before the middleware are executed.

Now, the first request has a different behaviour than the subsequent ones. Middleware will be executed before the controller is resolved in the first request but after the controller is resolved in the subsequent requests.

Take a look at the following example.

// "binder" middleware.
public function handle($request, Closure $next) 
{
    if ($request->get('status') === 'pending') {
         app()->bind(SomeInterface::class, SomePendingClass::class);
    } else {
         app()->bind(SomeInterface::class, SomeReadyClass::class);
    }

    return $next($request);
}

class ApiController
{
    function __construct(SomeInterface $obj)
    {
          // ...
    }
}

Route::get('/api/{status}', ApiController::class)->middleware('binder');

// GET /api/pending => $obj is instance of SomePendingClass::class.
// GET /api/ready => $obj is still an instance of SomePendingClass::class when application is kept alive.

I'm not saying this would have worked before this PR, but I'm worried that encouraging developers to add application logic and dependency injections within the __construct method and treating it like in a normal "action" class will cause more confusion because of the caching.

Finally, note that this is not about "code style". I also very much like the "construct + invoke" pattern in which the former registers dependencies and the latter executes the action. I just wanted to express my concern with regard to this change of behaviour which will likely cause unpredictability and confusion.

[koenhoeijmakers]

Thanks for the clear explanation, i can definitely understand what you're getting at and how this could be an issue in environments running with octane.

I suppose the only way to fix the issue you describe is to either prevent the controller caching, or remove the cached controller after it's dispatched. It seems the whole reason this caching exists anyways is to prevent double resolving said controller when middlewares are collected.

In my opinion this is a by-product of using coroutines and something that the developer has to keep in mind as this might just as well happen in user defined classes as well. But i'm all for preventing shipping the framework with said problem in its routing.

[stancl]

While I support the general change, perhaps specifically in the form of @X-Coder264's implementation, I agree with @lorisleiva.

In my opinion, the ideal behavior would:

• defer Controller __construct execution until after all middleware has been executed
• only instantiate controllers once

If you agree with those two points, then I think there's no implementation that'd work with middleware specified in __construct. It'd simply have to be moved to a completely different method, one that's accessible before the instantiation — therefore a static one.

This is also why I think @X-Coder264's implementation was the best one. 99.99%+ of users won't be affected in any way by changing $this->middleware() or public function getMiddleware() to public static function getMiddleware() — aside from making this one change in their controllers of course.

Perhaps that's the solution we could re-try for v10 if the Laravel team agrees that my two points above have merit.

I'd be interested in @lorisleiva's thoughts on that implementation as well, since he provided very useful context here.