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

[fix] fixing limit.start state in JModelList #8481

Merged
merged 1 commit into from
Nov 19, 2015

Conversation

Disaron
Copy link

@Disaron Disaron commented Nov 18, 2015

Fixing pagination bug according to this

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

Setting RTC based on review.

@Bakual Bakual added this to the Joomla! 3.5.0 milestone Nov 18, 2015
@Bakual Bakual added the RTC This Pull Request is Ready To Commit label Nov 18, 2015
@rdeutz
Copy link
Contributor

rdeutz commented Nov 18, 2015

Not sure if that is right. If you have SEF on then start is used and when off then limitstart. So I think we need to test that, wouldn't merge that on review

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

Current code is wrong for sure. Doesn't make sense to take the "start" value and save it into the $limit variable.
limitstart is a proxy to start as far as I know. The router does rewrite that somewhere if I remember right. If not, we would need an additionally case limitstart which could be added to the start one. But it would be a different issue unrelated to this PR.

@rdeutz
Copy link
Contributor

rdeutz commented Nov 19, 2015

But if we don't set $limit it is always 0 so $limitstart is also 0, doesn't makes sense either.

@Disaron
Copy link
Author

Disaron commented Nov 19, 2015

But if we don't set $limit it is always 0 so $limitstart is also 0, doesn't makes sense either.

But we set $limit, this fix set $start too. For what reason we set $start to $limit ("warm" vs "soft")?

@Disaron
Copy link
Author

Disaron commented Nov 19, 2015

For example: we have list.limit = 20 and list.start = 0. We set in url ?start=20.
Not fixed code must do this:
list.start = 0 (because we don't assign list.start)
list.limit = 20 (because we assign start to $limit)
Finally we have first 20 records instead of second 20.

@rdeutz
Copy link
Contributor

rdeutz commented Nov 19, 2015

@Disaron it is not that there isn't a problem to fix, but around this part of the code looks odd and doesn't makes sense at all

@Disaron
Copy link
Author

Disaron commented Nov 19, 2015

@rdeutz however it fixes bug described here
And base code is really really strange and not logical.
IMHO not logical code must be refactored or as minimum commented 😄

rdeutz added a commit that referenced this pull request Nov 19, 2015
[fix] fixing limit.start state in JModelList
@rdeutz rdeutz merged commit 7071e94 into joomla:staging Nov 19, 2015
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 19, 2015
@Disaron Disaron deleted the fix_start branch November 19, 2015 11:25
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.

None yet

4 participants