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

[X-Coder264]

This PR is a result of my comment on @stancl PR -> #40165 (comment)

This PR wants to solve the problem of having to instantiate the controller just to get the defined middleware on it which in turn has all the problems described in that comment (it's not possible to fully and reliably rely on controller constructor dependency injection etc.).

Instead of completely removing this feature (as I originally suggested in my comment) I've opted to preserve that feature in order to minimize the breaking change while still fixing the problems described in that comment.

This is how the middleware definition would look after this PR (the example contains a string and Closure based middleware):

use Illuminate\Http\RedirectResponse;
use Illuminate\Http\Request;
use Illuminate\Routing\Controller;

class UserController extends Controller
{
-     public function __construct()
-     {
-        $this->middleware('auth')->except('fooMethod');
- 
-        $this->middleware(function (Request $request): RedirectResponse {
-            return new RedirectResponse('https://www.foo.com');
-        });
-     }

+     public static function getMiddleware(): array
+     {
+        static::middleware('auth')->except('fooMethod');
+
+        static::middleware(function (Request $request): RedirectResponse {
+            return new RedirectResponse('https://www.foo.com');
+        });
+
+        return parent::getMiddleware();
+     }
}

I believe such a small breaking change is definitively worth it in order to decouple the middleware and controller layer (all the pros are described in my linked comment).

The only other purely technical breaking changes (which I don't expect to realistically impact anyone) are:

1. the controller dispatcher getMiddleware method no longer receives the controller object instance, it now receives the controller class name (string), I've added the typehints there to ensure that which in itself is a tiny BC
2. \Illuminate\Routing\Route::isControllerAction method was changed from protected to public visibility
3. The middleware related properties and methods in Illuminate/Routing/Controller are now static.

[stancl]

I think this change would also allow specifying middleware using attributes as I suggested.

The userland implementation would look like this:

class MyMiddleware
{
    public function handle($request, $next)
    {
        return $next($request);
    }
}

#[Attribute(Attribute::TARGET_CLASS)]
class Middleware
{
    public function __construct(
        protected array|string $middleware,
        protected array|string $only = [],
        protected array|string $except = [],
    ) {}
    
    public function toArray(): array
    {
        $options = [
            ... $this->only ? ['only' => $this->only] : [],
            ... $this->except ? ['except' => $this->except] : [],
        ];
        
        return [
            'middleware' => $this->middleware,
            'options' => $options,
        ];
    }
}


#[Middleware(MyMiddleware::class, except: 'index')]
class MyController
{
    public static function getMiddleware()
    {
        $reflection = new ReflectionClass(static::class);
	$attributes = $reflection->getAttributes(Middleware::class);
        
        if ($attributes) {
            return $attributes[0]->newInstance()->toArray();
        }
        
        return [];
    }
}

// ['middleware' => 'MyMiddleware', 'options' => ['except' => 'index']]
MyController::getMiddleware();

And if Laravel wanted to extend the support for PHP 8 attributes, it could use this code as the default for getMiddleware() in Illuminate\Routing\Controller. Perhaps default to this if static::$middleware is empty.

That would allow using both the standard syntax and attributes out of the box.

Also, I'd personally change the userland syntax you proposed. This:

class MyController
{
    public static function middleware()
    {
        return ['foo', 'bar'];
    }
}

is a lot better than getMiddleware() which does return parent::... at the end. Since middleware() isn't something you call anymore, it makes sense to change it into a "declarative" method that you return stuff from.

It could default to returning static::$middleware, which would let users specify middleware using 1) public array $middleware = [...]; 2) static::$middleware = [...] called from other static methods (that happen to get called at some point). So the class would retain a lot of flexibility, while still having very clean syntax.

The one thing that'd be made a bit harder by returning the array directly is the lack of the ->only() and ->except() methods. However, a good alternative could be passing the action name to the method itself:

public static function middleware(string $action = null): array
{
    if ($action !== 'fooMethod') {
        static::$middleware[] = 'auth';
    }

    static::$middleware[] = function (Request $request): RedirectResponse {
        return new RedirectResponse('https://www.foo.com');
    };

    return static::$middleware;
}

[taylorotwell]

No plans to do this at this time - don't want to put the breaking change on people because for 99% of Laravel devs it's most definitely not worth it IMO. They don't care about some edge case we need to make easier for a specific package.

[X-Coder264]

@taylorotwell This is IMO not an edge case (not even close) nor is it related to any specific package. The example I gave was just the most recent instance of when this came to bit me in the ass. There's a lot of other examples of this problem that I've personally ran into which do not require any 3rd party specific package.

Problematic use case 1 (with just Laravel + Laravel Passport, technically even Passport is not needed for this example, just having a web and api guards in the app):

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, $this->guard->user()); // unexpected behavior -> you get the 'web' guard, and null for the user instead of the 'api' guard and the User object instance
        dd($methodInjectionGuard, $methodInjectionGuard->user()); // you get the 'api' guard and the User object instance as expected
    }
}

