An Open Letter to the Core Laravel Developer Team [5.3 upgrade breaks Auth::user() in the base controller]

[sstringer]

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--specifically Auth::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 the Auth::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

[GrahamCampbell]

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.

[GrahamCampbell]

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.

[sstringer]

@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.

[GrahamCampbell]

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.

[mreschke]

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?

[GrahamCampbell]

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.

[GrahamCampbell]

So, if the app were to continue running, the 2nd instantiation would work just the same as in 5.2.

[sstringer]

@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.

[taylorotwell]

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.

[taylorotwell]

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.

[KennedyTedesco]

@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

[taylorotwell]

There is an alternative solution we could consider but need to see how others were using this behavior first.

[mreschke]

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 dump('x') show up multiple times? Even a file_put_contents('/tmp/x', 'here', FILE_APPEND); appends only once. And if that is true, __construct should be ultra light now as they are called many times, might want to document as that will cause performance issues for poorly designed __construct.

[sstringer]

@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,
Steve

[mreschke]

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 ?dealer=1234 parameter to define the dealer (car dealership) the dashboard is displaying.

Example

/dashboard/entry1?dealer=1234
dashboard/entry2?dealer=4567

With our system, we always REMEMBER the last selected dealer. So in my master Controller.php __construct I would check Input::has('dealer') if so, update the Session.

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.