[9.x] Revert changes to controller middleware

[driesvints]

This PR reverts #40397. We've rethought the changes made in that PR and, at the moment, are not comfortable making these changes without knowing all implications. These changes can potentially have large implications on users, apps and packages.

We do recognise that this needs a better approach. We'd like to take our time to investigate and work on a solution ourselves. Most likely, this won't be done before Laravel v10.

I'm sorry @X-Coder264, @koenhoeijmakers & @stancl, this probably isn't what you expected/want. But we have so much users to cater for, we want to be careful with these things and not run the risk of breaking their apps.

[stancl]

Ah I actually didn't even notice that PR. Just wrote a comment there. Loris explained the issues really well and my current position is still that the static methods were the best solution.

Also, if the general reason why people support this change is that they'd want middleware to run before controllers are instantiated, I think that can be solved pretty well with the RouteMatched event that we discussed here #40165 (comment)

If you'd want, I can try to create a package that makes middleware get executed like that — before the Controller __construct — with very similar or identical syntax to how middleware currently works. I'll need the logic for Tenancy v4 anyway.

And if it ends up working well & people use it for enough use cases, we can retry the change to static methods in Laravel 10.

[driesvints]

@stancl something outside Laravel might be a good approach for now if that's doable.

[X-Coder264]

I understand that you need to be careful regarding "potential large implications on users, apps and packages" and that the merged solution was more of a compromise because the ideal technical solution was deemed as too breaking for end users.

