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

Regression: We can't remove the URL-Parameter "limitstart" #4488

Closed
wants to merge 1 commit into from

Conversation

@Bakual
Copy link
Contributor

commented Oct 8, 2014

Issue

In some extensions the user can't get back to the first page once he navigated to a later page.
To my knowledge this doesn't happen in core extensions but other extensions have this reported.
It only happens when SEF URLs are enabled.

Explanation

If an extension uses JModelList::populateState(), the start of the pagination (limitstart) is stored in the session (userstate). That means the value is only changed if the limitstart parameter is present in the request. Due to a recent change (#3725) the limitstart was removed from the request when it was equal to zero. While this works for some extensions that don't use the userstates (like the core extensions), it broke pagination for the extensions which use those session states.

Solution

This PR just reverts the faulty PR and restores the limitstart parameter.

Testing

As this doesn't happen with core extensions, you need to use a 3rd party extension which is affected. You can use my own "SermonSpeaker" to do that. Either install it using the webinstaller or get it from http://www.sermonspeaker.net/download/sermonspeaker-component/sermonspeaker-component-5-2-3.html.
Create multiple sermons (batch copy the existing example sermon) and a menu item and test the pagination.
Make sure you have SEF URLs turned on as non-SEF URLs are not affected.

… value is 0). Closes #3725 (reverted from commit 4c7cd15)
@garkell

This comment has been minimized.

Copy link

commented Oct 9, 2014

Tested on a fresh install of 3.3.6 with limit set to 5. 10 articles - tested for features listing and category listings with pagination and all worked fine. Installed my component with parent::populateState in the model and pagination worked fine as well. Thanks again. Cheers.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4488.

@betweenbrain

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

@test

Was able to reproduce the issue with @Bakual's extension and the patch does resolve the issue of not being able to navigate to the first page.

@810

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

@test I have tested it with kunena, it works.

Results without patch:

Page 1: forum/recent
Page 2: forum/recent?start=5

You can't navigate back to first page.

Results with patch:

Page 1: forum/recent?limitstart=0
Page 2: forum/recent?start=5

You can navigate on all pages

@Lavsteph

This comment has been minimized.

Copy link

commented Oct 10, 2014

@test it's all right for me with Kunena

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4488.

@Bakual Bakual added the RTC label Oct 10, 2014
@Bakual

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2014

Setting to RTC as we have three tests.

@Bakual Bakual closed this in f450b30 Oct 10, 2014
@Bakual Bakual deleted the Bakual:RevertLimitstartRemoval branch Oct 10, 2014
@mbabker mbabker added this to the Joomla! 3.3.7 milestone Oct 11, 2014
rdeutz added a commit to rdeutz/joomla-cms that referenced this pull request Oct 24, 2014
@mbabker mbabker modified the milestones: Joomla! 3.3.7, Joomla! 3.4.0 Nov 22, 2014
@Hackwar

This comment has been minimized.

Copy link
Member

commented Dec 8, 2014

This "fix" means, that now the first page of every list has 2 URLs. "whatever" and "whatever?limitstart=0". At the same time, limitstart was always renamed to start in our URLs. please revert this. It is not the solution to the problem. The problem is in the code in JModelList::populateState().

@Bakual

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2014

This "fix" is a revert of a regression. It reinstates what was there before the original PR (#3725) broke it. I'm not going to revert this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.