[Proposal] Throwing exception upon negative chunk size in Builder

[jstoone]

• Laravel Version: 5.3.22
• PHP Version: 7.0.12
• Database Driver & Version: mysql v5.7.16

Description:

I made the following PR a while ago, and @vlakoff mentioned applying the same logic to the Query\Builder and Eloquent\Builder chunk() and chunkById().

• PR: [5.3] Return early when chunk size is less than zero #16206

I then see that @taylorotwell has removed the exception part here, 46ebd7f, so wanted to throw this out there before creating a PR with the solution below, or better yet something else if anything seems more appropriate.

Steps To Reproduce:

// using Query\Builder
DB::table('users')->chunk(-1, function(){});

// usingEloquent\Builder
User::query()->chunk(-1, function(){});

Produces:

Illuminate\Database\QueryException with message 'SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '0' at line 1 (SQL: select * from `users` offset 0)'

Solution

Throwing a proper InvalidArgumentException when $count < 0, I think would be a good solution. Please do tell what you think

[GrahamCampbell]

Seems fine, but we don't process proposals in this issue tracker.

[vlakoff]

I suppose in such cases Taylor prefer to discard/skip rather than produce exception. You can also find this in Query Builder offset() and limit() setters.

So for database chunk methods, we just have to do nothing if chunk <= 0.

I was planning to work on laravel/ideas#103, I'll have to keep this in mind.

[vlakoff]

Honestly it quickly becomes a mess, considering:

• each entry point in the hierarchy "chunk/paginate > forPage > skip/take" has to be thought of
• mysql requires a limit if there is an offset
• magic skip/take setters that silently discard/modify values
• query builder is mutable, so all the skip/take magic can have later impact

Couldn't people just pass correct arguments?

As a compromise, maybe we could support "0" values, but I think supporting "<0" values would have an horrible pain/value ratio. Please open an issue in internals for further discussion if you wish.

[jstoone]

@vlakoff i see your point. right now Eloquent\Builder and Query\Builder supports '0' values. So I think we should just let this be.