Steps to reproduce:

- On a clean Drupal installation, use the "Devel generate" module to create 100 nodes of type "Story".
- Go to admin/content/node and make sure all nodes are promoted to the frontpage.
- Now, if you go to the frontpage, in the bottom pager you'll see 9 (out of 10) pages listed, but no ellipsis at the end.

The issue appears to be in the theme_pager function, at the point where it checks whether to append an ellipsis element at the end of the list or not.

CommentFileSizeAuthor
#55 pager_ellipsis_D8-470428-55-complete_0.patch1.69 KBSivaji_Ganesh_Jojodae
#48 pager_ellipsis_D8-470428-44-complete.patch1.74 KBkoence
#44 pager_ellipsis_D8-470428-44-complete.patch649 byteskoence
#44 interdiff-470428-38-44.txt649 byteskoence
#38 pager_ellipsis_D8-470428-38-complete.patch1.74 KBkoence
#38 pager_ellipsis_D8-470428-38-test.patch1.34 KBkoence
#38 interdiff-470428-36-38.txt1.19 KBkoence
#36 pager_ellipsis_D8-470428-36-complete.patch1.34 KBkoence
#36 pager_ellipsis_D8-470428-36-test.patch959 byteskoence
#32 patch_not_applied_ellipsis_missing.png56.29 KBkoence
#32 patch_applied_ellipsis_shown.png57.6 KBkoence
#31 pager_ellipsis_D8-470428-31.patch414 bytesszt
#23 D7-ellipsispagerfixedforlastpage-470428.patch388 bytescepinos
#20 D7-pager-ellipsis-470428-1.patch418 bytescac2s
#19 D7-pager-ellipsis-470428.patch418 bytescac2s
#13 D7-pager-ellipsis-470428.patch418 bytescac2s
#12 D7-pager-ellipsis-470428-do-not-test.patch418 bytescac2s
#9 pager-ellipsis-D7.26.patch388 bytescac2s
#6 pager_ellipsis-470428-5.patch388 bytesszt
#4 pager_ellipsis-470428-4.patch386 bytesszt
#1 pager-ellipsis.patch560 bytesfoutrelis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

foutrelis’s picture

Status: Active » Needs review
FileSize
560 bytes

Unless I'm missing something, we can simply check if $pager_last contains a smaller number than $pager_max.

OpenChimp’s picture

Version: 6.12 » 7.x-dev

It seems like the above patch has been committed, but it is not quite correct.

Instead of:

      if ($i < $pager_max) {
        $items[] = array(
          'class' => array('pager-ellipsis'),
          'data' => '…',
        );
      }

It should read: (note $pager_max+1 in the conditional)

      if ($i < $pager_max+1) {
        $items[] = array(
          'class' => array('pager-ellipsis'),
          'data' => '…',
        );
      }

In D6 this will be:

      if ($i < $pager_max+1) {
        $items[] = array(
          'class' => 'pager-ellipsis',
          'data' => '…',
        );
      }
casey’s picture

Pager in D7 has changed a lot; does the issue still exist? If not you can move this to D6.

szt’s picture

FileSize
386 bytes

The issue is exists, #2 solves it, i've turned into patch.

Désiré’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Great, it seems work.

But, please see the coding stnadards for operators: http://drupal.org/coding-standards#operators

All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. should have a space before and after the operator, for readability. For example, an assignment should be formatted as $foo = $bar; rather than $foo=$bar;.

And this issue needs automated tests.

szt’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
388 bytes

...again with using coding standards

parthipanramesh’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Works fine. Thanks.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

Haven't tested Drupal 8, but the same code exists there, so I assume it would need to be fixed there first.

cac2s’s picture

FileSize
388 bytes

patch for 7.26

Status: Needs review » Needs work

The last submitted patch, 9: pager-ellipsis-D7.26.patch, failed testing.

cac2s’s picture

cac2s’s picture

Priority: Minor » Normal
Status: Needs work » Needs review
Issue tags: -Needs backport to D7 +fix backported to D7
FileSize
418 bytes
cac2s’s picture

Version: 8.x-dev » 7.26
Status: Needs review » Reviewed & tested by the community
Issue tags: -fix backported to D7
FileSize
418 bytes
cac2s’s picture

Status: Reviewed & tested by the community » Needs review
cac2s’s picture

Version: 7.26 » 7.x-dev
MrHaroldA’s picture

Status: Needs review » Reviewed & tested by the community

Works perfectly!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: D7-pager-ellipsis-470428.patch, failed testing.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Still seeing similar code in Drupal 8, and no indication from anyone that the bug doesn't exist there.

