Remove Eloquent\Builder::chunkById() already having the correct id field in Laravel
#25
Conversation
| $count += count($items); | ||
| $names = []; | ||
| User::chunkById(2, function (EloquentCollection $items) use (&$names) { | ||
| $names = array_merge($names, $items->pluck('name')->all()); |
There was a problem hiding this comment.
Would it make sense to use += here for an array union? I think it'd have the same effect for a list, and we can safely assume these aren't associative arrays.
If you like, you could also add a duplicate name to the fixtures above to assert that it's appended instead of merged. I'll note that there is no such problem with array_merge() as written here (again, since lists are concatenated).
There was a problem hiding this comment.
The + doesn't work well with lists, as it behave like array_replace.
var_dump(['a', 'b'] + ['c']);
// Result: ['a', 'b']| /** | ||
| * @inheritdoc | ||
| */ | ||
| public function chunkById($count, callable $callback, $column = '_id', $alias = null) |
There was a problem hiding this comment.
I'm not familiar with chunkById(), but https://laravel.com/docs/10.x/queries#chunking-results states:
If you plan to update the retrieved records while chunking, it is always best to use the chunkById method instead. This method will automatically paginate the results based on the record's primary key:
Looking at the test below, I'm not sure where the primary key is being overwritten. How do we know an _id field isn't being created internally? Or does the User model in the test suite define name as a primary key mapped to the _id database field?
There was a problem hiding this comment.
The primary key is set in the model. _id is the default for MongoDB models.
This key is used in Laravel's chunkById.
|
Moved to mongodb/laravel-mongodb#2569 |
This method was necessary to rename the key name to
_id.chunkByIdwas refactored in laravel/framework#28065 to allow custom key name.Reverts mongodb/laravel-mongodb#1317