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

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.

@mreschke Thanks! I just spent 5 hours for this.

class AppServiceProvider extends ServiceProvider
{
    /**
     * Bootstrap any application services.
     *
     * @return void
     */
    public function boot()
    {        
	$kernel = app()->make('Illuminate\Contracts\Http\Kernel');
        $kernel->pushMiddleware(\App\Http\Middleware\EncryptCookies::class);
        $kernel->pushMiddleware(\Illuminate\Session\Middleware\StartSession::class);
    }
...
}

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.

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

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

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.

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

I know this issues is closed and everyone has provided many ways in which you can get around using Auth inside your controllers __construct(). For me, 30+ apps with hundreds of controllers and Auth issues which will take MONTHS to work around in some way.

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 Auth in your controllers __construct() as normal.

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 boot() methods, just tell the middleware to load early with

$kernel->pushMiddleware(\App\Http\Middleware\EncryptCookies::class);
$kernel->pushMiddleware(\Illuminate\Session\Middleware\StartSession::class);

Now all your controllers __construct have Auth as usual. Again, this is an unwise hack…just an fyi.

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.

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.

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

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.

@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

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 FormRequest but was injected into the controller constructor because it was specific to that controller and had to occur regardless of what action was being invoked. That allowed me to compose controllers like this: https://github.com/cloudcreativity/demo-laravel-json-api/blob/master/app/Http/Controllers/Api/PostsController.php

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 ValidatesWhenResolved contract. And to do that removes the ability for packages like mine to do all the work on handling repetitive processing of HTTP requests.

This isn’t really an anti-pattern boot, it’s a hook on a controller to allow it to do pre-action work that is specific to that controller - and therefore doesn’t fit as middleware because it’s entirely coupled to the specific controller.

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

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? I think that rails works this way with beforeAction and afterAction, also we could also accept closures there.

Edit Also the Middleware closure may solve things out 👍

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.

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.

@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

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.

@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