[10.x] Optimize eager loading when no keys to be loaded

[garygreen]

Sending this again. @taylorotwell please could you provide suggestions on what I can do to get this merged?

Currently when eager loading relations it causes many impossible wheres to be executed, slowing down application load time. The reason for that is mentioned in the original PR - Laravel will add an impossible where 0 = 1 which would never return a result for an eagerly loaded relation. This PR fixes those eager loading scenarios, which are very common with NULL column values.

If you could provide alternative suggestions on how to get this merged that would be great.

[taylorotwell]

I'm curious if there is a reason this was sent to 10.x instead of 9.x?

[garygreen]

The reason I sent it to 10.x is because it potentially poses a behavioural change, but only for a strange edge case scenario when adding constraints to the eager load:

$user->load(['picture' => function($q) {
	$q->orWhere('id', 'some other id')->orWhere('id', 'another id');
}]);

In 9.x this currently results in this query:

Which could return results, but with this PR it wouldn't return any results because the model didn't have any loadable relation keys. I would say the latter is expected, and you could say this is actually an unintended bug in Laravel at the moment. Personally I would expect a query more like the below where it should restrict loading of model to that particular set of keys being loaded - i.e. the constraints is ADDITIONAL as oppose to a complete override:

select * from `pictures` where 0 = 1 and (`id` = 'some other id' or `id` = 'another id')

To be on the safe side it was sent to 10.x even though this shouldn't affect anything because when the eagerly loaded models attempt to be set it won't match to the dictionary of relation keys, as is the case with BelongsTo

[Tofandel]

What you describe is a bug, the query should definitely not load entities that don't pertain to that model (especially if the relation is marked as null in the first place), it could in fact be a security issue as that could load data from other users unexpectedly

Normally closures are added as subQueries to prevent wrong operator priority, this doesn't seem to be the case here

It should be sent to 9.X or 8.X imo

[garygreen]

What you describe is a bug [...] It should be sent to 9.X or 8.X imo

Agreed, I think it's a bug as well although this PR doesn't really intend to fix that. It technically does fix it for eager loading when all relations are null but that's just because it doesn't need to hit the database at all, so doesn't even attempt to build a query. For any others it would still allow a query like this:

select * from `pictures` where in (15, 88, 99) or `id` = 'some other id' or `id` = 'another id'

...because there are loadable model keys, so it go onto hitting the database constructing the query as above. Solving that should be a separate PR which wraps those additional eager constraints in a parentheses group.

[Tofandel]

I see, I thought you fixed it for all cases, then yes there should be a new PR to fix this issue, but then this PR is still not breaking any expected behavior

[JurianArie]

Is it intentional that once this property is true that it'll always be true?

Suggested change

	if ($modelKeys === []) {
	$this->eagerImplicitlyEmpty = true;
	}
	$this->eagerImplicitlyEmpty = $modelKeys === [];

[garygreen]

Well if the eager is loaded again it'll make calls to the relation methods addEagerConstraints which subsequently call whereInEager which could change the eagerImplicitlyEmpty property because it will know if there are any loadable keys. So once the eager constraints are added, it will set that property.