[9.x] Controller middleware without resolving controller

[taylorotwell]

This allows controller middleware to be resolved statically without instantiating the controller. Closes #44177

Alternative to #44192

AFAIK, this does not affect the order middleware are run and middleware priority is preserved. Therefore, there are NO breaking changes and I have submitted this to 9.x. This new behavior would be documented as the only way to define middleware on controllers; however, the prior way to define middleware would not be removed.

A new interface HasMiddleware and value object Middleware have been introduced:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use Illuminate\Routing\Controllers\HasMiddleware;
use Illuminate\Routing\Controllers\Middleware;

class TestController extends Controller implements HasMiddleware
{
    public static function resolveMiddleware()
    {
        return [
            (new Middleware('auth'))->only('index'),
        ];
    }

    public function index()
    {
        return 'Hello World';
    }
}

However, I REALLY do not like method name resolveMiddleware. I can't make it middleware because the base controller class already has that method defined non-statically. Does anyone have any ideas for better naming here?

@rodrigopedra @X-Coder264 @christhomas any thoughts on this?

[rodrigopedra]

@taylorotwell I like this PR better than the one I submitted for these reasons:

• It keeps the current behavior, which has been available for a decade now -- while allowing the alternative behavior
• It keeps the middleware stack as a single stack, thus not messing with middleware priority

I proposed PR #44192 as a middle-ground solution, but that could imply behavior changes on a part of the framework that is very sensible to changes, as we saw back in 5.3 when auth/session data was not available anymore on the constructor and closure middleware were introduced to patch that behavior change.

On the other hand -- sometimes we only think about a solutions after we see a different approach -- I think we could have a better opportunity here.

Currently the middleware instance method is defined at the Illuminate\Routing\Controller class. So all controllers declaring middleware in their constructor are extending this class.

What if, instead of testing for the new HasMiddleware interface, we test if the controller extends Illuminate\Routing\Controller and thus keep the current behavior, otherwise we defer the controller instantiation to after all middleware are processed?

Doing this we:

• Would avoid any breaking changes as the current behavior is unchanged
• The current behavior is actually the opt-in one, as it needs to extend the base class above
• Who expects a controller's constructor to behave differently would only have to skip extending the class above
• We wouldn't need to allow middleware to be declared from a static method if we don't want to. A developer would actually need to choose a trade-off between declaring middleware on a controller's constructor, or having its constructor's dependencies resolved after middleware are processed
• If static middleware are NOT an option, your method naming dilemma is solved as a bonus
• If static middleware ARE an option, then we could keep the HasMiddleware interface, have the static method simply named middleware, and only test for its implementation after testing if Illuminate\Routing\Controller is extended, thus allowing a escape hatch for those who, like me, prefer to keep their middleware in theirs controllers

Illuminate\Routing\Controller also declares two other methods not related to middleware: callAction(), and the magic __call() method.

We could move these methods to a trait, and make the base Illuminate\Routing\Controller class use it by default. This would allow a developer who wants those methods available to use this trait and skip extending the controller.

I would actually only move callAction() to this new trait, as __call()'s current implementation is similar to PHP's default behavior anyway, and to my understanding is there to allow implementing fallback methods, which could be implemented without having a base method to override.

https://github.com/laravel/laravel/ also provides a base Controller class, that in addition of extending Illuminate\Routing\Controller, make use of 3 other traits.

Not extending Illuminate\Routing\Controller and manually adding these traits -- or only the ones actually used by a controller -- would be the only work left to be done by those who want the new allowed behavior, which is reasonable as they are opting-in to a new behavior that breaks the current available behavior.

Hope this makes sense. Let me know if something is not clear enough.

[rodrigopedra]

Maybe it is better explained with code:

