-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[12.x] Fix chunked queries not honoring user-defined limits and offsets #52093
Conversation
* | ||
* @return mixed | ||
*/ | ||
public function getLimit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
/**
* Get the current "limit" value from the query or null if not set.
*/
public function getLimit() ?int
analog for all NEW function declarations. Dockblocks are not needed if you write the function definition with return type and typed params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just matching the same style that is there right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not in compliance with what laravel stated that starting with laravel 10, that they will implement typed declarations...
I see many new functions merged with old style codding...
Thanks! |
Changelog
getOffset()
andgetLimit()
)chunk
andchunkById
queries instead of silently overriding itchunkById
to silently skip the offset on every iteration (since it doesn't override it)Currently, the
chunk
method ignores any user-defined limits and offsets. ThechunkById
method ignores user-defined limits, but gets tripped up by user-defined offsets (it skips the offset amount of entries on every batch iteration).This PR ensures that user-defined limits and offsets are honored by chunked methods. For instance:
Given there are 7 entries in the database, this query would result in the following chunks distribution:
skipped
skipped
included
included
included
skipped
skipped
Warning
Anyone currently using
chunk
orchunkById
with limits and offsets (and not testing the behavior, I guess) could be affected by this fix.Context
I noticed chunked queries were ignoring user-defined limits while I was working on a legacy application that had a job doing something like this:
As I was covering this with a test like this:
I noticed the test was failing saying that 10 jobs were pushed instead of the expected 5.
I decided to dig in and found out the
each
method callschunk
behind the scenes (here), which calls theforPage()
helper, which overrides offset and limits for the query.After some searching, it looks like this was a known issue:
A fix hasn't been attempted in a while, so I figured it was about time for another one.
I've tested this patch in the application where I discovered it, and it works.
As I was looking for references in other frameworks, I found out that Rails' ActiveRecord also honors limits on their batched queries: