Controller is constructed before route middleware are executed

[christhomas]

Version: Laravel 9
PHP: 8.1

The problem that I'm having is that Laravel constructs the controller object and it's entire dependency injected set of parameters and the entire tree of other dependencies too. Before the middleware are run through and that means that middleware can't affect those dependencies by adjusting the request or setting other parameters that would affect how the AppServiceProvider can create the dependencies.

Is there a reason why the Controller is constructed before the route middleware are triggered and passed through? It seems like this is backwards, since the route middleware are supposed to do things based on the route and typically middleware affect how the system will run, THEN the controller executes and does the work.

But if the controller is created first, then all it's tree of dependencies are created too, direct parameters result in objects created, but those objects also have constructors, resulting in more objects being created, etc, etc. This entire tree of objects is created before the middleware has a chance to even do it's job.

This results in my rest api as a system which throws exceptions because one of the dependencies deep in the tree tries to do something that it's not allowed. Something the middleware would prevent by adjusting the request based on circumstances such as permissions or auth capabilities.

So whats the solution to this? I see this issue was talked about since 2016, but I can't think that I'm the only person who thought of this problem.

[DarkGhostHunter]

So what's the solution to this? I see this issue was talked about since 2016, but I can't think that I'm the only person who thought of this problem.

Because the framework allows to prepare the response a Controller construction level. That's why you can add middleware programmatically inside the __construct.

To me, this seems more like a dependency problem rather than Laravel's problem. Detach the logic from the controller middleware and add it after the controller receives the data. No need to do everything at the middleware level.

[christhomas]

The middleware problem I'm having has nothing to do with the controller in question though, it's a system level need to adjust the request constructed in order that the dependencies to the controller are constructed in the correct way.

I can't add the middleware into the controller, since the middleware does not care about the controller, it adjusts the objects that are created by the app service provider. There are a couple of controllers that also use the same dependencies.

The issue that I have, is that route middlewares, don't actually seem to exist. You can define middlewares on routes, but those middlewares are run AFTER the controller construction, hence, ALL route middlewares are in fact, controller middlewares.

I want a middleware, which is connected to the route requested, to execute and do it's job, before the controller is run, before any of the dependencies are created.

I understood from the docs that there are three sets of middleware

1. global middleware
2. route middleware
3. controller middleware

but seems that all route middleware are executed after the construction of the controller, then they are not route middleware, because they are not in the middle of anything. They are always executed AFTER the controller. That isn't the purpose of a middleware

[christhomas]

This pseduo-code is an illustration of the problem

class EnableServiceHeaderMiddleware {
    public function handle(Request $request, Closure $next) {
        config()->set('is_public', true);
        return $next($request);
    }

// service_headers is mapped to the EnableServiceHeaderMiddleware class
$router->group(['middleware' => ['service_headers', namespace...etc, etc], function(Router $router) {
    $router->get('/public', 'PublicController@getData');
});

class PublicController extends Controller {
    public function __construct(DepA, BrokenDepB, DepC) { ... }
}

class AppServiceProvider {
    public function register() {
        $this->app->bind(BrokenDepB::class, function() {
           $c = config('is_public') == true ? config('public_a') : config('private_b');
           return new BrokenDepB($c);
        });
    }
}

So I guess the heart of the problem, is how can I create a fully dependency injected controller, that depending on the route that controller is executed on, will construct the BrokenDepB object, with the correct configuration needed for that route, based on it's requirements.

I can't adjust BrokenDepB because it's used in public AND private scenarios. It needs to allow for both. I can't also create two subclasses for PublicBrokenDepB and PrivateBrokenDepB either, because I've noticed in the class heirarchy of some dependencies, the controller is not the one creating the dependency, but a dependency down in the tree of other objects created by another controller. So the problem is I have no direct connection with all the instantiations of that object.

So I need to be flexible, to use a configuration value to adjust how that class will be created, no matter where in the heirarchy tree it will be.

This problem would be solved, if route middlewares triggered SEPARATELY from controller middlewares, like this

Global middleware -> router -> Route middleware -> Controller Construction -> Controller Middleware.

It seems that Route middleware and Controller middleware are being lumped into one group, executed AFTER the controller is constructed. I think that's a mistake. That's what I'm trying to discuss here.

[X-Coder264]

Here are some links for some reference and context regarding this specific issue: #40165 (comment) and #40306 and #40397 and #40675.

I've tried fixing this issue as it had bitten me multiple times in the past and unfortunately every attempt got rejected/reverted in the end, mostly due to concerns regarding backwards compatibility.

IMO there's no way around this, at some point in the future the feature of being able to add middleware via the controller constructor needs to go away as that feature is the reason why all these bugs are happening. And that's exactly one of the reasons we have a major version concept in SemVer, so IMO the sooner this happens the better.

At the time when those PRs were a thing I don't believe the maintainers understood just how broken this behavior is and how big the impact is across the community (as it was just me and a few other people that were pushing for this to be fixed) so a decision was made that the BC cost outweighed the benefits. Hopefully with more and more issues popping up regarding this problem that decision is gonna be reevaluated especially as using dependency injection gets more popular within the Laravel community.

Laravel 10 is scheduled to be released in February 2023 so there's time for somebody to attempt a new PR. Maybe somebody has a better idea as to how to solve this problem.

[rodrigopedra]

@christhomas you can inject dependencies on controller methods, they will be run after any declared middleware:

single action controller

class PublicController extends Controller {
    public function __construct() {
        // middleware declaration, authorization, etc...
    }

    public function __invoke(DepA $depA, DepB $depB, DepC $depC) {
        // ...
    }
}

multi-action controller:

class PublicController extends Controller {
    public function __construct() {
        // middleware declaration, authorization, etc...
    }

    public function index(DepA $depA) {
        // ...
    }

    public function create(DepB $depB) {
        // ...
    }

    public function store(Request $request, DepC $depC) {
        // ...
    }
}

@X-Coder264

in the future the feature of being able to add middleware via the controller constructor needs to go away

I hope not.

Might be personal preference, but in projects with gazillion routes spread over many route files, I prefer to declare middleware in the constructor, so they are close to where it affects code. I also prefer to keep my route files as clean as they get.

To me it is much easier, and makes more sense, to look at a controller constructor and see all needed middleware close to the method I am working with, than to skim a very long route definition file. It also helps in code review, it is one less place to look when refactoring, and it is much easier to spot which parameter a middleware that accepts parameters is using when they are close to the route. Not just easier to spot, but easier to reason and understand why that parameter might be different to that particular route.

I always advocate for declaring middleware on a controller's constructor when showing people Laravel.

Also, resolving dependencies on the controller method seems, to me, to be a better solution:

• On single action controllers (ones using __invoke) it actually doesn't matter, and there is no reason to add state to the class object just to hold the dependencies for a single method that can require its own dependencies.
• On multi-action controllers, injecting dependencies by methods helps avoiding instantiating unnecessary dependencies that can be costly to instantiate when calling a method that won't need them.

One last thing, before the AuthorizesRequests, that streamlines adding the can middleware, was only available as a trait. Now I believe one can define them on the route definition (I am not sure), but in a lot of projects this helped pushing middleware definition to the controller's constructor.

I understand some people says "dependency injection should be done in the constructor...". Well, I believe there is nothing wrong in providing dependencies to a method if only that method will use it, and also - in my opinion - Controllers classes are not a typical class on a OOP sense. They are short lived, there is no sense on them to hold state, and their purpose is to connect the pieces needed to resolve a response from a request. I usually think of them more like lambdas, or plain functions.

And lastly when we declare closure routes, the dependencies are passed to the anonymous function parameter list. Having dependencies passed to a controller method instead to a constructor looks more consistent with this behavior.

being able to add middleware via the controller constructor needs to go away as that feature is the reason why all these bugs are happening

Which bugs? It is how the framework request workflow works. Just because something does not work as you expect that does not mean it is a bug.

I understand one might want something to work differently. I actually have things I preferred to be different in behavior in other parts of the framework. But I can live with those for the benefits Laravel brings to the table.

In my opinion, the trade-off of using a framework is exchanging the time to solve trivial day-today tasks with adapting to some conventions, and - sometimes - to workflows we really wished would be different.

And let's agree on something, Laravel helps to solve way more than the trivial things a web-app needs.

But that is definitely no a bug per se. Sorry for the long post, no hard feelings =)

[X-Coder264]

@X-Coder264

in the future the feature of being able to add middleware via the controller constructor needs to go away

I hope not.

Might be personal preference, but in projects with gazillion routes spread over many route files, I prefer to declare middleware in the constructor, so they are close to where it affects code. I also prefer to keep my route files as clean as they get.

Nobody said that those declarations need to be removed from the controller. I said those declarations need to be moved away from the controller constructor - constructor being the key word here as it means that the controller must always get instantiated in the "gathering of middleware for the route" step of the request lifecycle. If you take a look at the PRs that I've linked you can see that my proposal was to move those calls from the constructor to a static method (within the same class). During the discussions attributes/annotations were suggested for this too, especially since we now have native support for them in PHP. Either way, every approach that would properly fix this issue would be a breaking change.

Also, resolving dependencies on the controller method seems, to me, to be a better solution:

It doesn't even matter whether you think method injection is better than constructor injection. Without even looking at all the other issues that currently affect the constructor injection approach in controllers (which were described in the linked PRs) the fact that the behavior between these two injection approaches is different/inconsistent is an issue in and of itself.

being able to add middleware via the controller constructor needs to go away as that feature is the reason why all these bugs are happening

Which bugs? It is how the framework request workflow works. Just because something does not work as you expect that does not mean it is a bug.

These bugs: #40306 (comment) and auth0/auth0-PHP#543

And these are just the examples I could think of the top of my head that I've personally run into multiple times.

Every bug in the framework that was ever fixed was how the "framework worked" up until the point at which the bug/behavior was fixed so that statement makes no sense IMO.

To be fair, as far as this goes, nobody can convince me that this is not a problem. As an example, getting a completely different guard depending on whether you've used constructor vs method injection cannot be described as anything other than a bug (especially since you can't even catch that problem by writing tests) so at worst the code reaches production and it doesn't work because the guard keeps returning null instead of the logged in user. Method injection is just a workaround, the issue/bug with constructor injection is still there. Even Taylor acknowledged that this is a bug, which is why he merged a fix for it (which was later reverted like I've already mentioned due to BC reasons).

