Add an option in Pagination class to generate URL without limitstart=0 by csthomas · Pull Request #19467 · joomla/joomla-cms

csthomas · 2018-01-27T00:25:37Z

Summary of Changes

SEO impovement for views with pagination.

Add an option to not generate/build URL with empty limitstart - &limitstart=0

I added a few changes in com_content component to show how it can work.

Changes in pagebreak plugin:

remove empty &showall= - if someone will see a problem then I will revert it.
remove empty &limitstart=

Testing Instructions

I encourage to review the code. Any improvement is welcome.

You can test a new pagination option on article/category/featured/archive views.

Expected result

No more &limitstart=0 or &start=0 in com_content component

Documentation Changes Required

Every 3rd party extension can enable this feature in model or view by adding:

// Flag indicates to not add `&limitstart=` or `&limitstart=0` to URL
$pagination->hideEmptyLimitstart = true;

Bakual · 2018-01-27T09:17:14Z

libraries/src/Pagination/Pagination.php

+	 * @var    boolean  The flag indicates whether to add limitstart=0 to URL
+	 * @since  __DEPLOY_VERSION__
+	 */
+	public $append_empty_limitstart = true;


Please don't use underscores for property names. Use studly caps (eg appendEmptyLimitstart) like the property above.

Also it may be just me but I always find it a bit contra-intuitive if I have to set a "positive" action ("append") to false to change a default behavior. Why not change the logic and name it "hideEmptyLimitstart" and set it to true if you want to change the behavior. Looks more intuitive to me. But that's probably highly subjective.

Yes, I was not sure how to call it, I will change it. I also tried to use a setter but simple public variable is easier.

Bakual · 2018-01-27T12:53:04Z

libraries/src/Pagination/Pagination.php

 	 * @since  __DEPLOY_VERSION__
 	 */
-	public $append_empty_limitstart = true;
+	public $hideEmptyLimitstart = true;


I think that should be "false" (or null) here.

Yes, I forgot to change it.

csthomas · 2018-01-27T13:15:32Z

Now all should work as expected without any B/C break.

Affected views:

com_content/category
com_content/featured
com_content/article (plugin pagebreak)
com_content/archive
com_contact/category
com_contact/featured
com_newsfeeds/category
com_search/search
com_finder/search
com_tags/tags
com_tags/tag

Every 3rd party extension can enable that option in model or view by adding:

// Flag indicates to not add limitstart=0 to URL
$pagination->hideEmptyLimitstart = true;

Quy · 2018-01-27T21:09:35Z

Please apply to Tagged items.

csthomas · 2018-01-27T21:40:04Z

Done

Quy · 2018-01-28T00:11:54Z

Notice: Undefined variable: page_prev in \plugins\content\pagebreak\pagebreak.php on line 374

ggppdk · 2018-01-28T06:17:17Z

plugins/content/pagebreak/pagebreak.php

-			$link_prev = JRoute::_(ContentHelperRoute::getArticleRoute($row->slug, $row->catid, $row->language) . '&showall=&limitstart=' . $page_prev);
+			if ($page > 1)
+			{
+				$link_prev .= '&limitstart=' . ($page_prev - 1);


wrong variable name,
$page_prev should be $page

csthomas · 2018-01-28T09:21:12Z

Fixed

Quy · 2018-01-29T15:20:07Z

I have tested this item ✅ successfully on 4c8c189

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19467.}

csthomas · 2018-02-09T12:40:27Z

@Bakual Could you approve this PR? Could it go to 3.8?

Bakual · 2018-02-09T13:17:35Z

@csthomas Technically it's a new feature and thus can't go into 3.8.x. It has to go into 3.9.0.
The code related to the pagination itself looks fine to me. I barely know the pagebreak plugin so I can't comment much there.
If it works for all affected views and that plugin, then it's fine 😄

csthomas · 2018-02-10T16:20:16Z

This PR requires one more human test.

joomdonation · 2018-02-11T08:24:47Z

I have tested this item ✅ successfully on 4c8c189

Tested with Category Blog menu option, tested page break plugin, too

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19467.}