public function controllerMiddleware()
{
    if (! $this->isControllerAction()) {
        return [];
    }

    [$controllerClass, $controllerMethod] = [
        $this->getControllerClass(),
        $this->getControllerMethod(),
    ];


    // first we check if the controller extends base controller if so we
    // then keep things as they current are, without breaking changes
    if (is_a($controllerClass, Illuminate\Routing\Controller::class, true)) {
        return $this->controllerDispatcher()->getMiddleware(
            $this->getController(),
            $controllerMethod,
        );
    }

    // OPTIONAL: then we check if the controller implements the new interface,
    // if so we gather this controller's middleware from its static method
    if (is_a($controllerClass, HasMiddleware::class, true)) {
        return $this->staticallyProvidedControllerMiddleware(
            $controllerClass,
            $controllerMethod,
        );
    }

    // else, we don't need to bother as no middleware could be provided
    return [];
}

[X-Coder264]

I agree with @rodrigopedra with one small adjustment to his code snippet:

    // first we check if the controller implements the new interface,
    // if so we gather this controller's middleware from its static method
    if (is_a($controllerClass, HasMiddleware::class, true)) {
        return $this->staticallyProvidedControllerMiddleware(
            $controllerClass,
            $controllerMethod,
        );
    }

    // then we check if the controller extends base controller
    // if so we keep things as they currently are, without breaking changes
    if (is_a($controllerClass, Illuminate\Routing\Controller::class, true)) {
        return $this->controllerDispatcher()->getMiddleware(
            $this->getController(),
            $controllerMethod,
        );
    }

    // else, we don't need to bother as no middleware could be provided
    return [];

This way even controllers that extend the base controller can get marked with that new interface which enables them to use the new syntax as it'd have priority over the non static Illuminate\Routing\Controller syntax. From a framework perspective this is beneficial because it gives users an easy upgrade path so over time if we want to we could deprecate and remove the old syntax (for Laravel 12 or something like that).

The benefit of doing these two checks (compared to the current state of this PR) is that I wouldn't have to change every single controller in my projects, having them implement HasMiddleware (which would always return an empty array) just to disable the behavior of instantiating the controller before the middleware is run.

[Jubeki]

Some other names for resolveMiddleware (maybe as inspiration):

• withMiddleware
• prepareMiddleware
• usesMiddleware
• defineMiddleware
• middlewareDefinitions
• controllerMiddleware

[deleugpn]

I just can't figure out how this change allow us to have Request $request in controller constructor?

---

Edit: reading through @rodrigopedra comment, I can totally see how it could indeed be better to tie Controller middleware with \Illuminate\Routing\Controller as that has always been the API offered by Laravel. I would actually prefer not having the HasMiddleware interface at all (which benefits Taylor with the naming issue) and make it so that controllers extending the base controller will never have constructor dependency completely fulfilled with request data while controllers without the base class could have it.

[taylorotwell]

@deleugpn can you explain your comment - this still works fine for me?

[taylorotwell]

@X-Coder264 I have made the change you suggested. 👍

[christhomas]

Maybe I'm misunderstanding something. But my problem was never about the controller middleware. I don't have controller middleware. I attached middleware to my router endpoints and it was causing a problem because the controller required construction before the router middleware triggered.

So I'm not sure whether this solves my problem.

[X-Coder264]

@christhomas The reason why your controller was constructed before the middleware ran was because controllerMiddleware method of the Route class (which has now been changed in this PR) had to instantiate the controller to get the controller level defined middleware. The new static syntax added by this PR enables to define middleware in the controller in such a way that the instantiation of the controller is no longer necessary.

So yes, this does solve your problem. If your controller does not extend the Illuminate\Routing\Controller class you won't have to change anything in your project after this PR gets merged and tagged and you update your framework version. If your controllers do extend that base controller class you'll have to implement the HasMiddleware interface and explicitly return an empty array there so that the controller does not get constructed during the middleware gathering phase.

[christhomas]

Yeah, I've just checked out the branch and done a simple test of using a middleware to change a config value and then inject into the constructor the dependency. So I can see that it does work.

But if I implement constructor middleware, like was posted as a potential problem, doesn't that mean everybody using constructor middleware now has to update their code to support this new way of working? Or does it just add another possible way to set middleware which doesn't upset the existing controller middleware method that people used in the past?

[X-Coder264]

That's already explained in the PR description:

Therefore, there are NO breaking changes and I have submitted this to 9.x. This new behavior would be documented as the only way to define middleware on controllers; however, the prior way to define middleware would not be removed.

[christhomas]