So if you are "lucky" your application immediately blows up due to an exception (like the author of this issue mentioned) during development, if you are not lucky then you get an issue like the guard one which does not throw an exception and you have broken behavior in your app that you are most likely completely unaware of until your QA department (or potentially even worse - end users on production) report the problem to you. And then, especially if you are not an experienced developer and/or not familiar with the internals of the Laravel middleware gathering logic you are scratching your head because you can't figure out why the guard that is supposed to return a logged in user is all of a sudden constantly returning nulls.

[rodrigopedra]

If you take a look at the PRs that I've linked you can see that my proposal was to move those calls from the constructor to a static method (within the same class)

I did. Very carefully. Now and then. And the comments made myself even surer of my intance on this.

It is very odd you assume I didn't. Maybe you assume I just had to read the comments you made or the ones supporting your personal preference.

As you assumed something out of the blue about me, I think it is safe to assume you don´t quite remember the discussion had strong arguments with a different and more compelling argument. It is natural to have selective memory and tend to remember only the reality we wish it happened, but unfortunately not always the reality is the same as we wished.

See this comment (linked below), which you seem to have missed or forgotten, which even has a diagram to help to illustrate for those who can´t follow only with words. Some excerpts from that comment, (which I, and as you can see others too, strongly agree):

[...] 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.).

...

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.

#40675 (comment)

It doesn't even matter whether you think method injection is better than constructor injection. Without even looking at all the other issues that currently affect the constructor injection approach in controllers (which were described in the linked PRs) the fact that the behavior between these two injection approaches is different/inconsistent is an issue in and of itself.

It also doesn't even matter whether you think constructor injection is better than method injection.

This is a (my) personal opinion:

There is no sense on adding a constructor dependency to a object state just be used by a single method when the object is short-lived and meant only to act as a short-lambda connecting two dots into the request cycle

This is also a (your) personal opinion:

Method injection is just a workaround, the issue/bug with constructor injection is still there.

All the "issues" you mentioned can be solved with a note in the docs.

Unlike civil engineering, and other engineering, that many times, due to regulation requirements, have to follow strict standards, software engineering does not have an agreed set of standards on how things should be done.

Most best practices are just "personal opinions". And arguing Laravel should follow the behavior because "other frameworks do like this" (see your past comment linked below), is not actually a compelling argument to change how things have worked across the years just to satisfy a edge case that can be solved with a note, or to satisfy your personal preference or "opinion".

Also using the argument That's what users expect because that's how it also works in other frameworks, to back up your own preference, when this works the way it works on Laravel for many, and many years, is also not a compelling argument (also in the same comment linked below).

#40397 (comment)

