laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 31 forks source link

Pitfall: Auth::user()->with('company')->first() #2642

Open kw-pr opened 3 years ago

kw-pr commented 3 years ago

Today I tried Model::preventLazyLoading(! app()->isProduction()); and walked right into this pitfall.

I changed

$user = Auth::user();

to

$user = Auth::user()->with('company')->first();

While the first line returns the logged in user. The second line returns the first user in the database.

After I figured out my mistake I felt stupid about it, but I talked to some other developers and they all had the same WTF moment.

Well, if you like me did not realize the problem: with is a static function that is called on the already loaded user object. Like User::with('company') this creates a new query that has nothing to do with the user returned from Auth::user().

This actually a PHP "problem" as you always can call statics functions from the object.

I was wondering how this pitfall could be prevented and found a magic solution. As this is the real world I am not very happy with magic hacks to fix a problem.

What do you think about this? I am the only idiot and this does not need to be fixed or do you agree? Maybe even have a good solution?

rognales commented 3 years ago

How bout load() https://laravel.com/docs/8.x/eloquent-collections#method-load

kw-pr commented 3 years ago

Yes, I know. This works:

$user = Auth::user()->load('company');

Still $user->with('company') returns a unexpected result.

michaeldyrynda commented 3 years ago

Auth::user() returns the User model for the currently authenticated user, not a query builder instance.

Running Auth::user()->with('company') is akin to running something like User::firstWhere('id', $id)->with('company').

Chaining first() on a model instance is essentially the same as calling User::first().

As above, if you already have a model instance, the appropriate course of action is to call ->load('company') on it, as eager loading works only on the Eloquent query builder. Note that calling load() won’t be prevented by preventLazyLoading() as - although lazy - it is still considered eager loading.

The lazy loading being prevented by that functionality would be calling Auth::user()->company, which would otherwise query the database if the relationship was not already loaded.

kw-pr commented 3 years ago

Auth::user()->with('company') does nothing with the $id. It is a static method and can not access the object at all. That is kind of my point.

I am not looking for a method to load my relationship. load() works fine.

I am complaining that the code does not what it looks looks like from a clean code perspective.

$user->with('company') should load the relations (as it looks like) or in this case maybe better throw an exception. Calling the query builder on the object is wrong. This only works because of a PHP forgives everything quirk. It is a static method. It has nothing to do with the object.