Sorry, it's late, I missed that. Yeah ok, so this is going to be the way people should do it in the future, but the old way continues to work but it's just undocumented now. I guess to be deprecated in the far future.

I think it's actually a smart solution. It's smaller and more compact. Thanks a lot guys!

[christhomas]

Am I being dumb here? I can't see how it's executing my middleware before constructing the controller, it gathers the router using middleware(), then the controllerMiddleware(), but I don't see how it's triggering my middleware before constructing the controller, but it does actually do that. What am I missing here?

[X-Coder264]

https://github.com/laravel/framework/blob/v9.34.0/src/Illuminate/Routing/Router.php#L714-L727

It gathers the middleware here and it runs them through the pipeline which executes them and when that's done it runs $route->run() which constructs the controller and executes the controller action method.

[taylorotwell]

I'm curious if we should even introduce the HasMiddleware interface at all? Maybe we should only make the most recent change I committed where the controller is only instantiated to retrieve middleware if the controller actually extends Illuminate\Routing\Controller?

Another option is to introduce the HasMiddleware interface, but update the skeleton to no longer extend Illuminate\Routing\Controller. Then, we can make the new interface use the middleware method name and update the documentation to reflect this new usage going forward. So, new applications would gradually move to this style.

[rodrigopedra]

EDIT after some thought I think the second option is better. It keeps an option for those who -- like me -- prefer to declare middleware on their controllers, to move past declaring them on the controller's constructor.

I'm curious if we should even introduce the HasMiddleware interface at all? Maybe we should only make the most recent change I committed where the controller is only instantiated to retrieve middleware if the controller actually extends Illuminate\Routing\Controller?

I like this idea best. Skipping adding HasMiddleware, and only checking for Illuminate\Routing\Controller sub-classes.

I would keep Illuminate\Routing\Controller, and the skeleton sub-class, as a convenience base class, as it also adds other features through traits, such as authorization.

It keeps familiarity with how the framework worked for almost a decade.

But I don't oppose moving to the new behavior by default -- in the skeleton, for new apps -- and making the current (old) behavior an opt-in.

Just to clear any contradiction, my suggestion is:

• Keep shipping the current skeleton base class -- including its traits
• Remove extending this base class from the controller's stub

Docs can state that extending that class is optional, and by doing that Laravel will add some convenience methods in exchange of method injection being preferred to avoid conflicts.

If this sounds too confusing for a patch release, 10.x is around the corner (I guess), and we can defer the skeleton changes for when it is released. This would also allows early-adopters to test for any conflicts.

[deleugpn]

sorry @taylorotwell for some reason I don't get email notification about metions anymore and I haven't been able to fix that yet.

My comment was exactly like yours here:

I'm curious if we should even introduce the HasMiddleware interface at all? Maybe we should only make the most recent change I committed where the controller is only instantiated to retrieve middleware if the controller actually extends Illuminate\Routing\Controller?

I think not adding the interface and using the base class as a way to identify whether to offer DI on the constructor of the controller would have been better.

As for your screenshot with dd($request), was the Request data already filled in or was it still empty?

[jakewisse]

I know this issue is a couple of years old now, but I wanted to share my experience as another data point around this conversation.

Right now I'm working on migrating a fairly large Lumen app to Laravel 5.7, and then on up the versions. I was first surprised by this behavior before beginning the migration, when I realized that in Lumen 5.7, global and route middleware are run before the controller is instantiated, while any middleware defined at construction time via $this->middleware() are run afterward (obviously). This app exposes a handful of REST APIs which utilize some sort of custom authentication middleware that sets tenancy state on a singleton. Most of these APIs add this middleware in the route definition under a group, but one of them does so in the constructor of a base controller class with $this->middleware(). I discovered this difference in behavior when I noticed that most of the app was not susceptible to bugs where the current tenant had not been resolved when a controller's dependency tree was built, but the API that used controller middleware was. Constructor injection is prevalent throughout the app, including in controllers.

Anyway, it's just funny that there's a strong argument around not changing this since it's been this way for over a decade... except in Lumen. 🤪 My solution at the moment is to backport this change and #44590 by extending a few core routing classes and rebinding them, until we can make our way from 5.7 to 9.