Even Taylor acknowledged that this is a bug, which is why he merged a fix for it (which was later reverted like I've already mentioned due to BC reasons).

Well, so many things here:

Even Taylor acknowledged that this is a bug

I would appreciate if you can link where he says it is a "bug", or that this behavior needs to be fixed (which could imply it is a bug).

On every discussion I can find, he at most says he is open to alternatives. He propose using an event as a better alternative, and even expresses, more than once, the risks of breaking changes for "put the breaking change on people because for 99% of Laravel devs it's most definitely not worth it IMO" (link below)

#40306 (comment)

which is why he merged a fix for it

Being the first affirmation false, you can't assume it was a justification for he merging anything. On my understanding he changed on good faith the BC would minimal, after much insistence, even after trying to persuade that using events would be a better solution.

#40165 (comment)

which was later reverted like I've already mentioned due to BC reasons

Well, one might interpret the justification on the reverting PR differently, for instance:

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.

I concede that "Large implications" could be related to BC.

But one might agree that the value proposed by the changes were not sufficient to justify changing a well-defined behavior most developers are comfortable with. Which is slightly different than a BC done on the will of improvements. And how would a revert before a release have caused any BC?

Actually the only real comment regarding the justification tried to appeal for those changes is when Dries states the arguments are overwhelming and asks for a minimal description on a next attempt:

#40675 (comment)

Prior art

One last thing. As you assumed I didn't read any of the discussions you linked, I guess you won't bother if I assume you are, in good faith, not aware of past discussions regarding of injecting dependencies into the constructor.

Prior to Laravel 5.3.4, there as a common pattern of expecting the session and request to be available at a controller's constructor, just like you are pushing now.

Since Laravel 5.3.4 this behavior was changed, and the Controller@middleware method was added as a workaround for those who wanted a way to still access application state inside a controller's constructor. You can see the upgrade path here:

https://laravel.com/docs/5.3/upgrade#5.3-session-in-constructors

And this interesting excerpt from the official docs linked above:

In previous versions of Laravel, you could access session variables or the authenticated user in your controller's constructor. This was never intended to be an explicit feature of the framework.

There were discussions about either a controller constructor should have access to application state or not, and at the end the decision was to introduce the breaking change linked above, while providing a workaround (closure-based middleware defined in a controller's constructor), as accessing application state in a controller's constructor ".. was never intended to be an explicit feature of the framework".

These are some PR discussions regarding this matter:

• [5.3] Controller closure middleware #15080
• An Open Letter to the Core Laravel Developer Team [5.3 upgrade breaks Auth::user() in the base controller] #15072

And a complementary blog post:

https://josephsilber.com/posts/2017/01/23/getting-current-user-in-laravel-controller-constructor

Also, as you attributed an opinion to Taylor without providing any link to it, in PR #15072 his opinion seems very clear:

... from a design perspective it's very bad to use session or auth in your constructor and here is why: no request has happened yet and session and auth are INHERENTLY tied to an HTTP request. You should receive this request in an actual controller method which you can call multiple times with multiple different requests. By forcing your controller to resolve session or auth information in the constructor you are now forcing your entire controller to ignore the actual incoming request which can cause significant problems when testing, etc.

Most of the confusion on this issue stems from a failure to realize that sessions and auth are specifically tied to http requests and cookies and it's impossible to access those things until a request has actually entered your application. The very fact that you can not go into the command line and instantiate your controller via the "tinker" command should be a huge red flag that the controller is not constructed properly.

#15072 (comment)

Of course I will understand if his opinion changed since 2016. But this is an actual opinion he shared related to the matter, not a personal interpretation based on my personal wishes or preferences.

Also I am not saying you are lying, as you are a long time contributor you might have had a personal talk with him. I am just saying I could not find any comment that back up this opinion you attributed to him.

Epilogue

I agree maybe we need to document better that application state is not available in a controller's constructor. And that this is intended behavior, and how the framework works.

Having this documented would solve the "issues" you linked that third-parties might have when integrating with Laravel.

A mechanical can expect that an engine works the same as they are used to work with other engines. But if there is a manual stating the differences, they won't fight against it due to personal preferences.

This is absolutely not a bug, and using method injection is absolutely not a "workaround".

Even Symfony, that you mentioned to back your personal opinion, allows method injection along with property injection these days.

So following your idea of "we should do what others are doing", maybe we could propose adding property injection, which "in my opinion" does not bring enough value compared to the hassle implementing and maintaining it.

But as I stressed, that is my opinion. If, and when, added to the framework I could try to present my arguments, but as long it is accepted, I will also accept it and deal with it, regardless of my personal instance.

Appealing to labeling "method injection a workaround", implying with a constructed narrative that constructor injection is better in any form, is the kind of purism that reminds me of the discussion around if Façades are true Façades in regards to design patterns, or if they should be named proxies instead.

As I said, there is no agreed standards on software engineering on how to best built things. And personally I think this is better than forcing a single way of thinking that foster innovations.

Not agreeing with a particular behavior is fine. Proposing changing a behavior you don't agree with is fine.

Labeling behavior you don't agree with as bugs is not fine.

Trying to argue with fallacies (I noticed appeal to authority, ad hominen, bandwagon, and casual fallacies) without presenting compelling benefits on why such a structural change should be accepted is not fine.

[X-Coder264]

• It's not "very odd" that I assumed that you haven't read that as you were asking "which bugs" in the previous comment which is the reason why I've linked you the post which explains them. Unfortunately I don't remember every Github username that was part of every Github discussion that I was ever part of.
• I didn't say that Taylor said it was a bug. I said he acknowledged that it was a bug and he did so by merging a fix for it. You can't fix something that is not broken, right? :) If that wasn't a bug/issue the PR wouldn't have been merged, it would've been closed instead. "We do recognise that this needs a better approach. We'd like to take our time to investigate and work on a solution ourselves" ([9.x] Revert changes to controller middleware #40675 (comment)) - also a quote from the maintainers -> if there is no issue/bug why would they talk about working on a "solution"? In order for there to be a solution there needs to be a problem/bug/issue first.
• "a well-defined behavior most developers are comfortable with" -> this behavior is the exact opposite of "well defined" as it is not "defined" at all anywhere in the documentation nor would any person that is new to Laravel expect this behavior. People usually get to know this behavior when they run into a bug which they have to debug because of this. If this behavior was so well-defined this Github issue would have probably never existed. The only people that are actually familiar with this behavior (excluding those who run into issues and have to debug and find out about this issue that way) are the maintainers themselves, other people who just love to deep dive into the framework internals for fun and people who are constantly reading all Laravel Github issues and PRs. That excludes basically most of the framework users so most developers are not comfortable with this behavior as your average Laravel developer does not even know about this behavior (the average Laravel developer does not even use any kind of dependency injection, they use facades or global helpers instead because that's what is used in 99% of the Laravel documentation). Also it is my opinion that "fixing" this by adding a warning to the documentation is not adequate.
• I don't see how Taylor's comment from 2016 and the "you could access session variables or the authenticated user in your controller's constructor, this was never intended to be an explicit feature of the framework" sentence applies here. I don't want to access session nor get the authenticated user in my controller's constructor, I actually believe that to be an anti-pattern so I'm glad that Laravel explicitly said that that was not an intended feature. What I want is to be able to reliably get the authenticated user in a controller action method while still being able to use constructor dependency injection (and that's not the only thing that this issue impacts).
• You can use constructor dependency injection in all classes including controllers. It doesn't make sense to all of a sudden say "you can use constructor dependency injection in controllers, but you can't use it for X, Y and Z scenarios". A feature either works or it doesn't so IMO you should either be able to always use it in a controller constructor or not.
• I brought up Symfony back then because I thought that was a thing that they are doing better than Laravel currently does (the controller is a service that is resolved out of the container like any other without any catches and "edge cases" like the ones we are discussing here). Guess what, I think that property injection is an anti-pattern that Symfony shouldn't have merged as a feature and should never be used as there's already constructor and method injection available so that debunks your "we should do what others are doing" statement about me. I've used various frameworks over the years (both publicly available ones as well as in-house built ones) so I don't bring up other frameworks because I think Laravel should blindly follow anyone, I only bring up very specific things when I believe that those things are better solved elsewhere.
• Most of your comment is focused on trying to convince me that this is expected behavior instead of discussing the substance of the problem which you basically label as non-existent even though it is obvious (as evidenced by the existence of this Github issue and all of the previously linked PRs) that a problem exists. Even on both a purely technical and logical level it doesn't make sense to instantiate the controller before we know that the controller logic will even have to be executed (e.g. if the middleware already returned a response or if the middleware throws an exception like for example the authentication and authorization middleware do when the user is not logged in or when they don't have permission to access the resource). So it's no wonder why the author of this Github issue is asking why is this behaving in this way.
• I don't understand why third party libraries are brought up when this issue is being discussed (as that is usually done to somehow try to prove that this is some rare edge case). You do not need a third party library to be affected by this bug. You can reproduce this issue very easily with just a few lines of code on a freshly cloned Laravel skeleton app. But yes, third party libraries are also affected by this issue so it'd be best for everyone if Laravel's controller constructor does not behave in such a weird way as it currently does. Whether that's the expected behavior according to the maintainers or not is IMO irrelevant in this case as it does not match what the average developer would expect and it cannot be expected of third party packages, especially of framework agnostic packages to be aware of every quirk that some framework has.
• I don't care what facades are being called. I don't use them in my projects nor does naming affect functionality. And that was/is the advertised beauty of Laravel - it doesn't force you to use facades nor global helpers nor dependency injection. But all of a sudden you are telling me that I should use method injection in controllers instead of constructor injection and that's why I consider that a "workaround" and why I consider this to be a bug/issue, as this actually affects me and a lot of other developers.
• One other advertised beauty of Laravel is DX. Having such an inconsistent constructor dependency injection behavior which is not well known at all definitively negatively impacts DX.
• This is probably gonna be my last reply on this topic as I've already spent too much time and energy on this topic, trying to make the framework a better tool for every user and trying to find a solution which would be a compromise so that the root issue gets fixed while keeping both sides happy.

