-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: QueryBuilder limit(0) bug #8280
Conversation
Um. Don't you think this has the potential to alter behavior in a major way for thousands of sites? As you said before this has always been this way in v4. And I am pretty sure I did not make any changes to the way this works when I ported the database layer over from 3. While I understand the reporter has a valid use case I think this is probably a situation where we should say sorry, we can't that. This is way too big of a breaking change. |
While there is a difference in handling In CI4 we simply ignore the |
1d8861b
to
533d27e
Compare
533d27e
to
d19bf0d
Compare
Thank you for the comments. I have modified the current implementation and checked to see if the tests passes, but the impact seems to be larger than expected, so I will be more careful in handling this issue. However, I think this bug should be fixed.
|
c035ef1
to
a1ed340
Compare
a1ed340
to
b0edcc7
Compare
24b2897
to
ce350fc
Compare
Added docs. |
2280d82
to
23c17cf
Compare
23c17cf
to
f65f95e
Compare
@codeigniter4/database-team Review, please. |
Just started wondering... will this change have any effect on pagination? |
I want to fix this behavior because I don't think it is a good idea to have QueryBuilder behave so differently from the behavior of the original SQL. |
I agree with your reasons. Well, the Pager class has tests and since they pass, I should not worry. |
The CodeIgniter4/system/BaseModel.php Line 1212 in 6c487a1
|
3eedb83
to
f65c787
Compare
Make implementation the same as CI3.
Co-authored-by: Michal Sniatala <michal@sniatala.pl>
f65c787
to
05645ce
Compare
There were other |
Do we reach a consensus here? The feature flag will be removed in v5, so this is a transitional solution. |
It is true that the code is more complicated with the addition of feature flag. I do not see these as major maintenance obstacles. |
Description
Fixes #8278
LIMIT
can be used in MySQL, SQLite3, PostgreSQL, and they return 0 rows when we specifyLIMIT 0
.limit()
is to limit the result rows. So when we specify it, the result should be limited, not be all rows.Checklist: