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

Add an option in Pagination class to generate URL without limitstart=0 #19467

Merged
merged 7 commits into from Aug 2, 2018

Conversation

Projects
None yet
9 participants
@csthomas
Contributor

csthomas commented Jan 27, 2018

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;
* @var boolean The flag indicates whether to add limitstart=0 to URL
* @since __DEPLOY_VERSION__
*/
public $append_empty_limitstart = true;

This comment has been minimized.

@Bakual

Bakual Jan 27, 2018

Contributor

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.

This comment has been minimized.

@csthomas

csthomas Jan 27, 2018

Contributor

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.

@@ -71,7 +71,7 @@ class Pagination
* @var boolean The flag indicates whether to add limitstart=0 to URL
* @since __DEPLOY_VERSION__
*/
public $append_empty_limitstart = true;
public $hideEmptyLimitstart = true;

This comment has been minimized.

@Bakual

Bakual Jan 27, 2018

Contributor

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

This comment has been minimized.

@csthomas

csthomas Jan 27, 2018

Contributor

Yes, I forgot to change it.

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jan 27, 2018

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

Affected views:

  1. com_content/category
  2. com_content/featured
  3. com_content/article (plugin pagebreak)
  4. com_content/archive
  5. com_contact/category
  6. com_contact/featured
  7. com_newsfeeds/category
  8. com_search/search
  9. com_finder/search
  10. com_tags/tags
  11. 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

This comment has been minimized.

Contributor

Quy commented Jan 27, 2018

Please apply to Tagged items.

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jan 27, 2018

Done

@Quy

This comment has been minimized.

Contributor

Quy commented Jan 28, 2018

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

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

This comment has been minimized.

@ggppdk

ggppdk Jan 28, 2018

Contributor

wrong variable name,
$page_prev should be $page

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jan 28, 2018

Fixed

@Quy

This comment has been minimized.

Contributor

Quy commented Jan 29, 2018

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 csthomas changed the title from Add an option in Pagination class to not generate URL with limitstart=0 to Add an option in Pagination class to generate URL without limitstart=0 Feb 6, 2018

@csthomas

This comment has been minimized.

Contributor

csthomas commented Feb 9, 2018

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

@Bakual

This comment has been minimized.

Contributor

Bakual commented Feb 9, 2018

@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

This comment has been minimized.

Contributor

csthomas commented Feb 10, 2018

This PR requires one more human test.

@joomdonation

This comment has been minimized.

Contributor

joomdonation commented Feb 11, 2018

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.

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Feb 11, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Feb 11, 2018

@mbabker mbabker added this to the Joomla 3.9.0 milestone Feb 13, 2018

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jun 30, 2018

Can we consider milestone Joomla 3.9.0 once again?

@mbabker mbabker changed the base branch from staging to 3.9-dev Jul 7, 2018

@mbabker mbabker removed this from the Joomla 3.10.0 milestone Jul 7, 2018

@mbabker mbabker added this to the Joomla 3.9.0 milestone Jul 7, 2018

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 7, 2018

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

@mbabker mbabker added PR-3.9-dev and removed PR-staging labels Jul 7, 2018

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jul 7, 2018

OK, merged

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jul 9, 2018

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

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 9, 2018

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

@csthomas

This comment has been minimized.

Contributor

csthomas commented Jul 9, 2018

Yes, obviously.

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

One more human test please if needed.

@mbabker mbabker merged commit 0341188 into joomla:3.9-dev Aug 2, 2018

3 of 4 checks passed

continuous-integration/drone/pr the build failed
Details
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Aug 2, 2018

@csthomas csthomas deleted the csthomas:pagination_hide_limitstart_zero branch Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment