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.

Sign up for GitHub

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

Jump to bottom

[8.x] Fix LazyCollection#unique() double enumeration #39041

Merged
merged 1 commit into from
Sep 30, 2021
Merged

[8.x] Fix LazyCollection#unique() double enumeration #39041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Illuminate/Collections/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,6 +1411,28 @@ public function transform(callable $callback)
return $this;
}

/**
* Return only unique items from the collection array.
*
* @param string|callable|null $key
* @param bool $strict
* @return static
*/
public function unique($key = null, $strict = false)
{
$callback = $this->valueRetriever($key);

$exists = [];

return $this->reject(function ($item, $key) use ($callback, $strict, &$exists) {
if (in_array($id = $callback($item, $key), $exists, $strict)) {
return true;
}

$exists[] = $id;
});
}
Comment on lines +1421 to +1434
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to explain why this didn't work:

The call to $this->reject returned a new LazyCollection instance. Every time the collection is enumerated, the callback (that was passed to reject) gets called again, but it's reusing the same $exists cache of found items, so anything that was previously enumerated is treated as a duplicate 😦

The solution is to have a separate implementation of the unique() method for the LazyCollection class. In that implementation, we can move the $exists cache into the Generator function passed to the constructor, so that each enumeration has its own cache 👍


/**
* Reset the keys on the underlying array.
*
Expand Down
24 changes: 24 additions & 0 deletions src/Illuminate/Collections/LazyCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,30 @@ public function tapEach(callable $callback)
});
}

/**
* Return only unique items from the collection array.
*
* @param string|callable|null $key
* @param bool $strict
* @return static
*/
public function unique($key = null, $strict = false)
{
$callback = $this->valueRetriever($key);

return new static(function () use ($callback, $strict) {
$exists = [];

foreach ($this as $key => $item) {
if (! in_array($id = $callback($item, $key), $exists, $strict)) {
yield $key => $item;

$exists[] = $id;
}
}
});
}

/**
* Reset the keys on the underlying array.
*
Expand Down
22 changes: 0 additions & 22 deletions src/Illuminate/Collections/Traits/EnumeratesValues.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,28 +773,6 @@ public function reject($callback = true)
});
}

/**
* Return only unique items from the collection array.
*
* @param string|callable|null $key
* @param bool $strict
* @return static
*/
public function unique($key = null, $strict = false)
{
$callback = $this->valueRetriever($key);

$exists = [];

return $this->reject(function ($item, $key) use ($callback, $strict, &$exists) {
if (in_array($id = $callback($item, $key), $exists, $strict)) {
return true;
}

$exists[] = $id;
});
}

Comment on lines -776 to -797
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider this a breaking change.

Maybe you can only add the unique Method to the Lazy Collection and let the normal Collection stay the same with using the EnumeratesValues Trait.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EnumeratesValues trait was never meant to be consumed by the public. It's an internal trait that's only there to house the shared code between the two collection classes. I can't really believe anyone is using this trait in the wild.

I'll let Taylor decide whether he considers this a breaking change.

Copy link

@RomanNebesnuyGC RomanNebesnuyGC Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Laravel Nova global search is not working anymore because of this fix...

Class Illuminate\Support\LazyCollection contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Illuminate\Support\Enumerable::unique)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomanNebesnuyGC is your comment intentionally in this thread, or you meant to comment generally on the PR?

Can you paste the actual stack trace?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JosephSilber
Yes my comment only for this thread, because if I will back this part of code all is working fine.

Symfony\Component\ErrorHandler\Error\FatalError
Class Illuminate\Support\LazyCollection contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Illuminate\Support\Enumerable::unique)

it's FatalError
Laravel Nova use ->cursor() in global search (vendor/laravel/nova/src/GlobalSearch.php@60)
Снимок экрана 2021-10-08 в 19 54 37
.

So it's globally broken. Everywhere when you want to use ->cursor() in Eloquent it will be this FatalError.

Laravel: 8.63.0 (on 8.62.0 all is ok)
PHP: 7.4.23 & 8.0.10
Laravel Nova: 3.29.0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a sample repository (with a readme) showing that the cursor() method is not broken.

@RomanNebesnuyGC Can you please create a PR to that repository that shows how it's broken?

Copy link

@RomanNebesnuyGC RomanNebesnuyGC Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JosephSilber sorry all is ok. It's my fault. All is working perfectly.
In our project we use a lot of extensions (something like https://github.com/nWidart/laravel-modules) and they have in composer required illuminate/support that required illuminate/collections and I need only to update all extensions. Problem was that core composer was with illuminate/support 8.63, but extensions on 8.62.. )

/**
* Return only unique items from the collection array using strict comparison.
*
Expand Down
9 changes: 9 additions & 0 deletions tests/Support/SupportLazyCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,13 @@ public function testTapEach()
$this->assertSame([1, 2, 3, 4, 5], $data);
$this->assertSame([1, 2, 3, 4, 5], $tapped);
}

public function testUniqueDoubleEnumeration()
{
$data = LazyCollection::times(2)->unique();

$data->all();

$this->assertSame([1, 2], $data->all());
}
}