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.3] Return early when chunk size is less than zero #16206

Merged
merged 5 commits into from
Nov 1, 2016

Conversation

jstoone
Copy link
Contributor

@jstoone jstoone commented Nov 1, 2016

I found that this PR might be relevant after working on spatie/laravel-collection-macros#13, and figured it might be relevant for the core. It is very much an edge-case.

The problem

Many use chunk() as a way to split their collection into parts, so that it's easier to create columns of content. But if the $size is dynamically calculated, it may occur that the result of the calculation is 0.
If array_chunk() is given a value less than 1, it will throw an error. This will, from my point of view, generate a lot of unnecessary empty checks in peoples views.

The solution

If the chunk() method would return early if the given chunk size was == 0,

@@ -955,6 +955,10 @@ public function split($numberOfGroups)
*/
public function chunk($size)
{
if ($size == 0) {
return $this;
Copy link
Member

Choose a reason for hiding this comment

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

You should return a new instance, not the same one please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It now returns a new instance instead.

@taylorotwell
Copy link
Member

Honestly to me it should throw an error? If you're passing 0 into chunk it definitely shouldn't return the current collection. That doesn't make any sense to me. If anything it should return an empty new collection as Graham said.

@jstoone
Copy link
Contributor Author

jstoone commented Nov 1, 2016

I totally see your point(s).

In either case I think it would be nice to see it handled in another way. It's not always directly obvious when getting an exception/error from array_chunk.

I'll change the comments mentioned in the review by @GrahamCampbell. If you feel like throwing an error @taylorotwell, I think it would be more appropriate to message that the error is from within Collection::chunk.

I hope that makes sense.

@@ -955,6 +955,10 @@ public function split($numberOfGroups)
*/
public function chunk($size)
{
if ($size == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

We should throw an invalid argument exception for less than 0 also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has now been added, together with a test.

@taylorotwell taylorotwell merged commit 5e6de19 into laravel:5.3 Nov 1, 2016
@jstoone jstoone deleted the fix-empty-collection-chunk branch November 1, 2016 18:59
@vlakoff
Copy link
Contributor

vlakoff commented Nov 2, 2016

Maybe a similar change could be applied to Query\Builder and Eloquent\Builder's chunk and chunkById methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants