[9.x] Add resolveMiddleware() method to Router

[stancl]

Edit: The original description below is obsolete. We added support for the functionality in a different way.

---

This adds the ability to execute logic in route-level middleware before controllers are instantiated.

I'll try to explain the use case and implementation as well as possible, since I understand that routing is one of the more sensitive parts of Laravel where changes should be made with care.

Motivation

In v3 of Tenancy for Laravel, one of the biggest limitations our users deal with is that controllers which use constructor DI are incompatible with multi-tenancy.

This means that Fortify, Jetstream, and many other packages cannot be used out of the box.

The issue is that we're using route-level middleware for initializing tenancy (= switching the application state into a tenant context). In v2, we were using global middleware for initializing tenancy, which didn't have this problem. However, global middleware is very limited in functionality, and doesn't give any access to e.g. route parameters which are needed for features like like path-based tenant identification.

And the problem with route-level middleware is that it runs after controller constructors. This is because controllers can define middleware as well:

class UserController extends Controller
{
    public function __construct()
    {
        $this->middleware('auth');
    }
}

But this becomes an issue when the controllers inject services that would use a different state in the tenant context. For instance, injecting something like the Cache\Repository will make the controller use the central cache even if tenancy is initialized before the route action runs.

class UserController extends Controller
{
    public function __construct(
        public Repository $cache,
    ) {}

    public function show(User $user)
    {
        if ($this->cache->get('foo')) {
            // $this->cache uses a different cache store than cache()
        }
    }
}

I also described this in our documentation: Early identification.

In this PR, I'm adding functionality that lets route-defined middleware run some logic before the route action (controller) is resolved. This sort of adds a distinction between route-defined middleware and route-level middleware (route-defined + controller-defined).

Implementation

This adds a new middleware interface, PreparesApplication. The interface includes one method, prepareApplication(Route $route), which gets executed right before controller middleware is gathered. In other words, right before controllers are instantiated.

The middleware's handle() method is not affected in any way.

Tests

The PR includes detailed tests for this feature. In the second commit, I added a few to specifically assert that the middleware runs at the correct point in the request lifecycle, as I realized that the original implementation was executing the middleware way too early.

The tests could be simplified a bit — the PreparesApplicationController could just inject $app->make('prepares_application.context') instead of a service which depends on that — but I specifically went with a service to make it closer to the real world problem of injected services having incorrect state. And this is an integration test.

Limitations

I believe the current implementation doesn't support middleware groups/aliases. And it perhaps doesn't respect excludes and middleware order.

This is because the middleware() and gatherMiddleware() methods on Route are pretty low level. With the actual middleware classes being resolved in Router::gatherRouteMiddleware.

Route groups are supported.

If you think that the feature should support middleware groups too, I can try to implement this. I think it'd involve extracting some of the logic from Router:gatherRouteMiddleware() into a separate method, and calling that from the code I added.

[taylorotwell]

I'm a little confused regarding what you mean by:

I believe the current implementation doesn't support middleware groups/aliases. And it perhaps doesn't respect excludes and middleware order.

Are you saying a middleware that implements this new interface can't be part of a middleware group? Can you also elaborate and the excludes and middleware order portion of that sentence as well?

[taylorotwell]

As a follow up, should we perhaps explore resolving this situation with an event that is dispatched directly before controllers are instantiated? The event could contain the request as an available public property.

[stancl]

I'm a little confused regarding what you mean by [...]

My implementation changes what's returned from gatherMiddleware() which is called in $router->gatherRouteMiddleware().

The latter is where route groups are actually "decoded", and where logic like excluded is handled. Which means that if you used a middleware that implements my interface in a route group, and applied the group on a route like ->middleware(['tenant']) for instance, it wouldn't work.

As I said, completing this part is probably easy, but I wanted to wait for feedback before diving into that.

should we perhaps explore resolving this situation with an event that is dispatched directly before controllers are instantiated

I'm definitely open to alternative solutions. If this didn't get merged, I'd write a custom abstraction that runs some checks on the request to see if the logic should be executed.

However, the reason why I like middleware is that:

• it's route-specific
• it's composable — you can combine it with other middleware with different priorities
• it has route access

The event could contain the request as an available public property

Global middleware would work for requests already, but I'm trying to find a good solution that involves route-level features because the main point of this is accessing things directly on the route.

To make this less abstract and give you some concrete examples, in Tenancy for Laravel we have middleware such as:

• InitializeTenancyByDomain — this uses $request->getHost()
• InitializeTenancyBySubDomain — this uses $request->getHost()
• InitializeTenancyByDomainOrSubdomain — this uses $request->getHost()
• InitializeTenancyByPath — this uses $request->route(), fetches the first parameter (e.g. the team in /{team}/{project}/{issue}), uses it to identify the tenant, and forgets it so that the route actions don't have to accept it
• InitializeTenancyByRequestData — this checks $request headers