But isn't the whole point of the Laravel beta releases exactly that - to catch such things? There's been 4 beta releases so far and I'm not aware that there's been any complaints of that PR breaking any use case (maybe I'm missing something?). Surely I'd expect if the breaking change was so big that somebody would've already raised concrete issues about that.

The only raised concern so far was #40397 (comment), which in itself has only one code example that is not even working at all on Laravel 8 so I'm not considering that as a realistic example. As such I think this reversal might be a bit too abrupt if there are no real issues reported from real use cases so far (even if they pop up all of a sudden on 9.0.0 release there's nothing preventing this from being reverted for 9.0.1 release IMO nor is there anything preventing us from trying to go for a better solution on Laravel 10).

[lorisleiva]

@X-Coder264 The reason why I came across this PR in the first place is that this broke the package Laravel Actions due to the change of behaviour caused by checking the existence of the getMiddleware method. I've managed to fix it but that's another example of things that can break: packages that extend the framework in more extensive ways than applications do. I'm not saying that packages shouldn't adapt to the framework — quite the opposite — but in this instance, I am finding the solution provided not ideal.

My biggest issue with PR #40397 is that it tries to couple middleware with controller instantiation.

However, that coupling is problematic because:

• middleware are, by definition, strongly coupled to the current request.
• controller instantiation is (and should be) strongly decoupled from the current request as it happens only once for applications that are kept alive (Octane, tests, etc.).

Here's an updated version of the diagram provided in #40397 (comment) to illustrate this.

I do like the idea of using middleware for static binding resolution — for instance, when the binding depends on the route and not on the request — but we cannot neglect that middleware depend on requests and therefore using them for that purpose is, IMO, a hack in the sense that we're using a pattern to implement something it was not designed to support.

When it comes to providing another solution, I believe a simpler and more predictable design is necessary. And I'm afraid, for me that already exists. You just need to add your dependency injections to the controller's __invoke method instead of its __construct method. And that's not even a workaround, that does make sense because:

• Dependency injections from the __construct method will be cached throughout all requests received regardless of the middleware it registers.
• Dependency injections from the __invoke method will depend on the current request and will be affected by the middleware it went through.

So we have two distinct ways of registering dependencies based on the application's lifecycle which is perfect.

However, if you need a more flexible way to register static dependencies in your controllers then there are plenty of ways to be creative within the framework. As @stancl mentioned, hooking into routing events in a package might be a good idea.

[X-Coder264]

If I understand what you are saying (which I'm not sure if I did since there's no concrete code example of what broke), does removing the caching of the controller on the route solve that problem? If it does then I think that's a better solution than completely reverting this which reintroduces the bug which that PR fixed. The controller caching is I guess just a performance microoptimization so that there's one object less to resolve from the container in long running processes, but since hundreds of services (if not more) are resolved out of the container on each request I doubt that caching only one gives us any tangible benefit. A better solution could definitively be pursued for Laravel 10 either way though.

[stancl]

Dependency injections from the __construct method will be cached throughout all requests

Does that currently happen with Octane? Because, as far as I know, it's not uncommon (validity aside) to use the constructor to set request-specific data on the controller.

[X-Coder264]

Dependency injections from the __construct method will be cached throughout all requests

Does that currently happen with Octane? Because, as far as I know, it's not uncommon (validity aside) to use the constructor to set request-specific data on the controller.

That's also a fair point. I know for sure that one of the apps that I currently work on injects a different implementation into the controller constructor depending on the current request.

        // AuthTokenProviderInterface is typehinted in the controller constructor
        $this->app->bind(AuthTokenProviderInterface::class, static function (Container $app): AuthTokenProviderInterface {
            $request = $app->make(Request::class);

            $client = $request->header('x-client');

            $authTokenProviderClass = match ($client) {
                'foo' => FooAuthTokenProvider::class,
                'bar' => BarAuthTokenProvider::class,
            };

            return $app->make($authTokenProviderClass);
        });

So the diagram above where it says that the controller is (prior to the PR changes) decoupled from the current request is certainly not correct for all apps so it definitely seems like the route controller caching should be removed in order for this code example to work with Laravel Octane.

[stancl]

Right. I'd expect the cache to be cleared between Octane requests since I've seen people inject even DB-related dependencies in the constructor I think. Plus, anything like the current user or a service that gets scoped to the user needs to be fresh on each request.

[lorisleiva]

@X-Coder264 No, there were no bugs or issues prior to merging PR #40397. I've also never mentioned the caching of controllers as an issue. I'm not sure why this keeps coming up. 😄 As for the bug in Laravel Actions, it was to do with the fact that actions are decorated inside ControllerDecorators which are responsible for providing the controller's middleware. Therefore, getMiddleware does not exist on the action class but on the ControllerDecorator resolved from the container. So basically by adding this early check, we're assuming that the controller will resolve to the same class which, for Laravel Actions, it doesn't. The fix was to force a getMiddleware method on the AsController trait so that it could be recognised as a controller that has middleware.

That's also a fair point. I know for sure that one of the apps that I currently work on injects a different implementation into the controller constructor depending on the current request.

Genuinely asking: Is that app using Octane? Also, if you run a test that calls the same endpoint twice, would that dependency (injected in the constructor) be different on the second request?

@stancl Are you sure about this? "Octane boots your application once, keep it in memory, and then feeds it requests". Why would Octane clear the cache at every request when its purpose is to make things faster by keeping things in memory?

[X-Coder264]

@X-Coder264 No, there were no bugs or issues prior to merging PR #40397.

Yes, there were bugs/issues (if that PR didn't fix anything then it wouldn't have been merged in the first place). The bugs/issues that were resolved by that PR were explained in detail here -> #40306 (comment) (and those are not all the issues that were solved, there were also other edge case issues which I haven't even bothered to write down as the comment was too long as is).

Genuinely asking: Is that app using Octane?

Obviously not (:smile:), since if Octane keeps reusing the same controller class on every subsequent request I don't see how could that app properly work with Octane at the moment.

Also, if you run a test that calls the same endpoint twice, would that dependency (injected in the constructor) be different on the second request?

Based on the current 8.x code it should be the same instance which is IMO wrong and part of the problem.

@stancl Are you sure about this? "Octane boots your application once, keep it in memory, and then feeds it requests". Why would Octane clear the cache at every request when its purpose is to make things faster by keeping things in memory?

Because an app is supposed to behave in the same way with or without Octane. That's why Octane already takes care of flushing cache where it's needed, just look at all those listeners -> https://github.com/laravel/octane/tree/1.x/src/Listeners (a lot of them are flushing some cache so my guess is there's a cache flushing listener missing for this case or Laravel shouldn't cache the controller in the first place). The reason why Octane speeds up your app so much is because the framework doesn't have to be booted on each and every request from scratch, caching the controller instance makes probably zero impact on the average request response time.

[stancl]

@lorisleiva Octane has a bit nuanced container state management logic. It stores the "pure" version of the container before request stuff is injected. Additionally, some packages clear other cache (such as static properties) with Octane hooks.

I asked because you wrote that it "will be cached throughout all requests" which sounds like it refers to setups like Octane. Perhaps aside from tests, normal apps should only have one Request in the entire lifecycle afaik

[driesvints]

Hey all, I tried to give this another review. But I'm afraid the sheer amount of information on these PR's/issues is just a bit overwhelming and hard for me to process. I'm still concerned there's a chance any of this will impact actual apps so for now I'm going to let this go. You can always attempt a new PR at master. If you do so please keep the explanation to a minimum while trying to address all of the concerns already outed here by @lorisleiva amongst others. Thanks