[rodrigopedra]

You can't fix something that is not broken, right? :)

Laravel is always open to merge different ways to complete a task. From the discussion on the merged PR it is clear it was merged on good-will to address the request, but all the times the concern on adding a edgy-case that could led to many apps bring broken was expressed.

If that wasn't a bug/issue the PR wouldn't have been merged, it would've been closed instead.

A lot of PR are merged to add new features, to add new ways on doing something, to improve DX, to remove deprecated code.

Laravel has a good-will tendency to embrace lots of opinions. And when something goes wrong it gets reverted. This also happens commonly.

I could rephrase your argument as : "If it was a true fix it would not have been reverted, right?"

There are many features, such nested filed validations, that took many minor versions to get it right. And although reverting was considered, there was energy and effort on trying to solve the issue and make an improvement.

On my understanding is that they are open for improvements. But if it was an issue I am sure it would have been resolved by now.

I didn't say that Taylor said it was a bug. I said he acknowledged that it was a bug and he did so by merging a fix for it.

You're right. For me as a foreigner both statements seems similar to me. But I see the difference and I apologize for stating this.

My intention was argue on the later. But late justification is not a good argument either. And I don't like when people states someone said something they didn't.

I am sorry for not reviewing carefully this part of my reply. And I hope you can apologize me for this part.

