Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Handle duplicates method in eloquent collections #28194

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
2 participants
@timacdonald
Copy link
Contributor

commented Apr 11, 2019

This PR handles the new $collection->duplicates() method for eloquent collections.

The eloquent collection method compares entities with the $a->is($b) comparison. If you think it should just compare keys $a->getKey() === $b->getKey() than I can switch it over. The only time I see this being an issue is if a collection contains two models with the same key from different tables or something like that - which I can't imagine happening - but is something to consider.

Note that the eloquent method does not alter functionality with strict mode. I've written more about this below. It currently matches how unique() method works: #28193

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Here is a bit more info on the strict / loose comparison and why: When it comes to eloquent models how should comparison work. Should eloquent models be treated differently? Because as it stands, they are.

The existing unique method bypasses strict comparison and is solely based on the keys. So if you have two different instances of a model, both with the same key...

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->unique($key = null, $strict = true);

Doing this will result in a collection that only contains $three. Which raises the question - how do we strict compare models - but also points out how Laravel currently strict compares models.

Being that unique and duplicates share a similar problem space, I think it makes sense to match existing comparison functionality.

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->duplicates($key = null, $strict = true);

Which results in a collection containing $two and $three.

If you ran unique and got the above result, and then ran duplicates and got an empty collection - those two are conflicting results for a similar problem. The following illustrates how you would end up with a collection with one unique item, but no duplicates...

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->unique($key = null, $strict = true);
// [$three]

$users->duplicates($key = null, $strict = true);
// []

I think it makes sense to keep these methods in sync. If a strict comparison for unique was to be introduced, which I am personally not against, you could update this as well to match.

This illustrates the considerations you have to make when comparing eloquent models...

$userA = User::find(1);
$userA->someAttribute = 'hello';

$userB = User::find(1);
$userB->someAttribute = 'world';


$userA->getKey() === $userB->getKey(); // true

$userA->is($userB); // true

$userA == $userB; // false

$userA === $userB; // false
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

So is this ready to merge or you are still unsure regarding the things you mentioned above?

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I’m happy with it as is @taylorotwell

I think any strict / loose comparison change is a different conversation / PR which would target 5.9 as it would change existing behaviour for the unique method as well.

@taylorotwell taylorotwell merged commit 4b9d3e7 into laravel:5.8 Apr 12, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.