Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tbonomelli created an issue. See original summary.

tbonomelli’s picture

Issue summary: View changes
Berdir’s picture

Category: Feature request » Bug report
Issue tags: +Novice, +Needs tests

This is a bug, if you have more than 50 results, they don't show up anymore.

We either need to show the pager (with the limitation that drag and drop doesn't work across pages, you have to manually change the weight) or we need to disable the pager as done in #2821696: On alias patters listing page more than fifty patterns are not showing up, with the risk that some config entity types might have a lot of entities.

faline’s picture

Assigned: Unassigned » faline
Robin Monks’s picture

Went down the path of #2821696 and simply made this draggable not have a limit.

Robin Monks’s picture

Status: Active » Needs review
Robin Monks’s picture

As a note for why I chose to not have a pager at all rather than one with the limitation: From my end-user standpoint having a possibly slower-loading page of all items I could move things between would be less frustrating than a multi-page system that I couldn't move items between page 1 and 2. Having that limitation feels more breaking to me.

kmoll’s picture

I have tested the above patch on the user/admin/roles page after creating more than 50 roles. Without the patch only 50 rows show with no pager as mentioned by @Berdir in #3. With the patch all roles show up. I also created a test for this.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/src/Tests/ConfigDraggableListBuilderTest.php
@@ -0,0 +1,50 @@
+
+    // Get count of number of rows.
+    $role_count = count(user_roles());
+

The amount of roles should be predictable. I think it would be better readable and more strict if we just hardcode it.

Please also provide a test-only patch, then we should be good to go.

kmoll’s picture

I updated the patch, I now just test that the amount is greater than 50 which is the default amount. This patch now just contains the test.

kmoll’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: dragable_list_builder_has_no_pager-2826389-10.patch, failed testing.

Berdir’s picture

You need to provide both, a test-only patch (first) and the combined patch. As you uploaded the test-only patch already, just the combined patch is enough now.

And you should also include an interdiff when making changes, so that it is easier to see the difference.

kmoll’s picture

Here is the interdiff and full patch.

kmoll’s picture

Status: Needs work » Needs review
faline’s picture

Status: Needs review » Reviewed & tested by the community

It is working for me!

Just a curiosity. Did someone try to limit it? Because I tried and even though the pager appeared on the page, it did not work for me.

faline’s picture

Assigned: Robin Monks » Unassigned

The last submitted patch, , failed testing.

alexpott’s picture

Re uploading #14 because something is weird and I can't retest it. No credit for me please.

alexpott’s picture

Moved the test to a browser test and clean up some spacing and made the test a little more rigourous.

The last submitted patch, 20: 2826389-20.test-only.patch, failed testing.

  • xjm committed 3cabea2 on 8.3.x
    Issue #2826389 by alexpott, kmoll, Robin Monks, Berdir, tbonomelli:...

  • xjm committed 16850db on 8.2.x
    Issue #2826389 by alexpott, kmoll, Robin Monks, Berdir, tbonomelli:...
xjm’s picture

Title: Draggable List Builder has no pager. » Draggable List Builder has no pager
Status: Reviewed & tested by the community » Fixed
Issue tags: -Novice, -Needs tests
Related issues: +#2536826: Add a pager to the list of Views, +#2429813: DraggableListBuilder should support pagers for scalability, +#191360: Scalable menu selector, +#2770649: [PP-1] Refactor OverviewTerms form to use the pager.factory service

#2429813: DraggableListBuilder should support pagers for scalability was a duplicate. I've rescoped it.

So I agree that displaying the entities is better than simply pretending they're not there. However, there is also a scalability problem with not using the pager, because if the page fails to load, instead of only having access to 50 entities, the site admin has access to 0. Looking at the core implementations:

FilterFormatListBuilder.php in core/modules/filter/src/FilterFormatListBuilder.php
LanguageListBuilder.php in core/modules/language/src/LanguageListBuilder.php
RoleListBuilder.php in core/modules/user/src/RoleListBuilder.php
SearchPageListBuilder.php in core/modules/search/src/SearchPageListBuilder.php
VocabularyListBuilder.php in core/modules/taxonomy/src/VocabularyListBuilder.php

It's quite easy to imagine a large site with hundreds of vocabularies. g.d.o/core has hundreds, for example. However, it's also easy to imagine having hundreds of block instances, and the Block UI's list builder also uses no pager. Even the menu UI, which includes content entities, has no pager. So there are already similar places in core where we're not even trying to use one. (The taxonomy term overview form has an example of draggable reordering with a pager for content entities, but it's a pretty scary one-off, and we broke it about every six month's during D8's development.)

So, let's go ahead with simply disabling the pager, and we can tackle a generic solution for a draggable list builder with a pager as a followup as needed. In the meanwhile, an extending class can still overrride this.

Committed and pushed to 8.3.x and 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.