Skip to content
This repository was archived by the owner on Aug 22, 2023. It is now read-only.

Remove Eloquent\Builder::chunkById() already having the correct id field in Laravel #25

Closed
wants to merge 1 commit into from
Closed

Remove Eloquent\Builder::chunkById() already having the correct id field in Laravel #25

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
8 changes: 0 additions & 8 deletions src/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,6 @@ public function decrement($column, $amount = 1, array $extra = [])
return parent::decrement($column, $amount, $extra);
}

/**
* @inheritdoc
*/
public function chunkById($count, callable $callback, $column = '_id', $alias = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Owner Author

@GromNaN GromNaN Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The primary key is set in the model. _id is the default for MongoDB models.

This key is used in Laravel's chunkById.

{
return parent::chunkById($count, $callback, $column, $alias);
}

/**
* @inheritdoc
*/
Expand Down
8 changes: 4 additions & 4 deletions tests/ModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,12 +834,12 @@ public function testChunkById(): void
User::create(['name' => 'spork', 'tags' => ['sharp', 'pointy', 'round', 'bowl']]);
User::create(['name' => 'spoon', 'tags' => ['round', 'bowl']]);

$count = 0;
User::chunkById(2, function (EloquentCollection $items) use (&$count) {
$count += count($items);
$names = [];
User::chunkById(2, function (EloquentCollection $items) use (&$names) {
$names = array_merge($names, $items->pluck('name')->all());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Owner Author

Choose a reason for hiding this comment

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

The + doesn't work well with lists, as it behave like array_replace.

var_dump(['a', 'b'] + ['c']);
// Result: ['a', 'b']

});

$this->assertEquals(3, $count);
$this->assertEquals(['fork', 'spork', 'spoon'], $names);
}

public function testTruncateModel()
Expand Down