Pager option 'Items per page' with value 0 gives confusing summary, change to 'All'

Thank you for a detailed bug-report

Not exactly a duplicate, but #2791455: Pager option 'Items per page' with exposed value 0 should by default show 'All items' on view shows a use case for setting it to 0. I don't think we can change the behaviour to default to something else as it would break existing sites relying on this.

I agree with #3, it would be hard to change this behaviour and not break things. So yeah the option to display 'All' and not 0 sounds pretty good

Attaching patch.

Patch works as intended. Screenshot attached.

Patch is working fine and this issue resolved.

Thank you @alecsmrekar.

RTBC +1

+++ b/core/modules/views/src/Plugin/views/pager/Some.php
@@ -19,10 +19,17 @@
-      return $this->formatPlural($this->options['items_per_page'], '@count item, skip @skip', '@count items, skip @skip', ['@count' => $this->options['items_per_page'], '@skip' => $this->options['offset']]);
+      $summary_text .= ', ' . $this->t('skip @skip', ['@skip' => $this->options['offset']]);

Concatenating translated string like this it not supported. It loses "safeness" and so any html in the translation will be lost and it doesn't give the translator the full context. All the the text this can result in...

"All items, skip 20"

... is pretty confusing too.

The patch #13 addresses comment #11.

The patch works perfectly, but this functionality should be added to full and mini view modes.

Just applied the solution from patch#13 to full and mini options, in order to standardizing the "All Items" view.

Applied patch #18 on 8.9.x and it works fine. Resolution "b" is available with display a specified no. of items. mini pager and full pager after this patch.Adding screenshots below.

After patch:

Display a specified no. of items mode

Full pager

Mini pager

RTBC +1

+++ b/core/modules/views/src/Plugin/views/pager/Full.php
@@ -68,10 +68,24 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        $summary_text = $this->t('All items, skip @skip', ['@skip' => $this->options['offset']]);

This is pretty confusing "All items, skip 10" is odd because it's not all items.

One thing we could ask is what is the point of a pager where the number of items per page is 0? I'm not sure that all items is the correct term here. When the UI has the test Use Pager: Full | 5 items it's pretty clear that 5 items are going to be displayed per page but Use Pager: Full | All items is not clear at all.

I really could use some suggestions of a better phrasing for this solution.

What about:

$summary_text = $this->t('All items, except first @skip', ['@skip' => $this->options['offset']]); 

Sending patch with phrasing changes addressing comment#20.

@guilhermevp I think the problem here is that \Drupal\views\Plugin\views\pager\Mini and \Drupal\views\Plugin\views\pager\Full should adjust items_per_page to have a minimum value of 1. A value of 0 makes no sense - that's what the \Drupal\views\Plugin\views\pager\None is for. Also \Drupal\views\Plugin\views\pager\Some should have a should adjust items_per_page to have a minimum value of 1.

Making that change is complex because we'd need to update existing views that have 0 for items per page and are not using the None pager.

The patch applies correctly and does what it should, if not making a mass change, it can be accepted

@ilgnerfagundes I think we should consider doing the better change here.

Applied patch #23 on 9.4.x and it works fine.

Seems like a change that needs test coverage.

Keeping the novice tag on this one after reviewing the issue.
This needs a simple assertion which can be added in the existing test SorePagerSettings to assert that when
- the number of items in the pager is set to `0`
- and the pager type is set to "Display a specified number of items"
the text on the pager option link is "All items" and not "0 items"

Novice issue reserved for the Mentored Contribution during the Contribution Day at DrupalCon Portland 2024. After the 8th of May 2024, this issue returns to being open to all. Thanks

My name is Ian and I'm working today on this issue at DrupalCon 2024

I think I added the suggested test. Forgive the extra commits I was unable to get failing tests with DrupalPod.

No need to apologize. Tests were exactly what I was thinking

Ran test-only feature of gitlab https://git.drupalcode.org/issue/drupal-3183766/-/jobs/1550667