In a real HTTP request (when we send the appropriate access token to this endpoint) the user is gonna be null, because the guard is actually gonna be the web guard which is the default guard in the Laravel skeleton. The expected result here was to get the Passport api guard because the auth middleware was supposed to set it as the default guard - unfortunately the controller is currently being instantiated too early just to get the middleware list so it got the wrong guard. This guard example is just one example into which I've run into multiple times in the past and is probably just one of many such cases.

This also unfortunately cannot be caught by tests because Passport::actingAs method would call app('auth')->shouldUse($guard); which sets the correct guard in the tests even before the test kernel handles the request (the same problem is in the the InteractsWithAuthentication trait actingAs method). So while the tests would be green and $this->guard->user() would return the configured user in the tests the code in a real world HTTP request would return null instead. Tests behaving differently than the app would actually behave with a real HTTP request is IMO a major problem which makes tests unreliable. This PR would fix the problem of getting different guards when doing constructor vs method injection, even though ideally the ->shouldUse($guard) method call would also be deleted from both methods to solve the problem of tests not behaving like a real world app.

Problematic use case 2:

Middleware can define container bindings (example from https://github.com/laravel-json-api/laravel/blob/v1.1.0/src/Http/Middleware/BootJsonApi.php#L70):

    public function __construct(Container $container, Repository $servers)
    {
        $this->container = $container;
        $this->servers = $servers;
    }

    public function handle($request, Closure $next, string $name)
    {
        $this->container->instance(
            Server::class,
            $server = $this->servers->server($name)
        );

        $this->container->instance(
            RouteContract::class,
            $route = new Route($this->container, $server, $request->route())
        );

        return $next($request);
    }

Due to the controller being instantiated before this middleware runs and registers these bindings I cannot typehint them in the controller constructor because I'd get a \Illuminate\Contracts\Container\BindingResolutionException exception when the container would try to instantiate the controller which I definitively wouldn't expect to happen.

In short I don't believe this to be an edge case as there's a lot of use cases that are affected by this problem (here are two more problematic scenarios, plus the one from the previous comment makes that three scenarios), some which cannot be detected even by tests so I definitively think that this needs to be fixed.

[stancl]

Yeah, I think this would be a good general change of the routing internals. Just to be clear, we've found that this isn't needed at all for my package now, it'd just make the code significantly simpler.

However, there are many other cases when the current logic is confusing. For instance, even sessions are a bit weird with the current implementation. They're started using middleware, which means that Laravel views interacting with the SC in middleware as a correct practice. But when devs try to do it, they encounter edge cases often. Therefore, it'd make sense to support this well for other middleware as well.

@taylorotwell Please re-consider the change. Even if my package doesn't need this, I still think this would be a good change. The current behavior really is pretty confusing for anyone who doesn't source dive the internals to see why the logic is limited the way it is. And I think for 99.999% of people, it's a syntax change at most. The PR doesn't remove any behavior, the only time it'd lead to real changes being needed is when people do $this->middleware($this->somethingDynamicFromTheControllerInstance()) which sounds like a very bad code practice anyway.

I think the correct understanding of middleware is that it's something that prepares the app before the route action is executed. And currently, the route action (controller) is half-executed. It just seems wrong that it's instantiated at all before the all middleware gets executed.

[X-Coder264]

@taylorotwell @driesvints Any feedback on the last two comments (which went into greater detail as to why this is needed) would be greatly appreciated.

The amount of positive feedback (13 thumbs up reactions and zero thumbs down so far) that this PR got shows that the rest of the community would also like to get this merged and that it's not just some edge case.

[driesvints]

I'm sorry guys but following Taylor's reply we won't be spending more time on this.

[X-Coder264]

@driesvints I must admit it's somewhat demoralizing in terms of contributing to the framework when you spend time on a PR (especially on its long description/answers where you describe a behavior that is very weird at best, and a serious bug at worst) and it gets closed for weird/misunderstood reasons (as the problem that this PR fixes is not an edge case nor is it related to any specific package which was the reason given for its closure) so I definitively expected some feedback from Taylor after further describing the problems/bugs that occur due to the current behavior.

Maybe I could've done a better job of describing the problem in my original post but it was long enough already IMO. But at least thanks for the reply.

[stancl]

@driesvints If more time won't be spent on this PR, would more time be spent on a bug report? See the first code example in this comment: #40306 (comment)

I think Taylor is viewing this as something related to a feature in my package, but @X-Coder264 brought up good points about where this looks like a real bug. The behavior is very confusing to anyone who doesn't source dive the routing codebase to find that things don't work like one would expect them to.

The change is very minor and non-breaking for 99.9% of apps. However, it's breaking enough to be impossible to address post-RC1. So it's just my opinion that it'd be worth taking a look at what @X-Coder264 wrote, to avoid having what's basically a bug in the routing codebase stuck for years.

Again, the only impact of this change is that a minority of users would change their syntax of defining middleware in controllers. That's it. And middleware would actually work like it's expected to.

[mabdullahsari]

@driesvints I agree with all of the statements above and can confirm I also git bitten by this "quirk" of the framework in the past.
I'm a constructor injection "type of guy" usually, but I always avoid it in controllers due to the exact reasons described in the comments.

[derekmd]

If the constructor behavior is fixed, I'd like a simpler userland API similar to Eloquent methods booting()/booted() that don't require calling parent::boot().

Without return parent::getMiddleware() and a hard typehint:

public static function bootMiddleware()
{
    static::middleware('auth')->except('fooMethod');

    static::middleware(fn () => redirect()->to('https://www.foo.com'));
}

which is about equal to the simplicity of just calling middleware() in the constructor.

[taylorotwell]

I'll open this up so I can read up on the discussion about it tomorrow - but we have to be clear we are talking about breaking behavior that has existed for many years and for many people without issue. Therefore, forcing thousands of developers around the world to change their controllers for this is a very big deal and something we must take very seriously.

[koenhoeijmakers]

I've looked into this a few times but withheld from creating a PR as i'd expect it wouldn't pass as there's a super small edge case that could possibly be breaking (edge case below).

In my opinion the best way to fix this is to prevent controller instantiation when middlewares are collected and the given controller does not have a getMiddleware() method.

You could probably just rewrite the Route@controllerMiddleware() method to include this method_exists check and this issue would probably be fixed.

Edge case:

It could be breaking when someone has overridden the ControllerDispatcher and has decided to return something else then an empty array in the ControllerDispatcher@getMiddlewares() method or rename the getMiddlewares() method on their controllers (why though?), but what's the chance of anyone doing that? Doesn't really sound like a real scenario to me.
(feel free to enlighten me on why i'm wrong here).

[taylorotwell]

I think this would get more consideration if an alternate way to define controller middleware without instantiating the controller could be introduced without breaking the old syntax. Then, we would only document the new way so that apps can gradually transition over time.

I mean, I guess one could argue such an alternate way does already exists - defining middleware on the route definition.

[X-Coder264]

@taylorotwell Thank you for reopening/reconsidering the PR.

I think this would get more consideration if an alternate way to define controller middleware without instantiating the controller could be introduced without breaking the old syntax.

I'm not sure what you mean, can you explain this a bit more? The old syntax requires the controller to be instantiated as the instance method $this->middleware() is being called so whatever syntax gets proposed in which the controller is not instantiated will be a breaking change.

[stancl]

@taylorotwell You could instantiate the controller just to grab its middleware and then throw away the instance. It'd then get re-instantiated when it's actually executed to serve the response.

This could still cause issues if some people were e.g. setting properties in the constructor based on the injected dependencies. But it'd add backwards compatibility for the 99% of people who only do simple $this->middleware('foo'). With Laravel 10 fully dropping the old syntax.

That said, I think there's not much point in adding "transitional" BC.

With Laravel 9 being a major version (and especially an LTS version), forcing people to adopt this small change is fine imo. There's no difference between L9 and L10 in this context, so if some change is appropriate in a major version, it'd be appropriate in both. Adding temporary BC to let people transition easier makes sense with frequent major versions, but since Laravel's release schedule is one major version per year, I'd just do a full switch to static syntax.

[X-Coder264]

You could instantiate the controller just to grab its middleware and then throw away the instance. It'd then get re-instantiated when it's actually executed to serve the response.

Yeah, I was actually looking into that yesterday so that there's no breaking change for end users. The controller instance is currently cached on the route object. If that caching were to be killed it should solve the first problem from #40306 (comment), but we would then be instantiating the controller twice on every request which is suboptimal (and the second problem/use-case from that comment would still not be solved) so that's why I haven't decided to PR that if there's a chance to get this PR merged instead.

[taylorotwell]

Well, it may makes sense to backwards compatibility to you because you don't have to hear the complaints of the thousands of developers affected. 😅

[taylorotwell]

I think if there is no transitional path forward I'm not willing to break applications for this. Sorry all.

[christhomas]

@taylorotwell I recently came into this problem and the PR #44192 explains the situation very well, if we can fix this I think it would make the middleware setup a lot more logical.

From what I understand after stepping through the code, is that the router middleware and controller middleware are gathered and executed. But to do this the controller needs to be instantiated and inside the controller constructor any controller middleware are inserted, so afterwards they can be gathered into one final list to execute.

But I think this is a mistake. If you have router middleware, they are not related to the controller. It means I can't adjust the container before the controller is instantiated. So my constructor dependencies are now broken with the incorrect configuration.

I think the correct solution is:

1. gather router middleware
2. trigger all router middleware
3. construct the controller
4. gather all controller middleware
5. trigger all controller middleware

That way router middleware execute at the expected point in the request lifecycle. I am not sure whether it was for historical reasons that the router middleware were gathered and triggered in the same stage as the controller, but it does expose this problem.

[taylorotwell]

@christhomas it's because in your suggestion algorithm the middleware priority is not respected.