[5.5] Method load($relation) in Eloquent\Collection should use newQueryWithoutRelationships as the one in Eloquent\Model

[salexi]

Iluminate\Database\Eloquent\Collection

public function load($relations)

should be aligned with the same method from

lluminate\Database\Eloquent\Model

by in order to prevent lazy eager loading to load again already loaded relations. This can be accomplished by using newQueryWithoutRelationships instead of newQuery

This fix complements commit a0e3a02

[sisve]

This sounds like a change in behavior and should target 5.6.

[salexi]

On the other hand commit a0e3a02 which introduced this behaviour was introduced in 5.5.18, and if you ask me it fixes a bug. Since you already have relations loaded by declaring them in the with property of the model, it's highly unlikely that you want them loaded again when you want to lazy eager load another relation on the collection. It just makes duplicates queries and slows everything down.

[sisve]

The linked commit is an example of the total lack of QA we have at the moment. There was no discussion and no thought about backward compatibility. The actual PR is at #21710 and shows that the community has just above two hours to point out problems before it was merged.

I am just pointing out that a change in behavior should target the 5.6 release. It's the sane approach to keeping backward compatibility in the 5.5 release lifecycle. The fact that a sane approach is ignored is a separate QA problem.

[sisve]

I would like to add that I am not saying that you've done anything wrong. The development process in Laravel isn't totally clear. It's fully possible that this is merged to be in sync of the behavior of the previously merged PR.

I'm a bit disappointed that the previous PR didn't attempt to change both models and collections, making these implementations become out-of-sync. I'm a bit disappointed that there were only 2 hours to give input on it. I'm a bit disappointed that there wasn't any talk about backward compatibility in the previous PR.

Most of my disappointment in this process is now surfacing in your PR, but please don't take it personally. We're now stuck in a weird catch-22 where this PR changes behavior so I think it should target 5.6, but it's also an expansion on a fix that has already been merged into 5.5. There is no obvious solution except saying "okay, we've learned something from this"... but we never seem to learn anything.

[salexi]

Totally agree on that.

[themsaid]

Thanks @salexi, I should have changed the load method of the Eloquent\Collection in my PR, I completely forgot it existed :)

As for the question if this is a breaking change or not, I don't believe it is, the loaded relationships are still loaded with this change, you call ->load('relationship') to load an extra relation to your model, the extra queries were definitely an unwanted behaviour.

[sisve]

If a model already has a filtered relation loaded, was that relation kept filtered after a call to $collection->load before this change? Is it after this change?

$collectionOfBooks->load(array(
    'authors' => function($q) { $q->where('something', '=', 'something'); }
));

// Would this keep a filtered authors relation, or would it load all authors?
$collectionOfBooks->load('publishers');    

[themsaid]

@sisve what is the correct behaviour in the case you shared? Have you tested this scenario and found a difference? is that difference towards the expected results or not? This all greatly helps us identify if the change is breaking or not, a feedback like this helps.

[sisve]

I'm not trying to argue what is "correct", but highlight that this is probably changing the behavior in already released code. It wouldn't help if we call the old behavior "wrong"; it's there, it's released, and people may have written code that depends on it. So, if merged, it needs to be documented in the changelog appropriately.

[themsaid]

@sisve So all changes/fixes should be submitted to be released in the next release? sometimes there's a thin line between a fix and a behaviour that should be changed, it's quite hard for us to identify that line without multiple developers giving feedback, a feedback that helps includes an example that proves this change would break another case, once we have this kind of feedback we move the change to the next release, or if it was already merged we revert it rightaway.

[sisve]

My argument was primarily that this is a change in behavior, and should be documented as such.

Discussing things like "all changes/fixes" will not be productive, because it opens up for someone blindly going "I want to change the spelling of a copy error, I cannot do that, your methodology is wrong". There has to be some kind of definition of the words, and boundary of what and when they are applied. I believe that there are many people that have input on QA and release management in Laravel, and that it needs to be discussed in the open, but that the comments on a PR named "Method load..." isn't the obvious place to have that discussion.

I am not against that this specific issue is merged. I was initially against having this target 5.5, but that was before I was informed that this is just a "second half" of the functionality, and the first half is already merged and released. We just need to be open with that it is a change in behavior, and so was the previous half of it, and that was something a working QA process hopefully would have found out. But we're past that point, and now have to consider api consistency to not surprise developers where $model-> works differently than $collection->load.

[Miguel-Serejo]

As I recall, this was originally changed because models that defined a $with or $withCount property were loading those relationships again on every call to load(), while any relationship loaded through the with() or load() methods were not reloaded. Also, loadMissing() was loading $with relationships every time.

I suppose there may be an edge case out there where database values change during script execution and if someone is lazy eager loading a relationship after checking the values of a relationship that was eager loaded because of the $with property, and then checking the values of that relationship after lazy loading other relationships, expecting them to have changed, they would no longer see those changes.

Something like this:

//posts is defined with $with = ['comments'];
$posts = Posts::all();
foreach($posts->pluck('comments') as $comment) 
  // call an external service that may alter comments in the database
$posts->load('tags'); //previously this would reload the comments as well, reflecting changes made by the exteral service
return view('posts.index')->withPosts($posts);

It's a bit contrived, and is a great example of how not to do things, but it is a change in behavior in at least one scenario, as most bug fixes are.

That said, I see this as a bug. I don't expect relations to behave differently based on where or how they were loaded ($with property vs with()/load() methods) and I don't expect a relation to be reloaded when I load a different relation, so I would like to see this merged as well.

As a side note RE @sisve

The actual PR is at #21710 and shows that the community has just above two hours to point out problems before it was merged.

This is true, but the issue that the PR fixed was open for a few days: #21653
I believe this was considered a bug fix as opposed to a change in behavior.

[LasseRafn]

How would reloading related data work with this change? Just wondering

[salexi]

@LasseRafn we don't have a method for reloading from what i've seen, neither on collection or on model, but we could simply call load($relations) with all relations that we want to reload.
load() doesn't prevent us from reloading relations, but it will not automatically reload the relations defined in protected $with unless they are explicitly passed via $relations.

[LasseRafn]

Oookay now I get it. I thought that this prevented us from using load twice, because that would be a huge breaking change for a few of our applications. Thanks for clarifying :)