Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
DraggableListBuilder.php
doesn't have the functionality to add a pager if a limit is specified.
Proposed resolution
Add the functionality to the DraggableListBuilder.php
Example in EntityListBuilder.php
Comment | File | Size | Author |
---|---|---|---|
#20 | 2826389-20.patch | 1.96 KB | alexpott |
#20 | 14-20-interdiff.txt | 2.46 KB | alexpott |
#20 | 2826389-20.test-only.patch | 1.47 KB | alexpott |
#19 | draggable_list_builder_has_no_pager-2826389-14.patch | 1.83 KB | alexpott |
#10 | dragable_list_builder_has_no_pager-2826389-10.patch | 1.34 KB | kmoll |
Comments
Comment #2
tbonomelli CreditAttribution: tbonomelli at MD Systems GmbH commentedComment #3
BerdirThis 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.
Comment #4
faline CreditAttribution: faline at CI&T commentedComment #5
Robin Monks CreditAttribution: Robin Monks at Appnovation commentedWent down the path of #2821696 and simply made this draggable not have a limit.
Comment #6
Robin Monks CreditAttribution: Robin Monks at Appnovation commentedComment #7
Robin Monks CreditAttribution: Robin Monks at Appnovation commentedAs 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.
Comment #8
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedI 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.
Comment #9
BerdirThe 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.
Comment #10
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedI 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.
Comment #11
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedComment #13
BerdirYou 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.
Comment #14
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedHere is the interdiff and full patch.
Comment #15
kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commentedComment #16
faline CreditAttribution: faline at CI&T commentedIt 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.
Comment #17
faline CreditAttribution: faline at CI&T commentedComment #19
alexpottRe uploading #14 because something is weird and I can't retest it. No credit for me please.
Comment #20
alexpottMoved the test to a browser test and clean up some spacing and made the test a little more rigourous.
Comment #24
xjm#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:
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!