List of Views limited to 50 items

51 views is a lot! Good find :)

#1798332: Add paging to the EntityListBuilder always adds a pager, and if a subclass of EntityListBuilder doesn't add a #type => pager element, this will happen.

But the views listing is custom and has the separate lists of enabled and disabled, so we should opt out of paging altogether.

We should add a test to prove this works.

1. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -92,12 +92,14 @@ public function load() {
   +
   +    // Only add the pager if a limit is specified.
   +    if ($this->limit) {
   +      $query->pager($this->limit);
   +    }
   

   Are you sure we need this? The documentation says that NULL will disable the pager, so passing in NULL should be fine.. this might be a bit more explicit, though.

2. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -33,6 +33,11 @@ class ViewListBuilder extends ConfigEntityListBuilder {
       * {@inheritdoc}
       */
   +  protected $limit = FALSE;
   

   I'd use NULL here, not FALSE, don't really have a good argument though.

1) The docs do say If passed a false value (FALSE, 0, NULL), the pager is disabled., but I was just going to skip the whole thing since we can.
2) What's your not-so-good argument? :)

The only argument I have is that we changed quite a few API's from FALSE to NULL, entity entity load return value if it doesn't exist.

NULL is closer to none/nothing than FALSE, imho. I think FALSE should preferably only be used when it's a boolean.. TRUE or FALSE... not an actual value or nothing.

I thought of it as "Do I want limiting? No I do not! FALSE!"
But I can switch it when I write tests.

I added the test.

1. +++ b/core/modules/views_ui/src/Tests/ListLimitTest.php
   @@ -0,0 +1,42 @@
   +/**
   + * Tests the list of views limit.
   + *
   + * @group views_ui
   + */
   +class ListLimitTest extends UITestBase {
   

   Lets make the test class a bit more generic, we might want to add more tests at some point.

   I was looking for an existing test where we could add this but didn't find a good one.

   Maybe just ViewsListTest as class name and something like "Tests the views list" ?

2. +++ b/core/modules/views_ui/src/Tests/ListLimitTest.php
   @@ -0,0 +1,42 @@
   +  /**
   +   * Tests the list of views limit.
   +   */
   +  public function testWizardListLimit() {
   

   Leave "Wizard" out (Wizard is the thing that allows you to create a new view, this is nothing to do with that. We're not listing wizards).

   Description could be "Tests that the views list does not use a pager."?

3. +++ b/core/modules/views_ui/src/Tests/ListLimitTest.php
   @@ -0,0 +1,42 @@
   +    $rows = $this->xpath('//tbody/tr');
   +    // Check if all the rows are listed.
   +    $this->assertTrue(count($rows) > $limit);
   

   We should be a bit more explicit here and for example explain that we have a lot more views because there are also a number of default views.

Uploading the patch with the changes

Its a small step forward. Does someone mind open up a new issue to make views again working with a pager? I think this is what you want at the end.

Created the follow-up #2536826: Add a pager to the list of Views.

Thank you!

1. I'm not entirely convinced by this fix since sites with 100's of views will have a problem - talked with @dawehner and @Berdir on IRC - I didn't really get the feeling we had a consensus. However given that the filter does not return all the views we certainly have a bug here.

2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -40,9 +40,9 @@ class EntityListBuilder extends EntityHandlerBase implements EntityListBuilderIn
      /**
   -   * The number of entities to list per page.
   +   * The number of entities to list per page, or FALSE to list all entities.
       *
   -   * @var int
   +   * @var int|bool
       */
   

   I think we should improve the docs to mention that FALSE should be used when the overview page has a filter on it (like the views page).

3. +++ b/core/modules/views_ui/src/Tests/ViewsListTest.php
   @@ -0,0 +1,44 @@
   +class ViewsListTest extends UITestBase {
   ...
   +    // Check that all the rows are listed, there are also default views so
   +    // we have more views than created.
   +    $this->assertTrue(count($rows) > $limit);
   

   I think we should just extend WebTestBase and not have any default views get in the way. Then we can assert that we have the number of expected views.

4. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -33,6 +33,11 @@ class ViewListBuilder extends ConfigEntityListBuilder {
      /**
       * {@inheritdoc}
       */
   +  protected $limit = FALSE;
   

   We should document why we are doing this here.

Here is the patch with the recommendations.

I don't think that test would actually fail right now? You want to have one more than 50 atleast.

Limit has been augmented (by 1), running test does not fail.
Also, admin/structure/views/ shows more than 50 views, sorted alphabetically.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -55,6 +60,10 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
+    // @todo Change the filtering to support a pager .

This @todo should point to the followup issue. And not have a space before the fullstop.

Fixed that

Comment has been updated. Reference to the follow-up issue was added, space before the full-stop was removed.

Committed 99a340d and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views_ui/src/ViewListBuilder.php b/core/modules/views_ui/src/ViewListBuilder.php
index b015154..00be178 100644
--- a/core/modules/views_ui/src/ViewListBuilder.php
+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -62,8 +62,8 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
     $this->displayManager = $display_manager;
     // This list builder uses client-side filters which requires all entities to
     // be listed, disable the pager.
-    // @todo Change the filtering to support a pager, see
-    // https://www.drupal.org/node/2536826.
+    // @todo https://www.drupal.org/node/2536826 change the filtering to support
+    //   a pager.
     $this->limit = FALSE;
   }
 

Fixed on commit - slightly reformated the comment. The next line in @todo's need to be indented.