All of these need to run before the controllerMiddleware() call here. But for instance the path one needs to run after we have enough information about the route.

I agree that middleware intuitively feels a bit off for this when you look at it as essentially a pipeline that passes the request through handle() on the provided classes. I think the right way to decide about this is whether you think middleware should run side effects or prepare the application for the request in any way other than just changing $request or aborting.

If you think that it's a good responsibility for middleware, then finding a way to let it run logic prior to controller DI is good.

As I mentioned, being route-specific is one of the main value props here:

Route::view('/register', 'register-form');
Route::post('/register', function (Request $request) {
    $team = CreateTeam::run([ some data from $request ]);

    return redirect()->route('team.home', $team);
});

Route::middleware(ScopeTeamData::class)->group(function () {
    Route::get('/{team}/home')->name('team.home');
    Route::get('/{team}/projects/create')->name('team.projects.create');
    Route::get('/{team}/projects/{project}')->name('team.projects.show');
});

This lets you selectively affect routes. If we introduced a new event, e.g. RouteIdentified, that would allow for the same logic like my InitializeTenancyByPath middleware, but the user would have no choice over when it's used.

There's one alternative solution that I'd be very happy with, but I'm not sure if you'd be willing to add a new larger-scale abstraction to the routing system.

Essentially, allow something like "flags" on routes. You could do e.g.:

Route::flags('tenant')->get('/user/profile', ShowProfile::class);
Route::flags(['tenant'])->group(function () {
    Route::get('/user/profile', ShowProfile::class);
    Route::flags(['tenant' => false])->get('/something-non-tenant', ShowProfile::class);
});

// flags can also have values. here they tell the package the tenant identification strategy
Route::flags(['tenant' => 'subdomain'])->get('/bar', ...); // foo.acme.com/bar
Route::flags(['tenant' => 'path'])->get('/bar/'...); // acme.com/foo/bar

And then the RouteIdentified listener could check if ($identifier = $route->flag('tenant')) { ... }.

This might be easy to implement, as routes already have attributes and group() accepts an array of them.

And one more alternative would be having route-aware "app preparers", but IMO that'd be very confusing due to the similarity with middleware.

---

So in short, the options are:

1. Decide that preparing the application via side effects & outside the handle() method is a valid use case for middleware. And I'd finish this implementation
2. Introduce a RouteIdentified-type event that's fired before executing controllerMiddleware() + a system of route flags
3. Introduce route-level "app preparers/bootstrappers", i.e. Route::bootstrap(InitializeTenant::class)->get('/user/profile', ShowProfileController::class)

[stancl]

Hmm actually I thought of a decent simplification. We could do the event mentioned in option 2) above, with route filtering being entirely user-implemented.

I remembered that I had implemented something like the flags in the past — I created an empty middleware group and used it to add flags that another middleware would detect.

If option 2) were implemented, I'd still much rather include the flags system as well. But if keeping the PR simple is a priority, we can do 2) without flags.

Either way I'd be happy with both 1) and 2).

[taylorotwell]

I think you could combine the event with your existing middleware and let the middleware itself serve as the flag? The middleware would essentially only contain $next($request).

Then, gatherRouteMiddleware ends up looking like this:

/**
 * Get all middleware, including the ones from the controller.
 *
 * @return array
 */
public function gatherMiddleware()
{
    if (! is_null($this->computedMiddleware)) {
        return $this->computedMiddleware;
    }

    $this->computedMiddleware = [];

    $routeMiddleware = $this->middleware();

    $this->dispatchRouteMiddlewareGatheredEvent($routeMiddleware);

    return $this->computedMiddleware = Router::uniqueMiddleware(array_merge(
        $routeMiddleware, $this->controllerMiddleware()
    ));
}

And the method to dispatch the event:

/**
 * Dispatch the route middleware gathered event.
 *
 * @param  array  $middleware
 * @return void
 */
protected function dispatchRouteMiddlewareGatheredEvent(array $middleware)
{
    if ($this->container &&
        $this->container->bound(Dispatcher::class) &&
        $this->container->bound(Request::class)) {
        $this->container->make(Dispatcher::class)->dispatch(
            new RouteMiddlewareGathered(
                $this->container->make(Request::class),
                $this,
                $middleware
            )
        );
    }
}

What do you think? Admittedly it is a bit unfortunate to have to service locate the event dispatcher and the request here - but I don't see any other option at the current time.

Within your event listener you would have access to the middleware array (string class names) where you can look for your marker middleware to serve as a "flag" and do whatever you want from there.

[X-Coder264]

Is there any chance that we can actually remove the ability to define middleware in the controller constructor for Laravel 9?

