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 pagination in user components and com_users (with enabled filters) #8475

Closed
wants to merge 2 commits into from

Conversation

Disaron
Copy link

@Disaron Disaron commented Nov 18, 2015

Sometimes pagination in JModelList don`t work. Usually it happens when you are on first page and has no active filters (if it present) in frontend. Reason that list.start state is cleared in populateState method.

Second commit fix pagination in users list com_users when you activate some filters.

@joomla-cms-bot
Copy link

Please add more information to your issue. Without test instructions and/or any description we will close this issue within 4 weeks. Thanks.
This is an automated message from the J!Tracker Application.

@infograf768
Copy link
Member

Please first test #8467

@Disaron
Copy link
Author

Disaron commented Nov 18, 2015

infograf768, tested. No this is not helps in frontend (solved by 3aee886). It helps only for com_users users list, yes. But rows removed in 7bd23ff solves this too, and this rows are unnesesary because JModelList::populateState do it.

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

Can you provide exact steps to reproduce the issue you see?

The first commit almost certainly is wrong. The value for start should be cleaned for sure, removing that is no option as it would be a security issue allowing uncleaned input into the states.

The second commit looks wrong as well. The PR #8467 should indeed have fixed that.

@Disaron
Copy link
Author

Disaron commented Nov 18, 2015

First commit break my mind at two weeks after migration to 3.4.5. I think for this reasons populateState in com_content (articles list) in frontend totally overriden (has no parent::populateState).

Steps to reproduce:
Create custom model list with filtering fields in frontend, but don`t select any filters (limit also).
Fill some items to see pagination.
Select 1st page.
Drop session (logout or anything else, I have HTTP-SSO and automatically log in after logout but with new session), but not go from this page.
Login. (still on this page)
Try to go to 2nd page.
1st page still selected.

Well I see another solution (no time to create pull request now):

case 'start':
    $value = $inputFilter->clean($value, 'int');
    break;

For second commit I think that this commit and this lines says you why it possible ;) And yes, #8467 fix it too.

@mbabker
Copy link
Contributor

mbabker commented Nov 18, 2015

com_content had overridden the parent populateState method long before 3.4.5's release, the two topics are unrelated.

The line you've removed in JModelList actually is 100% irrelevant to processing. Even if you inject a value for the list.start model state via the request, it is overwritten by the limitstart query variable and its default value if it isn't sent. Even so, if it were the actual value used then list.start must be an integer given its use cases and that is exactly what is happening there.

@Disaron
Copy link
Author

Disaron commented Nov 18, 2015

Sorry but happening what? What do with start - nothing. $limit is assigned, $start goes to blackhole.
Current code in staging:

case 'limit':
case 'start':
    $limit = $inputFilter->clean($value, 'int');
    break;

$limit = - seriously? Maybe must be $value =? ;)
No it`s not, because
By now need to add separate option

case 'start':
    $value = $inputFilter->clean($value, 'int');
    break;

I understand that my first fix is incorrect, I do not insist to merge this PR because it's fix really wrong. But bug is present.

@Disaron
Copy link
Author

Disaron commented Nov 18, 2015

com_content had overridden the parent populateState method long before 3.4.5's release, the two topics are unrelated.

Then you must know, that in current release pagination in com_content goes to first page when you reopen list? Because list.start not save in states (maybe it's feature, I don't know...)

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

True, that part with the start and limit indeed looks wrong. Can you do a PR to fix that alone, without anything else and without removing filtering for the start.

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

Then you must know, that in current release pagination in com_content goes to first page when you reopen list? Because list.start not save in states (maybe it's feature, I don't know...)

That is indeed by design. The pagination isn't saved in the userstates in frontend views.

@Disaron
Copy link
Author

Disaron commented Nov 18, 2015

Done: #8481

@Bakual
Copy link
Contributor

Bakual commented Nov 18, 2015

Closing this one as we have a PR that deals only with the issue at hand

@Bakual Bakual closed this Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants