Problem/Motivation
\Drupal\Core\Entity\EntityListBuilder() has a hardcoded limit of 50 entities, at which it paginates. This is a fine default, but there's no way to override it dynamically, making it unusable for any use case that requires the entire list (such as the node/add, page, for example--see #2693485: Content types are ordered by machine name on /node/add page (+ similar issues with other entities)). It would sure be nice to have a public setLimit() method. Patch to follow.
Steps to reproduce
Get the entity list builder service to build list of entities.
Note that you'll received 50 entities in the first page load since it is hardcoded to 50. Each page will have 50 items.
It should be flexible so we can pass the number of items that we want in pager.
Proposed resolution
\Drupal\Core\Entity\EntityListBuilder() should have a public method setLimit() that allows to override the pager limit for enity list builder service
Remaining tasks
Based on feedback from #33 we need a general agreement from community about the solution approach.
User interface changes
N/A
API changes
A new method setLimit() is introduced to EntityListBuilder class.
Data model changes
N/A
Release notes snippet
EntityListBuilder now allows to override the number of items per page using setLimit() method.
| Comment | File | Size | Author |
|---|---|---|---|
| #35 | interdiff_26-35.txt | 1.1 KB | sourabhjain |
| #35 | 2736377-35.patch | 6.08 KB | sourabhjain |
| #27 | 2736377-26.patch | 6.11 KB | sourabhjain |
Issue fork drupal-2736377
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
traviscarden commentedComment #3
traviscarden commentedComment #19
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
MR will need to be updated for 10.1
New function should be typehinted
Also will need test coverage.
Comment #21
mohit_aghera commentedFixed the following things in the patch:
- Added return types and argument types in the methods.
- Added test cases to validate that limit affects listing as well as pager count.
I've specifically added functional test because Kernel test was not able to fetch the proper pager count.
Also interdiff is taken from patch #2 since almost all the patches/changes are same.
I'm uploading patch since I don't have access to change the destination.
Test and code-commit check passing on local
Comment #22
smustgrave commentedTests look good!
Think this will need a change record to show how users can change this.
Comment #23
tedfordgif commentedGiven that the setLimit() method returns self, it probably makes sense to test that contract in the test, e.g. instead of these two lines:
just this one
Comment #24
sourabhjainLet me work on #23.
Comment #25
tedfordgif commentedMy apologies for leaving out the assignment to $build, I'll edit my comment.
Comment #26
mohit_aghera commentedFixing feedback mentioned in #23
Added change record here: https://www.drupal.org/node/3349556
Comment #27
sourabhjainI have resolved the #23. Please review.
Comment #28
sourabhjainAhh! mohit_aghera has added the patch at the same time while I was uploading it so same patch added in #26 and #27 so you can ignore any of the patch.
Comment #29
borisson_Unpublished the change record, it should only be published when it is actually merged. Removing the tag as well,
Comment #30
smustgrave commentedChange record looks good and makes sense.
Will let committer decide but this may get sent back for IS update (if anyone wants to do that before it's reviewed)
Comment #32
borisson_Not sure why it was retesting #21, enabling #27 instead.
Comment #33
quietone commentedTriaging the RTBC queue.
Always nice to see the old issue getting resolved. Go Needs Review Queue Initiative!
As pointed out in #30 the IS is incomplete. There is very little there to help the reviewer use their time efficiently. We should assume that the Issue Summary always needs to be complete. Another committer has consistently told me they set back issues that do not have a complete proposed resolution section and I am more particular than they are!
Reviewing the comments I see reviews of the test but no comments about the actual code changes, which were first introduced in 2016. Is this the best solution? Of course, correct me if I am missing the review.
I read the code comments. Function comments are to use a third person singular present test verb (yes it is in the standard)
This doesn't seem quite right. I think this is what is meant, "Renders a list of the specified number of entities."
Can be simpler. "Tests the setLimit() method."
I read the CR and it does have all the information. But it does need some work to improve the grammar and readability, including the title. There are two examples given and I think it would help to have those in a section titled 'Examples'. Remember, that not all the readers of the CR are native English speakers and we need to strive to use correct English and be concise and clear.
Setting to NW for the above items.
Comment #34
sourabhjainLet me work on #33
Comment #35
sourabhjainFixed the issue mentioned in #33. Please review.
Comment #36
smustgrave commentedPlease read the full comment and not jump straight to the suggestions. It’s making it increasingly difficult to keep up with tickets that keep doing this.
Called for an issue summary update
Verification of the solution.
Change record updates.
Comment #37
quietone commented@sourabhjain, in cases like this, where you respond to part of a review, it is good practice to comment on exactly what you did do to address #33 and the things you did not do. This helps the next person to know what to review and what to work on.
Comment #38
sourabhjainSure @quientone. So in #35, I have resolved the point 1 and 2 suggestion which is mentioned in #33. Please review it.
Comment #39
mohit_aghera commented- Updated the issue summary.
- Updated the CR
Still keeping in needs work since we need consensus around the solution.
Comment #40
mohit_aghera commented