1) Drupal\Tests\views\Functional\Plugin\PagerTest::testStorePagerSettings
Behat\Mink\Exception\ResponseTextException: The text "All items" was not found anywhere in the text of the current page.
/builds/issue/drupal-3183766/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3183766/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3183766/core/tests/Drupal/Tests/WebAssert.php:975
/builds/issue/drupal-3183766/core/modules/views/tests/src/Functional/Plugin/PagerTest.php:89
/builds/issue/drupal-3183766/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!

Which shows the issue. Manually testing also seeing the issue.

Change fixes the issue for me so believe this one is good.

Thank you for adding the tests and reviewing the issue @ian.ssu, @sushyl.

Thank you for reviewing the added tests and manually testing, @smustgrave.

I read the issue summary, but it was not clear to me what among the 3 proposed resolutions was the proposed resolution that was RTBC. I changed the status to Needs work only so that we can update the issue summary to clarify the proposed resolution.

The text changes should also be mentioned under User interface changes per Issue summary field - Issue Summary Template.

Thanks @mradcliffe,
Updating the Issue summary.

Good catch @mradcliffe

Solution seems clearer now.

Thank you to everyone at Portland for working on this issue!

I am triaging the RTBC queue. I began by reading the title, Issue Summary and then the last few comments. The title could be improved, it should indicate something about the change. We need to consider that this is used in the commit message and needs to make some sense to someone doing some git archeology. #41 asked for changes to the Issue Summary in the User Interface section. That section is empty so that has not been completed. Also, this is making changes to the User Interface so before and after screenshots should be available from the Issue Summary. The latest screenshots here are from 2 years ago.

Issues that are changing the UI are to be tagged 'usabilty', I am adding that tag. Usability issues often need a usabilty review as well. That may not been needed but without screenshots I am not able to make a decision.

I then went back and reviewed the code and tested locally. While doing so I made screenshots and have updated the issue summary. The changed code does what it needs to and is working well. Yay! I did spot that the test is only testing the changes to the 'some' pager type. It should test all types and all text changes.

While the tests were running I read the comments. I see that #4 is addressed but I don't think that #24 has been. That is a totally different approach. It is also a reminder to us all to read all the comments first!

I am going to save the changes I made to the MR but set this postponed and ask the other committers.

After some time away and eating a meal I concluded there is no need for consultation. The way forward is to open an new issue to look into the change suggested in #24. This issue improves the user interface and I think that is more important than an ideal improvement, even if that improvement means removing the code added here.

The title was changed in #46.

I implemented @quietone's feedback and added a unit test, deleting the previous changes to the functional test.

The three primary pager plugins; Full, Mini, and Some; now have identical summaryTitle() functions. This repetition is painful and may only lead to more issues in the future if only one is changed again later. Frankly, I think the changes should go in PagerPluginBase instead of having it return "Unknown." Let outlier plugins like the None plugin override this sensible default. Just my $0.02.

Also, I think it's important to note that the proposed changes remove unique text from the Full and Mini pagers. Previously, the Full pager output a string like:

Paged, @count items

And the Mini pager output a string like:

Mini pager, @count item

The "Paged" and "Mini pager" text is now gone. I don't know that it matters, but it needs to be mentioned.

I was able to apply the patch but the choices still seems a bit "clunky"

I suggest having 3 choices, "Display all items" which will do that and has no need for a pager. Then have Full and Mini pagers that are configurable.

To discuss the feedback on #50, also was tagged for a follow up but don't see the issue for that.

The Drupal Contribution Mentoring team is triaging issues for DRUPALCON NARA 2025, and we are reserving this issue for Mentored Contribution during the event. After November 19, 2025, this issue returns to being open to all. Thanks!

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because it seems we have the opportunity to refine the pager type messages, approach and also realign changes with the tests, It's also interesting to have a universal pager message that can handle offset.