You can use constructor dependency injection in all classes including controllers.

That is not actually true. Well the container is capable of trying to create a not bound object , this is correct.

But in Laravel, jobs have their dependencies inject on its handle/__invoke method, while the constructor is reserved for passing state variables to be used on the job execution.

The same is true for mailables, notifications, and other components.

One could argue it is more idiomatic in Laravel to have a execution dependency injected on a method and not on a constructor.

don't understand why third party libraries are brought up when this issue is being discussed

I brought it up because that was actually the single real issue with any impact you linked before. All the other links were PR trying to change the behavior.

But all of a sudden you are telling me that I should use method injection in controllers instead of constructor injection and that's why I consider that a "workaround" and why I consider this to be a bug/issue, as this actually affects me and a lot of other developers.

Well, nowhere in the documentation says you can't use dependency injection on jobs classes. But if one tries, and the class is complex enough, it will error on serialization.

What I said, or at least what I want to argue, is that my understanding is that controllers are to be seen as state-less lambdas that connects the dots on a request/response cycle. In that sense it is more logical to inject its dependencies on execution than on instantiation.

If for example, the business logic on the example presented in this issue was extracted to a job/action, and this job or action (I am calling action a synchronous job), gets dispatched from the controller, the dependency injection of this job would happen after the request arrived the controller and all the middleware had already run, there would be no such issue.

I am not suggesting or imposing a way to how one should structure their code. As I said software engineering does not have agreed, or at least regulated, standards to follow.

But in the same sense you stating that some way of doing things needs to go away (see citation below), or that implying things should be done in a way and if done in another it is surely a workaround, is not a reasonable thing, and is "all of a sudden you are telling me that I should" do things the way you prefer, and calling other perfect reasonable ways - even explained with diagrams - as workarounds.

It seems, in my sole opinion, you are forcing your way into others and is not open to alternatives. And I am sorry, but this is something I would not like to see in Laravel, or a thing I think will improve any DX on the framework.

Again, a note in the docs that dependency injection should be done in the methods, such as it is currently done in:

• jobs
• mailables
• notifications
• Commands (can be done on the constructor too, but it is common to see examples of this done in the handle method)
• View class based components
• and other components

My argument on "a well-defined behavior most developers are comfortable with", is based on framework consistency across components, and not on personal preference.

This is probably gonna be my last reply on this topic as I've already spent too much time and energy on this topic

Me too. I hope you don't think this is personal, it is not.

To me controller classes are to be considered like a "super" closure, a way to decouple writing all your app on the route file. In that sense I don't see a reason why it should hold any state.

And to me, and I think to many others, constructor injection is primarily to set an instance state.

But I think it is better we agree on disagree. My contend was with statements like:

in the future the feature of being able to add middleware via the controller constructor needs to go away

As you said Laravel is known for its DX.

And keeping the ability to define middleware the way they currently are, and seeing method injection as a logical thing to do, and not "a workaround", is an opinion I, at least one more developer (the one I linked their comments), and I believe many others, are comfortable with and would not want it to go away due a purist discussion on how things are better done or should be done.

My suggestion is still adding a note on the docs, and call it a day.

But if you can find a full backwards way to keep things the way they current are, which again is not a bug, while avoiding this behavior you don't like, or at least don't seem to find correct, I will totally support it.

[rodrigopedra]

@X-Coder264 I sent PR #44192 as a proof of good-will to propose a middle-ground solution that I think will solve the issue you described while keeping changes a minimal.

If you can review it, I would really appreciate.

Thanks for all the insights, and I hope any contends are kept in the realm of the ideas an not personal.

[driesvints]

Hi everyone. I'm very sorry but this issue is just too much for me to take on and read through. Afaik as I can tell from glancing through it I can only say that this is the current expected behavior. I see @rodrigopedra has sent in a PR to change this behavior in Laravel v10. Let's see what Taylor says there.

[christhomas]

Hey guys, thanks for the replies. Sorry I'm late back to this cause I've been busy at the weekend.

I've read the first couple of lines of #40165 and this problem of being able to adjust the application state based on tenancy is the exact same problem that I've got. I need to adjust the app state based on the route, but the controller gets dependencies whilst when constructed, take the wrong state because the middleware runs AFTER the controller gets constructed. That actually is the shortest explanation of my problem I've been able to make so far.

[christhomas]

From what I can see, #44192 is exactly what I was looking for, what do you think @X-Coder264 ?

[stancl]

He replied in that thread. That PR does look good. I haven't been able to review it in depth but it seems to nicely solve the problem that all our PRs have tried to solve in the past.

[JerryBels]

Hey guys, I've been carefully reading the thread and the PR, yet I struggle to make it out of my issue. So, I created a middleware to limit the data a connected user has access to by adding global scopes depending on some informations:

public function handle(Request $request, Closure $next)
    {
          if (auth()->user()?->organization_id) {
              User::addGlobalScope(new OrganizationScope(auth()->user()->organization));
          }

        return $next($request);
    }

The middleware is added to the 'auth.group' middleware group in Kernel.php which is used in web.php:

Route::middleware(['auth.group'])->group(function () {
  Route::resource('users', UserController::class);
});

Then in the controller, I would expect a user to get a 404 when trying to see a page of a user he has no rights to. But the $user is retrieved before the middleware applies the global scope.

public function show(User $user, Request $request) {
  // dd($user); // <= This actually contains the User model! It shouldn't, of course.
  // dd(User::find($user->id)); // <= null, as it should!
}

So, the dependency is apparently calculated before the middleware is applied. That's how I got to this thread. And I can't make sense out of it about how I could have that middleware run when I want it.

Before that I was applying the global scopes at the Model level, in the booted function (as shown in the docs). But I had lots of issues with that - namely, accessing a relationship from there to check what is allowed or not is problematic, as the relationship call will look for something in the Model itself, and said model is not ready since it's booting. For example, checking a relationship of the connected user on the User model has to be done with a direct query to the db, that will be ran every time the model is called... Not good.

Anyway, I like the middleware approach as it is a clean way to deal with rights as well, I think. Any recommandation?