This feature has bitten me more than once, the last time was auth0/auth0-PHP#543

The issue is even worse than it appears at first since all cases that I had up until now where this issue presented itself were not caught by tests (which were written) as it couldn't be caught, for example I'd sometimes get the wrong guard in the controller constructor and the tests would not catch that due to actingAs method calling manually $this->app['auth']->shouldUse($guard) which does not happen in a real HTTP request so the tests would be green and the app would NOT work as expected (the workaround for this specific problem was to either move the Guard dependency from the controller constructor to the controller method or to add contextual binding in a service provider).

Pros for the removal of this feature:

• the middleware and controller layers would be clearly separated from each other (as it IMO should be), that way you could safely rely on controller constructor DI (plus there would be some tiny performance improvements as controllers wouldn't have to be instantiated if a route middleware already returned for example a redirect response)
• all defined routes for a controller action could be seen in one place (the route file), without having to check if the controller constructor adds additional middleware
• there would be no need for this PR which is essentially a workaround which is only needed because of this feature

Cons:

• it'd be a BC break and a lot of applications out there would have to move their middleware definitions from the constructor to the routes file (if nothing else then at least for the auth controllers which are shipped by default in the Laravel UI package)

Personally I don't use this feature so I might be biased, but I don't think that this would impact the time needed for the Laravel 8 -> 9 upgrade in any major way (as the usages of this feature can be easily found by a simple string search and all such definitions can be quickly migrated to the route file) so I'm of the opinion that the benefits of removing this feature greatly outweigh the cost.

[stancl]

@taylorotwell I think that'd work, yeah. One reason why I liked the flags a bit less is that they move the behavior away from the middleware (e.g. InitializeTenancyByDomain) and to the listener, but I guess if I'd be using the actual class string as the flag (rather than an empty middleware group), then I could implement some interface on the middleware, just like in my PR, and run the logic from there.

The only issue I see in your proposed code is that it wouldn't unpack route groups. As I mentioned:

My implementation changes what's returned from gatherMiddleware() which is called in $router->gatherRouteMiddleware().

The latter is where route groups are actually "decoded", and where logic like excluded is handled. Which means that if you used a middleware that implements my interface in a route group, and applied the group on a route like ->middleware(['tenant']) for instance, it wouldn't work.

There are two options here — either run the logic from $router->gatherRouteMiddleware() on the data that's being passed to the event, or let the listeners deal with it themselves. I've done the latter once, and it's a bit painful. You essentially have to recursively loop through the groups, or have some depth limit.

So I'd extract part of the logic from $router->gatherRouteMiddleware() into a separate method (perhaps literally unpackMiddleware($middleware) and then either call that before passing data to the event, or let the listener make use of this method. But in either case, I think it should be extracted to avoid having to use custom loops like in the example I linked above.

@X-Coder264 I agree with your critique of controller-defined middleware — it's sort of nice that the framework can do it, but ideally people wouldn't use it at all. However, removing it would probably cause very large scale breaking changes. And I guess that some packages with weird custom setups depend on this ability.

I think that in PHP 8.0, using route attributes in controllers is the better way to achieve this:

#[Middleware('auth')]
class UserController
{
    // ...
}

I believe that there have been discussions about this in the past few months, with the outcome being that Spatie maintains a package for this rather than it being a core feature. (I may be wrong, I wasn't following these things much.)

I'm not going to argue about the merit of route attributes here, but I wanted to propose them as an alternative mechanism for controller-defined middleware. Removing the feature wouldn't be that bad if upgrading meant just:

+#[Middleware('auth')]
class UserController
{
    public function __construct()
    {
-        $this->middleware('auth');
    }

    // ...
}

And I guess that even if Laravel didn't have #[Route], it could have #[Middleware], since middleware is like the only route-related thing that can already be defined in controllers?

[stancl]

I've been thinking about this and I actually like the #[Middleware] solution a lot.

I looked at Illuminate\Routing\Controller, and middleware seems to be the only route-related thing that can be defined in controllers.

So the new syntax would be:

- $this->middleware('auth');
+ #[Middleware('auth')]

- $this->middleware('log')->only('index');
+ #[Middleware('log', only: 'index')]

- $this->middleware('subscribed')->except('store');
+ #[Middleware('subscribed', except: 'store')]

And the docs also mention passing callbacks to middleware():

class UserController extends Controller
{
    public function __construct()
    {
        $this->middleware(function ($request, $next) {
            return $next($request);
        });
    }
}

which could be replaced by methods:

#[Middleware(method: 'middleware')]
class UserController extends Controller
{
    public function middleware($request, $next)
    {
        return $next($request);
    }
}

@taylorotwell If you think the idea is worth exploring, I can spend some time on implementing it to see how that'd change the codebase. And I'd submit a second PR as an alternative solution to this issue.

And writing the middleware() method in the example above made me think that instead of the #[Middleware] attribute, we could also go with a static method 🤔 public static function middleware() which would return a Closure or an array with the middleware. Just one more alternative to consider. I personally like the attributes, but I see that they're not used in Laravel much yet.

[kkbt]

Please allow me to re-phrase the general request to a concrete problem:
Is there any solution out of the box to make teams tenants in Laravel?
I have tried it with v3 of Tenancy for Laravel, but teams seem to be available in Jetstream only (which I would like to use anyway), and v3 of Tenancy for Laravel doesn't support Jetstream, as mentioned above: archtechx/tenancy#772 (comment)

[stancl]

@kkbt I don't think this belongs here as that's a question about the usage of my package. Please respect the Laravel maintainers and keep this thread on topic. It's already long and complex enough.

[kkbt]

@stancl This is a misunderstanding.
I'm asking generally whether there is a Laravel solution for teams = tenants.
(It's not about the use of your package, I am looking for alternatives, too.)

[stancl]

Neither teams nor tenants are core Laravel concepts. You can implement teams however you want, with Jetstream being one (but not the only) solution. And you can implement tenants however you want, with my package being one solution.

This PR is about a routing internals change in Laravel 9.

[kkbt]

@stancl Thanks for clarification. If you know a better place to ask this question (out-of-the-box Laravel solution for teams = tenants), please let me know.

[taylorotwell]

@stancl Getting back to this after vacation. Personally I think we should proceed with the event I proposed. It requires the least changes to the code base for now AFAIK and at least makes your use case possible. The route group thing we can leave to the listener at the moment since, again, it's at least achievable even if it's a bit cumbersome.

Marking as "draft" for now until that transition to using an event is complete. Once that is done mark it as ready for review and I will get it merged.

[stancl]

Cool, will make the changes tomorrow.

The route group thing we can leave to the listener at the moment since, again, it's at least achievable even if it's a bit cumbersome.

Above I proposed moving this logic to a separate method, which would make handling middleware groups easy from the listener. Now I noticed that most of that logic is actually just the MiddlewareNameResolver::resolve call, so perhaps it'd work already.

But either way, I think that logic could be extracted to a separate method, like resolveMiddleware():

// Router
public function gatherRouteMiddleware(Route $route)
{
    return $this->resolveMiddleware($route->gatherMiddleware(), $route->excludedMiddleware());
}

public function resolveMiddleware(array $middleware, array $excluded = [])
{
    $excluded = collect($excluded)->map(...);

    $middleware = collect($middleware)->map(...);

    return $this->sortMiddleware($middleware);
}

Since the logic already exists, this would make it usable by the listeners, and it'd allow:

1. resolving route groups using very simple syntax (app('router')->resolveMiddleware($middleware))
2. handling all of the extra logic, like excluded middleware (possibly relevant for tenancy)

Which would make the final usage:

Event::listen(RouteIdentified::class, function (RouteIdentified $event) {
    $middleware = app('router')->resolveMiddleware(
        $event->request->route->middleware(),
        $event->request->route->excludedMiddleware(),
    );

    // detect tenancy middleware in $middleware
    // identify tenant
    // tenancy()->initialize($tenant);
});

I'll submit a PR with this tomorrow so that we can see the real implementation and then tackle specific details like the event name.

[stancl]

@taylorotwell It seems like there's already an event for this: RouteMatched. It's fired after the route is bound, but before any middleware is executed.

$request->setRouteResolver(function () use ($route) {
    return $route;
});

$this->events->dispatch(new RouteMatched($route, $request));

return $this->prepareResponse($request,
    $this->runRouteWithinStack($route, $request)
);

It's very similar the the event you suggested. So the solution that I'd be attempting is redundant as far as I can tell, since we already have the event.

Looking at the details of how the event works, I suspect that there might be some issues (related to the listener executing too early in the lifecycle), but most likely it's fine since the behavior I'm looking for worked with global middleware too.

So if the event already exists, I'll only make the gatherRouteMiddleware() changes in this PR.

[stancl]

(Please squash when merging)

[taylorotwell]

So are there any (even subtle) breaking changes on this now?

[stancl]

I think not. I was considering changing the target to 8.x but was worried that there could be breaking changes for people using the router directly. Now looking at the diff, there's only an added public method, with nothing changed or removed.

The only scenario where this could cause issues would be people extending the Router, having their own resolveMiddleware(), and calling it from methods other than gatherRouteMiddleware().

Up to you if that's a valid concern. I personally don't need this in v8 as I'll only be using this in a new major version of my package.

[taylorotwell]

Thanks for contributing to Laravel! ❤️

[driesvints]

@stancl cool that you could find an easy solution to this 👍 🙂