[8.x] Fix LazyCollection#unique() double enumeration #39041
Conversation
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
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 👍
| /** | ||
| * 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; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@RomanNebesnuyGC is your comment intentionally in this thread, or you meant to comment generally on the PR?
Can you paste the actual stack trace?
There was a problem hiding this comment.
@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)
.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.. )
driesvints
changed the title
|
@JosephSilber can you rebase with 8.x? I've re-enabled tests on 8.x after a small break from yesterday. |
JosephSilber
force-pushed
the
fix-lazy-unique
branch
from
53ea45d
to
f0ac775
Compare
Fixes #38817