ghost · 2018-02-11T09:40:53Z

Ready to Commit after two successful tests.

csthomas · 2018-06-30T22:51:28Z

Can we consider milestone Joomla 3.9.0 once again?

mbabker · 2018-07-07T16:34:21Z

@csthomas After changing the base branch to 3.9-dev to try merging there are some merge conflicts in the pagebreak plugin.

csthomas · 2018-07-07T22:21:47Z

OK, merged

csthomas · 2018-07-09T13:30:08Z

Be aware that $this->assertEquals(); is not a strict comparison (== vs ===), and both ...->active = '' or ...-> active = false will pass unit tests.

mbabker · 2018-07-09T13:32:44Z

IIRC that's why $this->assertSame(); is the recommended approach (it does do (more) strict comparison).

csthomas · 2018-07-09T13:50:57Z

Yes, obviously.

Travis passed.
Javascript error is not related to this PR.

One more human test please if needed.

Add an option to pagination to not generate URL with limitstart=0

d1a0b8e

joomla-cms-bot added the PR-staging label Jan 27, 2018

csthomas mentioned this pull request Jan 27, 2018

Replace the URL parameter "limitstart=0" with "start=0" if the SEF mode is on #19452

Merged

Bakual reviewed Jan 27, 2018

View reviewed changes

Change variable name and add more views

173f5df

Bakual reviewed Jan 27, 2018

View reviewed changes

Fix pagination default behaviour and add unit tests

ebecd75

joomla-cms-bot added the Unit/System Tests label Jan 27, 2018

Add pagination hideEmptyLimitstart option to com_tags/tag

143a69d

ggppdk reviewed Jan 28, 2018

View reviewed changes

Change to

4c8c189

csthomas changed the title ~~Add an option in Pagination class to not generate URL with limitstart=0~~ Add an option in Pagination class to generate URL without limitstart=0 Feb 6, 2018

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 11, 2018

brianteeman modified the milestones: Joomla 3.9.0, Joomla 3.10.0 May 8, 2018

Merge branch '3.9-dev' into pagination_hide_limitstart_zero

00a7ff2

Fix unit tests for 3.9

8c07036

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 2, 2018

csthomas deleted the pagination_hide_limitstart_zero branch August 3, 2018 09:08

hendrikbehncke mentioned this pull request Nov 9, 2018

[3.9] navigating to first page in pagination not possible with SEF urls disabled #22983

Closed

csthomas mentioned this pull request Nov 11, 2018

[3.9] Fix navigating to the first page in pagination when SEF is off #23042

Merged

Uh oh!

Conversation

csthomas commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Documentation Changes Required

Uh oh!

Bakual Jan 27, 2018

Choose a reason for hiding this comment

Uh oh!

csthomas Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bakual Jan 27, 2018

Choose a reason for hiding this comment

Uh oh!

csthomas Jan 27, 2018

Choose a reason for hiding this comment

Uh oh!

csthomas commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Quy commented Jan 27, 2018

Uh oh!

csthomas commented Jan 27, 2018

Uh oh!

Quy commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggppdk Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

csthomas commented Jan 28, 2018

Uh oh!

Quy commented Jan 29, 2018

Uh oh!

csthomas commented Feb 9, 2018

Uh oh!

Bakual commented Feb 9, 2018

Uh oh!

csthomas commented Feb 10, 2018

Uh oh!

joomdonation commented Feb 11, 2018

Uh oh!

ghost commented Feb 11, 2018

Uh oh!

csthomas commented Jun 30, 2018

Uh oh!

mbabker commented Jul 7, 2018

Uh oh!

csthomas commented Jul 7, 2018

Uh oh!

csthomas commented Jul 9, 2018

Uh oh!

mbabker commented Jul 9, 2018

Uh oh!

csthomas commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

csthomas commented Jan 27, 2018 •

edited

Loading

csthomas Jan 27, 2018 •

edited

Loading

csthomas commented Jan 27, 2018 •

edited

Loading

Quy commented Jan 28, 2018 •

edited

Loading