An Open Letter to the Core Laravel Developer Team [5.3 upgrade breaks Auth::user() in the base controller] #15072
Comments
|
You should not attempt to get the user in the constructor itself. It's fine to inject the auth manager into the constuctor though for use inside a method later. |
|
We do have an LTS 5.1 release, and 5.2 will keep getting security fixes at least for now, so there's no massive urgency for you to upgrade straight away. I guess we perhaps should have documented this bad practice better in earlier versions. |
|
@GrahamCampbell - again, the point isn't what we should and shouldn't do. While there are merits to both approaches, the point is that many of us did it a certain way in 5.2 that is now broken in 5.3. We're either forced to aggressively refactor production apps to stay current, or abandon future upgrades and stop at 5.2. Neither is a desirable choice. I'm suggesting a compromise. |
|
Yeh, I'm not sure what we should do here. There seems to be a larger use of the constructors like this than we thought. |
|
This is definitely a massive breaking change...at least for our code base. I have noticed Auth and Session slowly moving up the stack since Laravel 4. Auth used to be accessible all the way up inside provider boot() and register(), then it moved down, and down and down with each release. Now even a controllers __construct has null Auth::user() and null Session::all(), which breaks over 30 production products. I get your argument of "better practice" blah blah...but assuming everyone will never have a need to access Auth or Session higher than a controller method is just wrong. In the end this was a massive change and there was zero documentation on it. @GrahamCampbell is there a way to let us "start" the session ourselves somewhere, instead of waiting on the middleware to kick in? |
|
No. The issue here isn;t that it's not available anymore, it's because we're newing up the controllers more than once. The errors you're seeing is from before we start doing anything with the request. |
|
So, if the app were to continue running, the 2nd instantiation would work just the same as in 5.2. |
|
@GrahamCampbell - I very much agree that some documentation supported by Laracast tutorials on the "proper" way to do this would be welcome by us all. Speaking for myself, the approach as you describe it seems antithetical and inefficient. I'm 100% sure my opinion is borne from my ignorance on injection and a lack of a deep understanding of the inner workings of Laravel, so some documentation that bridges that gap between standard MVC practices and the "Laravel" way would help a lot. |
|
It is documented as of this morning. Please post code samples of how you were using this behavior. Secondly, 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. |
|
Also, please see the workaround in the upgrade guide. Create a method that retrieves the data you need and call that from your action. If that does not solve your problem post a code sample that I can view. |
|
@sstringer I see your frustration. This will demand a bit more effort in your migration, but with the workaround proposed by Taylor your issue will be solved. I agree that we need to move forward, mainly to follow better practices. Note: https://github.com/laravel/docs/blob/5.3/upgrade.md#session-in-the-constructor |
|
There is an alternative solution we could consider but need to see how others were using this behavior first. |
|
Thanks for documenting, that was the point. I agree its wrong. __construct is easily abused because its the one function thats always loaded regardless of which route they enter the controller...so it's easy to abuse the __construct and add always executed code there. There are many workarounds. @GrahamCampbell if __construct is called multiple times now, wouldn't a |
|
@taylorotwell - Thank you for your reply and thank you for updating the upgrade documentation. I'll email you directly to address your request to see how I am using this methodology. I think you've hit on the core point of confusion for many of us when you say, "no request has happened yet" when a controller is constructed. Perhaps the surprise expressed by Graham and other Laravel framework devs at this controller-based approach is due to the fact that the order of initialization is pretty much hidden deep inside the inner workings of Laravel. In my experience prior to coming to Laravel (e.g. in CodeIgniter, Drupal, my own framework, and others), http request-dependent logic has always been available at the controller level. Considering the fact that it "just worked" in 5.2, it never occurred to me that I should have been taking a different approach in Laravel all along. Going forward, I agree that deeper documentation and Laracast tutorials are needed and would be much appreciated. Thanks, |
|
Here is one example of how I was using (abusing?) it. For your records. We have a reporting dashboard, with many entry points (routes). All controllers Extends the main Controller.php as usual. A user can hit ANY url and pass a GET Example
With our system, we always REMEMBER the last selected dealer. So in my master Controller.php __construct I would check Im not saying I can't find a workaround (custom middleware)...there is always a hundred ways to code something. Just giving an example. We code in Laravel 60 hours a week, every week since version 1 and we have 2 dozen production products powering nearly every car dealership in the US...all running nearly a million lines of Laravel code scaled on a hundred Debian servers. So....it breaks things...again not saying you should change it. I trust your judgment on whats right. If calling __construct multiple times (therefore forcing them to be very light) and removing Auth and Session at that level is right...then its right. |
|
Thanks for the feedback @mreschke. We have an idea to allow you to easily register a middleware via a Closure in your controller's constructor which would solve the problem but it's still being implemented right now. |
|
IMO i think that we could look again into the #14834 that suggested the creation of the boot method, again, it's not a good pattern but would keep things simple and provide a solid path for migration. Also i find it controversial saying the boot method is an anti pattern for creating a 2 step __constructor and recommend in the docs the creation of a method for handling your custom session logic and calling it each time (this is just as bad or even worse IMO). Maybe instead of creating the boot method we could come up with something that could bind methods that would be called every time before an action is executed so we could do: public __construct() {
$this->before('index', ['handleSessions']);
}
private handleSessions() {
//do your thing
}
public index() {
//handle sessions already executed
}Not sure if this is viable? Edit |
|
@mreschke - very interesting example. I'll share a simplified version of my own here: Let's take MapTechnica.com as an example. Like many (most?) sites, I have subscribers and admins. Based on what Laravel Cashier subscription status and level they have, coupled with permissions set at the database level, I set certain flags that control what features are visible and hidden to the customer through the front end. Since this switching logic is needed by every page of the app, and to avoid the need to trigger this in every single method (e.g. index(), show(), edit(), etc.) in every controller, it seems most efficient for me to simply call this once with the base controller's __construct() method. So that's an example of accessing middleware in the base controller's __construct() method. There's a legitimate need to access the session at this point, too. For instance, my admins are required to use 2-factor auth. Customers are able to use 2factor optionally. Google's 2 factor library sets a session token when the user has entered the correct Authy code. In my case, I handle app-wide re-routing to the 2factor login page if a) the user is an admin or desires it and b) the session token, "Session::get('2factorToken')" is missing or null. Perhaps there's a better way to do this, but since the permissions logic resides in the base controller, the logical place to handle 2factor is here as well. There are more use cases I could list, but I think you get the idea. In my mind, the logical place for this common permissions logic is here in the base controller's __construct() method. I don't have to litter my code with method calls to $this->setUserPermissions() and I know those flags are all consistently available to any controller that extends my notional "FrontEndController" parent. These flags are fed into the "data" object which, in turn, is fed to each view like I've been taught since the dawn of MVC. The base controller seems to be the logical place to me since the http request has literally always been available throughout every framework I've ever worked on until 5.3. I never occurred to me that it should be otherwise. Perhaps that's the source of confusion here. |
|
So what exactly changed in 5.3? In 5.2 session was still middleware, but the Request was FULL (with session) in the __construct. So was 5.2 just done wrong, and now 5.3 is right? And you say "http requests and cookies and it's impossible to access those things until a request has actually entered your application"...but it did in 5.2. And looking at the $request object at the __construct level it has a huge concept of the request at that moment. Just need sessions back like in 5.2 dump($request) inside __construct on l5.3 So the request is available, even the cookie and session ids...session just not started in laravel yet as it once was in 5.2 |
|
So what does @JosephSilber think about all this then? |
|
I don't think that the change was bad, the only problem actually is that no simple migration path were provided (one that would allow us to migrate big projects without having to rewrite so much code), as soon as we get a viable one we won't be facing any issues and this process will work like a charm. Currently you do have to figure out pretty nasty workarounds though. |
|
I like this answer #15072 (comment) |
|
@axyr - While I, too, like Taylor's explanation of why it's bad to use session and request data in a constructor, he explains why it's currently bad to do so in a Laravel 5.3 constructor. I have yet to read a cogent defense of why it's bad in general to do so. Giving the benefit of the doubt, I'm sure they decided to structure 5.3 this way for a canonical, best-practices reason. That reason escapes me and I am seriously questioning if I've been doing something horribly wrong all these years. That's an unusual feeling for me. Anyway, let's hope the workaround Taylor et. al. have in mind helps bridge the gap from 5.2 to 5.3. Cheers. |
|
@sstringer agree!! |
|
@taylorotwell Imagine I have 10 controllers, each controller have a lot of functions, every controller's access depends on if the logged in user have the permissions to enter or not, (imagine a controller that controls the users, another that controls the blog, another that controls the roles, etc) I used to make use of the conscruct to check if the user had the permission to access that controller, now i can't no loger use it and I need to do it manually on each funcion of the controller EDIT: Just nocided there's something called gates, but i don't know if they work on the conscruct |
|
I had similar problem. Fix it with middleware class. In middleware (I use Sentinel for auth) In controller I don't know is it good way or not, but it's working for me :) |
|
I've submitted a pull request to allow registering closure middleware in the controller constructor. This is still different than 5.2, but it provides an easier migration path forward. |
|
@doctorawesome - thanks for this. However, I don't think it would work in my case. I'm trying to avoid having to call the same global method over and over and over again in each method of each page controller. Your approach definitely works well if I'm just calling "mymethod()" intermittently, though. |
|
Hi! Just my ten cents on this issue. So my use case was request validation that occurs for every single action that is invoked on a specific controller. The request validation was modelled on Laravel's The proposal to allow closure middleware to be registered in the constructor will allow me to work around this, so thanks for that. However, I'm not convinced this is a neat solution. It seems to me that there is a strong argument for a controller to have a hook on it that gets invoked before the controller handles an action, regardless of what that action is. This should allow dependencies to be injected into the hook just like the controller action methods allow dependency injection. Otherwise you have to type-hint the same dependency injection into every action method if you want to use the This isn't really an anti-pattern |
|
Actually having mulled this over my biggest problem at the moment is how my package maintains support for all three of 5.1, 5.2, 5.3. The controller closure middleware is only in 5.3 and I'm assuming there's no plans to back-port it. So I think the controller that I linked to above is going to have to be refactored so that it takes the request class name as the constructor argument, rather than an instance of the class. It'll then have to register the classname as controller middleware and I'll have to invoke the request validation I'm doing via this middleware. This is the only backwards compatible way I can think of refactoring at the moment without having to manually invoke the validation in every single controller action. @taylorotwell going forward, is there any appetite for before/after action hooks in a controller for implementing logic that is 100% coupled to that specific controller and must occur after middleware but before an action? I think this is a lot better pattern than pushing logic that is applicable to a single controller into middleware. If so I'd be happy to work on a pull request but don't want to spend the time doing that if it's not an acceptable change. |
|
We're using middleware rather than before and after callbacks. They're far more flexible and allow you to do the same things and more. |
|
Just to give my input on this matter also. I understand why Taylor made the change, yet I am not able to find a way to how to work around this. In my situation, I am building an app which is multi-tenant. A user can manage several clubs, so that's why I made a ClubRepository. In this class I am getting - multiple times in my app, so I made it a singleton class in the boot-method of AppServiceProvider - the current club and all the clubs of the current user logged in. In the controller I am injecting the repositories. Even with the Repositories being constructed with the middleware closure, I am not able to get it working. Because I am creating the ClubRepository in the AppServiceProvider, I need to use the Auth facade and SessionManager in there. Is there any way to write a solution around my approach or do I need to rewrite my whole app? 😢 Code: https://gist.github.com/Thijmen/1e3039ded96878e776fa4c1f889dbad8 |
Prevents sticking to bad practice and stages enhancement for L5.3 Discussion: laravel/framework#15072 (comment)
|
@Thijmen Have you tried using event's? That can also lead to the solution of your problem, for example for getting your current user you could: Event::listen(Authenticated::class, function ($event) {
$this->user = $event->user;
});Combining both the closure's and event's may solve your problem. |
|
Closing since this has been discussed and addressed. Feel free to ask questions on the forums. |
|
I know this issues is closed and everyone has provided many ways in which you can get around using I know this is a hack, but you can actually fire up the Session middleware early in your own service provider, which allows you to access I don't know the ramifications of this...again I agree you should find the proper work around/refactor..but or me, this tides me over until I can spend the months to refactor code. In one of your main ServiceProvider Now all your controllers __construct have |
|
@mreschke Thanks! I just spent 5 hours for this. |
|
For anyone else bumping into this issue: I've written up a few strategies for dealing with session data in controller constructors: https://josephsilber.com/posts/2017/01/23/getting-current-user-in-laravel-controller-constructor |
Finally..! it still work in Laravel 7 |
With Laravel 5.3, an undocumented breaking change was introduced where middleware are initialized after the controller class is constructed. This means app-critical middleware like
Auth--specificallyAuth::user()--are not available to the controller's__construct()method.With the deepest respect to the Laravel team, I ask that you please reconsider this decision in light of the fact that this change appears to have adversely affected many developers with legacy 5.2 apps.
Surely there is a compromise solution here. Perhaps you could create a backwards-compatibility flag that lets me trigger the base controller's construction after the middleware has been initialized, you will allow legacy apps to be upgraded to 5.3 with ease. The default could be what you have now in 5.3 so fresh apps can be built the "new," faster way. This would go a long way towards making your app developers happy.
I've been taught my whole object-oriented coding career that efficient code is written once and used often. I've also been taught that, when employing MVC architecture, logic goes in the Controller class, never in the View. If it's common logic to more than one method, it goes in the parent class, and if it's common logic that is called every time the class is instantiated, it goes in (or is triggered by) the
__construct()method. With that in mind, I would argue that allowing access to theAuth::user()object in the base controller's__construct()method is not only desirable but the most efficient methodology here. It's written once, easily tested, called automatically every time, and is available to the entire app.Regardless of whether you take issue with this approach, the reality is that many Laravel apps have been built around the ability to access
Auth::user()from the base controller's__constructor()and 5.3 breaks these apps.On behalf of those affected, my open request is that you provide a compromise solution quickly with a point upgrade. A flag that lets us optionally turn on a backwards-compatibility mode that processes middleware before the base class is constructed would allow those of us who did it the "old" way to upgrade with ease, while those who are building new apps on 5.3 can do it the "new" way by default. Perhaps there is a better way to help those of us stuck at 5.2, but please don't underestimate the amount of pain this upgrade has caused.
Many thanks for your time and consideration on this matter.
Regards,
Steve Stringer
The text was updated successfully, but these errors were encountered: