Problem/Motivation
Hello,
The limit of entity reference when I use views is not working it's aways 10 instead of use the views pager
Steps to reproduce
- Create a new views and create a display type entity reference
- Select 50 items on pager
- Go to any content type and create an entity reference field
- On reference method select views after that select your view
- Create a new content type, fill out anything that have more than 10 contents on reference field, it will show just 10 items
Proposed resolution
If a limit is set on the view via a pager, use that setting. If no pager is set on the view, add a pager with a limit of 10 so users don't unexpectedly dig themselves into a huge performance hit when removing the pager from the view.
Remaining tasks
Add patch to use the pager
Add tests
User interface changes
None, UI settings are now actually used instead of ignored.
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|---|---|---|
#54 | 2772523-54.patch | 829 bytes | Anchal_gupta |
#27 | 2772523-27.patch | 6.75 KB | Lendude |
#27 | interdiff-2772523-26-27.txt | 4.03 KB | Lendude |
#26 | 2772523-22-26.interdiff.txt | 1.83 KB | mikeker |
#26 | 2772523-26-entity-reference-limit.patch | 4.8 KB | mikeker |
Issue fork drupal-2772523
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
guilopes CreditAttribution: guilopes commentedComment #3
guilopes CreditAttribution: guilopes commentedI forgot one thing on the last patch :)
Comment #4
guilopes CreditAttribution: guilopes commentedComment #5
guilopes CreditAttribution: guilopes commentedComment #7
guilopes CreditAttribution: guilopes commentedany updates on it?
Comment #8
claudiu.cristeaI don't think this is a good idea. The limit is hardcoded to 10 in \Drupal\Core\Entity\EntityAutocompleteMatcher::getMatches(), line:
This, I think, was the intention, to limit the matches to a decent number and preventing the flood of Ajax response with a too large list.
Comment #9
guilopes CreditAttribution: guilopes commentedI know, but it don't make sense when we have a view. If I want to show more than 10 I need to hack the core to do that?
I really want more than 10 items on my project because only 10 is not enought for my use case and for me it's the expected behavior when I have a view. What do you guys think about that?
Comment #10
guilopes CreditAttribution: guilopes commentedComment #11
dawehnerNo matter what we do, we should ensure that some test coverage is given.
Comment #12
Fidelix CreditAttribution: Fidelix as a volunteer commentedSorry but it doesn't make sense to hardcode this value to 10.
People may have different requirements and it's such a simple thing to change.
Test coverage however is certainly necessary.
Comment #14
jonathanshawRe #8 can we make the limit 10, unless an explicit limit is set in the view (in which case use that)? This way it works out of the box no matter how many items in the view, but you can get more than 10 if you explicitly say you want that.
Entity reference views are single-purpose by their nature, so they are crafted to serve this autocomplete, and we can place a high level of responsibility on sitebuilders to configure them right - it's not like autocomplete is using a views display that is also used for some other purpose that might require a different limit.
Comment #15
LendudeI agree that if we give the option to set this, we should use the setting. So either we remove the ability to set the number of items for entity references or we actually use the setting. I would be for using the setting.
I agree with @claudiu.cristea that this might lead to issues when set to a high number, but there are many ways to use Views to dig yourself into a huge performance hole if you don't know what you are doing, so *shrug*
Needs work for tests.
Comment #16
mikeker CreditAttribution: mikeker as a volunteer commentedI think we should postpone this issue until #2174633: View output is not used for entityreference options is resolved. Once that fix is in, here is a test to validate this fix (uploading as txt as it won't apply until 2174633 is committed).
Comment #17
mikeker CreditAttribution: mikeker as a volunteer commentedOoops. Left my debug code in there. This should be removed when we reactivate this issue.
Comment #18
guilopes CreditAttribution: guilopes commented@mikeker I don't think we need to wait that issue, since it's just the number of items. It's a really simple feature and I think it should be in core ASAP
Comment #19
claudiu.cristeaOK, let's limit to view's display limit if one is set.
Few notes:
none
) is used, all the records are returned. But this is not OK as we need, at least, to fallback to the hard-limit of 10. This bug can be observed in the "test only" patch when the view is used without a pager. Normally, users are not noticing this bug because the entity reference display comes with pager by default. We should enforce a pager in such cases and use the hard limit of 10 items.Comment #21
mikeker CreditAttribution: mikeker as a volunteer commentedI agree. I originally thought that fixing the above mentioned issue would include the limits imposed by the view but I was wrong. The code changes included in #19 (and earlier patches) are needed. Thank you for correcting that, @guilopes!
I disagree with this. I don't think we should be setting pagers when a site builder has specifically removed it (a default view comes with a pager of 10 items). I can't think of a good reason to allow unlimited options in an autocomplete, but that doesn't mean the reason doesn't exist.
We should be providing reasonable defaults and minimal limitations for site builders.
Comment #22
mikeker CreditAttribution: mikeker as a volunteer commentedComment #24
claudiu.cristeaI totally disagree with not keeping, at least, the 10 items fallback. The rule that I'm proposing is: If a pager has been explicitly provided by the view, use it as limit. Otherwise default to 10 items. I would not allow infinite (the max count returned by the view) items -- there's a very good reason for limiting the autocomplete suggestion list.
Comment #25
jonathanshawI don't see that it matters in practice. It'd be a shame to block the patch on this point.
But because we need to decide one way or the other, I suggest we have the 10 item fallback limit if no pager as suggested by @claudiu.cristea, because a sitebuilder can always specify a pager with a 100000 item limit if that really really is what they want. In other words, if they want to live dangerously then they have to be explicit about it.
I can see more easily imagine situations in which a sitebuilder could get into trouble if there was no fallback limit, than I can situations in which they can't achieve what they want because there is a fallback limit.
Comment #26
mikeker CreditAttribution: mikeker as a volunteer commented@claudiu.cristea, @jonathanshaw: Good arguments and I concede the point. As @jonathanshaw points out a site builder could set the limit to 9999999 if they really wanted all results. Still, I'm worried about adding a pager to a view that doesn't have one, but I can quash that worry in order to get this into core... :)
The attached patch is revert of #22, which means is should be identical to @claudiu.cristea's patch in #19.
Comment #27
Lendude++ to adding the pager when none is set. People are free to set the limit on a ridiculous number, but at least they would do so knowingly. I think the solution to add a pager is more elegant then just hardcoding a limit on 10, if none is provided. With the hardcoded limit a site-builder would then have no idea where the limit is coming from and would be completely in the dark about how to circumvent this.
Updated the IS to show the current fix direction.
Not real happy with the spot where we are fixing this. Any call to
\Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::initializeView
will now get the limit set to the View limit.So this call in
\Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::validateReferenceableEntities
which hardcodes its limit to 0, will now not use the passed 0, but the set limit on the View.
Moving the logic to
\Drupal\views\Plugin\EntityReferenceSelection\ViewsSelection::getReferenceableEntities
makes more sense to me, but that is also passed a $limit so the same would still go, just on a higher level (and it leads to some code duplication because we need to verify the View display twice before using it). So open to other ideas if somebody has them.Added some dependency injection too.
Comment #28
guilopes CreditAttribution: guilopes commentedCan we apply this now ?
Comment #29
guilopes CreditAttribution: guilopes commentedComment #30
guilopes CreditAttribution: guilopes commentedAny answer here? I think we should merge it
Comment #31
tstoecklerI think it's pretty strange, though, that we are now completely throwing away the passed-in
$limit
as soon as the view has a pager. I think we should either check if the passed limit is0
or maybe even do something like, but just (conditionally) ignoring it is strange IMO.
Comment #35
Publishing Future CreditAttribution: Publishing Future commentedIs there an update of the patch in #27 for Drupal 8.6 available?
Comment #38
colan#31:
!empty()
should ensure it's > 0. That should be sufficient.Comment #39
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedI am working on this..
Comment #40
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedComment #41
colan@swatichouhan012: Did you accidentally unassign yourself, or were you only planning on working on it for 2 hours & 14 minutes? :)
Comment #46
steveoriolThis issue is still present in D9.3.0...
Comment #47
cilefen CreditAttribution: cilefen commentedComment #51
kopeboy CreditAttribution: kopeboy as a volunteer commentedIt's a shame this isn't fixed yet after years in core.
Any site builder CANNOT force an entity reference selection: even if his custom logic defined in a View results in 1 item, a checkbox on the entity form will list multiple options, that he cannot even see in the Views' preview!
Also, why can't this be fixed in 10.1.x instead of 11?!
Comment #52
demon326 CreditAttribution: demon326 commentedI ran into this small issue using webforms to make a more visual selection of nodes. The patches are not working for 10.2.x. Would be great if this could be fixed , because showing ALL the results on a single page will result in performance issues.
Comment #53
kopeboy CreditAttribution: kopeboy as a volunteer commentedI was able to fix this just by adding to
/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
(ie. simpler than patch#27)
After the line with
$this->view->setDisplay($display_name);
and before the line with
$entity_reference_options = [
add these lines:
Comment #54
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedI have uploaded the patch
Address #53 comment
Please review
Comment #55
demon326 CreditAttribution: demon326 commented#53 and patch from #54 seem to be working
edit: Patch work, but not page navigation is possible.
Comment #56
kopeboy CreditAttribution: kopeboy as a volunteer commentedWhat do you mean?
There is no paging in entity references and if you set a limit in the View with "Display a specified number of items", you are explicitly removing the pager.
I just tested that the pager on another display (of type page) of the same View works.
Comment #57
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #60
Prem Suthar CreditAttribution: Prem Suthar for Drupal India Association commentedCreate the Mr For Issue. Please Review
Comment #61
smustgrave CreditAttribution: smustgrave commentedThis will need test coverage
#54 appears to have dropped a good portion of the original patch, how come? @Prem Suthar see you opened an MR for that too so maybe you know too?