cac2s’s picture

FileSize
418 bytes
cac2s’s picture

FileSize
418 bytes
MrHaroldA’s picture

Status: Needs work » Needs review

The test failed on queries, so re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: D7-pager-ellipsis-470428-1.patch, failed testing.

cepinos’s picture

Status: Needs work » Needs review
FileSize
388 bytes

This patch fix the problem, I tested it on Drupal 7.

Status: Needs review » Needs work

The last submitted patch, 23: D7-ellipsispagerfixedforlastpage-470428.patch, failed testing.

alx_benjamin’s picture

It looks like the proposed patches are for D7.
So why is this issue in D8 version?
You cannot test D7 patches against D8 and expect them to pass.

Are we not supposed to create a new issue with correct Drupal version and submit
patches with correct versions so they could be tested properly?

Correct me if I'm wrong
-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

alx_benjamin’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Needs review

Cannot replicate in D8. So moving this to original D7 version

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

szt’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Needs review » Needs work
Issue tags: -Needs backport to D7

I've posted exactly the same patch in #6 and was confirmed in #7.
The issue needs the D8 fix now.

szt’s picture

Version: 8.1.x-dev » 8.0.x-dev
David_Rothstein’s picture

Issue tags: +Needs backport to D7
szt’s picture

Status: Needs work » Needs review
FileSize
414 bytes

Ok, I've looked after: the bug exists in D8, here is the patch for it.
I've tested, works well.

koence’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
57.6 KB
56.29 KB

Ellipsis is shown indeed in D8.

Tested with and without patch.

With patch:

Without patch:

Wim Leers’s picture

Issue tags: +Needs tests
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I think this needs tests before it can be committed. We want to avoid this from regressing again.

You'll probably want to expand \Drupal\system\Tests\Pager\PagerTest.

koence’s picture

Assigned: Unassigned » koence

I'll work on the test

koence’s picture

Assigned: koence » Unassigned
Status: Needs work » Needs review
FileSize
959 bytes
1.34 KB
Wim Leers’s picture

Status: Needs review » Needs work

Great! But we also need a test for the case where the ellipsis is expected to not yet appear.

koence’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
+++ b/core/modules/system/src/Tests/Pager/PagerTest.php
@@ -99,9 +99,18 @@ protected function testPagerQueryParametersAndCacheContext() {
+    $elements = $this->cssSelect(".pager__item--ellipsis:contains('…')");
+    $this->assertEqual(count($elements), 0, 'No ellipsis has been set.');

Clever! :) Love it!

Excellent test — hope to see you around more often in the D8 core issue queue :)

The last submitted patch, 36: pager_ellipsis_D8-470428-36-test.patch, failed testing.

The last submitted patch, 38: pager_ellipsis_D8-470428-38-test.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/src/Tests/Pager/PagerTest.php
@@ -96,6 +96,29 @@ protected function testPagerQueryParametersAndCacheContext() {
   /**
+   * Test proper functioning of the ellipsis.
+   */
+  protected function testPagerEllipsis() {

This needs to be public. Yes it still works with protected and I know that another method in this test is incorrectly marked as protected but we should still get this one correct.

Wim Leers’s picture

Issue tags: +Novice, +php-novice, +Needs reroll

Oops, should've caught that. Sorry, Alex.

koence’s picture

Patch has been updated!

koence’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 44: pager_ellipsis_D8-470428-44-complete.patch, failed testing.

koence’s picture

Assigned: Unassigned » koence
koence’s picture

Assigned: koence » Unassigned
Status: Needs work » Needs review
FileSize
1.74 KB

Made a mistake with previous patch in #44.
Should be ok now.
Function changed from protected to public.

pjbaert’s picture

Status: Needs review » Reviewed & tested by the community

Looks like this is rtbc.
Function is now public as requested in #42

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Manual testing proves this fix is correct. Nice work. Committed 88470f9 and pushed to 8.0.x. Thanks!

  • alexpott committed 88470f9 on 8.0.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...
MrHaroldA’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

The patch in #5 and #23 (same patch) are already reviewed and tested a log time ago, including by me ;)

  • alexpott committed 88470f9 on 8.1.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

The tests need to be backported from Drupal 8 also.

Sivaji_Ganesh_Jojodae’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Patch for D7 with test.

  • alexpott committed 88470f9 on 8.3.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...

  • alexpott committed 88470f9 on 8.3.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...

  • alexpott committed 88470f9 on 8.4.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...

  • alexpott committed 88470f9 on 8.4.x
    Issue #470428 by koence, cac2s, szt, foutrelis, cepinos, Wim Leers:...