Widgets for entity reference fields are responsible for passing along the entity the widget is rendered on (aka host entity) so entity reference selection plugins know about the entity making the request.
The options buttons (checkbox/radio) and options select pass along the entity. These widgets create selection settings via:
- \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::getOptions
- \Drupal\Core\TypedData\OptionsProviderInterface::getSettableOptions
- \Drupal\Core\Entity\EntityReferenceSelection\SelectionPluginManager::getSelectionHandler
The entity autocomplete is not passing along the entity. Its selection settings are created in \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::formElement
Steps to reproduce:
Select widget
- Set up an entity reference field and reference it to content.
- Use the select list widget for the entity reference field.
- Edit an entity with the entity reference field.
- When the form is being rendered, inspect
\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities
, notice$this->configuration['entity']
contains the entity the field is rendered on.
Autocomplete widget
- Set up an entity reference field and reference it to content.
- Use the autocomplete widget for the entity reference field.
- Edit an entity with the entity reference field.
- When the form is being rendered, inspect
\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities
, notice$this->configuration['entity']
does not exist.
Proposed solution
- In the EntityReferenceAutocompleteWidget FieldWidget, pass the entity to the $selection_settings.
- Add a new #autocomplete_query_parameters property on the Element EntityAutocomplete if the $selection_settings['entity'] is an instance of EntityInterface. This property can handle the entity_type_id and the entity_id of the current entity being edited. We unset then the $selection_settings['entity'] set before to not have a different $selection_settings_key for each entity
- In the FormElement Element, for element with the property #autocomplete_route_name, check if the element has also the #autocomplete_query_parameters property, and if so pass theses parameters as query parameters to the Url built.
- In the EntityAutocompleteController of the system module, we can check now for query parameter 'entity_type_id' and 'entity_id' present in the URL and so load the entity and re-pass it to the $selection_settings
- And now we have the current entity in any EntityReferenceSelection plugin with an autocomplete widget
Comment | File | Size | Author |
---|---|---|---|
#96 | 2826826-autocomplete_host_entity-96.patch | 14.48 KB | vasike |
Issue fork drupal-2826826
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
amitaibuComment #3
amitaibuComment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is being fixed in #2280479: Add setting to EntityReferenceSelection plugins to prevent references to referencing entity :)
Comment #7
dpiComment #8
dpiComment #9
dpiI don't think we should be attempting to solve this bug in a feature request. This issue concerns the autocomplete widget, whereas the #2280479: Add setting to EntityReferenceSelection plugins to prevent references to referencing entity should concern selection plugins only, and is therefore widget agnostic.
The existing patch (currently at comment #2280479-41: Add setting to EntityReferenceSelection plugins to prevent references to referencing entity) is solving the problem incorrectly, by adding an extra part to the `system.entity_autocomplete` route. The entity can be unpacked out of selection settings, so adding a host ID to the entity selection route is unnecessary.
Attached patch is new work, not a derivative of #2280479: Add setting to EntityReferenceSelection plugins to prevent references to referencing entity.
Comment #10
jibranSo this well end up in \Drupal::keyValue('entity_autocomplete'). It will keep increasing the size of table. It will also serialize the entity as well.
Comment #11
jibranWe can just send the uuid.
Comment #12
dpiThe other side is
\Drupal\system\Controller\EntityAutocompleteController::handleAutocomplete
handles autocompletes whether they are for entity_autocomplete attached to entities or regular forms.Existing code may be sending along entity objects in KV. So we'd have to use a different settings key. Also some entities don't implement UUIDs, so theres even more to handle.
Comment #14
dpi#9 contains expected fails.
Attached patch addresses feedback where entity is serialised. Entity keys are now extracted and serialised, then reversed on the autocomplete request.
Comment #15
dpiComment #16
jibranDo we want to check for #entity->isNew() before passing?
Comment #18
dpi@ #16
Accounted for unsaved entities.
Comment #20
dpiFixed entity_type being set despite not having any ids.
Comment #21
BerdirAs mentioned in IRC, the problem with adding even just the entity id/UUID to those options is that we create entity * entity reference key value entries.
The key is based on the hash, so an ID/UUID in there results in a different hash for every entity. At the very least, we would need to switch to an expirable key value store, so we can clean them up but it is still a non-trivial overhead that is being added.
Comment #22
jibranIf that's the case then passing it in the URL seems much better than storing it.
Comment #23
dpi@ #22
Is it going to be a problem that users can enter any origin entity_type/id? We'd have to make sure the user has the same access as it does to the entity form that embedded the entity reference field.
My vote is switching to a expirable backend per #21.
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYes, we would need to validate that the entity type / entity ID combination received in the URL matches the entity type / entity ID we're currently editing. We can do that in
\Drupal\Core\Entity\Element\EntityAutocomplete::processEntityAutocomplete()
where we construct the route parameters for thesystem.entity_autocomplete
route.This approach has the advantage that it doesn't clobber the key/value (expirable) store with a lot of unneeded data, so it would be my preferred way to solve this issue.
Comment #25
jibranThat won't be a problem because
\Drupal\Core\Entity\Element\EntityAutocomplete::getEntityLabels
makes sure user can't see the inaccessible entity labels. I can't think of a scenario where the user can't have access to host entity because the user is creating/editing it.In case, someone brute force last two param of
'/entity_reference_autocomplete/{target_type}/{selection_handler}/{selection_settings_key}/{host_entity_type_id}/{host_entity_uuid/host_entity_id}'
and directly access the URI\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildEntityQuery
still won't show the inaccessible entities.Yes, I think
\Drupal\Core\Entity\Element\EntityAutocomplete::processEntityAutocomplete()
is the place to add the param.Thank you @amateescu for having a look at both issues and suggesting a path forward.
Edit: Fixed the wrong paste.
Comment #26
dpiAs discussed with @jibran and @amateescu on Dslack:
Identifiers in route
I see that the problem of passing arbritary entity type/ID combos is still a concern for #24 / #25. I fear that selection plugins may trust the entity given to them is the real entity generating the request. Which is what putting the ID's into the key store solves.
Ive created a patch in 2826826-autocomplete-host-entity-26--in-route.patch
Field details in route
An alternative implementation, is to completely bypass key value storage, creating a new route. This would direction would apply to base/attached fields only. See 2826826-autocomplete-host-entity-26--field-route.patch
New route would be: '/entity_reference_autocomplete/{field-or-base-id}/{host-entity-id}':
Comment #29
dpiPatches are experimental only
Seeking direction before thorough code review.
Comment #32
mpolishchuck CreditAttribution: mpolishchuck at Alpha Web Group for Dazzle commentedI think another way to implement this (passing host entity info in query parameters) deserves to exist. At least it works for me.
Note: the host entity is available on autocomplete only for editing existing entity. For new entity the entity config param of selection plugin will be still NULL.
Comment #33
mpolishchuck CreditAttribution: mpolishchuck commentedComment #35
heddnTagging as this is blocking OG Vocab.
Comment #36
heddnfrom #25:
OG is a scenario I can think of. Autocomplete w/ view that filters by OG. User has access to add blog posts and page but not create/modify the group entity itself. The OG entity id is what is passed to the autocomplete for filtering all content related to the OG.
I don't think we can assume the entity will always be the host entity. Or at least for the use case I want to use w/ organic groups, it isn't. I want to pass the OG and filter a view of content by the group entity. In that case, the field would be on one entity and the entity context would just be that. Context for a contextual filter.
Comment #37
Martijn de WitDid add a test for php7.2.
Don't know why its failing...
Comment #38
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedwill work on comment from #36
Comment #39
dhirendra.mishra CreditAttribution: dhirendra.mishra at Srijan | A Material+ Company commentedComment #40
jibranReroll of #32.
Comment #42
jibran.
Comment #43
gpap CreditAttribution: gpap commented[deleted]
Comment #45
mpolishchuck CreditAttribution: mpolishchuck at Alpha Web Group for Dazzle commentedModification of #43 (patch for 8.6.7).
Seems like new DrupalSelenium2Driver for the tests does not allow to check server response code. So I'm removing that line from the test for the moment. Anyway if something will go wrong - crawler won't find required element in DOM...
Comment #47
johnnydarkko CreditAttribution: johnnydarkko at Sage Tree Solutions commentedReroll of #45 for 8.7.x and 8.8.x.
Comment #48
Martijn de Witset issue to "needs review". So we can get a RTbtC :)
Comment #49
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@heddn, for the OG use case you mentioned in #36 I think you will need #1699378: Allow tokens in entity reference views selection arguments in addition to this issue.
Comment #50
colan...and #3073970: Add AJAX support to fields using Dynamic Views Arguments via Tokens.
Comment #52
brooke_heaton CreditAttribution: brooke_heaton commentedUgh, we need to reroll this for 8.9 :/
Comment #53
gpap CreditAttribution: gpap commented[deleted]
Comment #55
gpap CreditAttribution: gpap commentedComment #56
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #47 for 9.1.x, please review.
Comment #58
raman.b CreditAttribution: raman.b at OpenSense Labs commentedThis should resolve the failed test case from #56
Comment #59
hchonovBehind the key 'entity' we have the entity loaded from the key 'host_entity_id', which isn't consistent. Let us remove the 'host_' prefix.
Why?
Please stick to 80 char limit. Also we add an empty line after the comment.
Why do we need a new module just for the one selection plugin? Can't we place it in the entity_reference_test module instead?
Shouldn't we use an apostrophe for the genetive case here?:
results => results'
Comment #60
raman.b CreditAttribution: raman.b at OpenSense Labs commented@hchonov, thanks for the review
Addressed all the pointers
Comment #61
hchonovWe do not need those limitations.
It should not. We just need to get active if it is the same.
Other than that it looks good to me.
Comment #62
raman.b CreditAttribution: raman.b at OpenSense Labs commentedNot sure if I understood this bit correctly 🤔
Comment #63
hchonovI've meant the previous condition. We should exclude the host entity ID from the query only if its entity type matches the entity type being queried.
Comment #64
dpiTo aide in testing and reviews, something similar in concept was already committed for Media Library #3038254: Delegate media library access to the "thing" that opened the library
Comment #68
rwohlebModification of patch #62 to apply to D9.3.
Comment #69
rwohlebSlight fix in patch.
Comment #70
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed CS errors.
Comment #71
sourabhjainLooks good. Moving to RTBC.
Comment #72
quietone CreditAttribution: quietone at PreviousNext commentedSwitched this to 10.0.x because it needs to be applied there first. Patch no longer applies, so adding tag.
The Issue Summary here does not explain the proposed resolution and that needs to be added. Adding tag.
This is the only used of $other, can we just not create the variable?
Comment #75
rpayanmAdded the patch #70 and the proposed change in #72. Please review.
Comment #76
Zarpele CreditAttribution: Zarpele at Ocelot for Ocelot commentedRe-roll for 9.3.14
Comment #77
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedAdded patch against #76 in Drupal 10.0.x
Comment #79
bnjmnm#77 was an unnecessary reroll that will not receive credit. It's also an incomplete reroll and misses several changes (notice the difference in patch size). Additional work / review should be based on #76, as #77 breaks several things. As I've mentioned to you several times @Shubham Chandra - you can check if a patch truly needs a reroll by clicking "add test/retest" on the most recent patch and applying it to the most recent Drupal version. Drupal getting a new version doesn't necessarily mean a reroll is needed - and an unnecessary reroll that breaks earlier work is especially not needed.
Comment #81
vasikerebased ... and passed ... so i think we're back to review
For 9. patches ... i think it will be backported after ...
Comment #82
smustgrave CreditAttribution: smustgrave at Mobomo commentedWas previously tagged for an issue summary update, if that can be completed please.
Did not review.
Comment #83
flocondetoileComment #84
flocondetoileUpdated the issue summary with the proposed solution, implemented by #80. So back to Need Review.
Comment #85
flocondetoileComment #86
smustgrave CreditAttribution: smustgrave at Mobomo commentedClone of the MR to see if it passes 11.x
Comment #87
smustgrave CreditAttribution: smustgrave at Mobomo commentedPassed 11.x (committer can remove credit as I didn't do anything but reupload)
Reviewing the MR and changes look good.
Issue summary up to date.
Think it's good for committer review.
Comment #88
BerdirLeft a few comments on the MR.
Comment #89
BerdirI do like the general approach here with the extra query parameters with just the type and ID, it's pretty much the only option that works and won't cause your key value table to explode over time unlike some of the related and similar issues, so +1 to RTBC once those pretty minor things from my review are addressed.
I do want to point out that the chosen approach does have some limitations, it will not pass along a new entity and it will not be able to pass along field values that have been changed in the current edit form, not directly anyway. So you won't be able to write a selection handler that will filter values based on another field reliably, not unless you save first that is. But the introduction of general support for query parameters should kind of allow to make that work when you alter the form element and add extra query parameters and some ajax refresh magic. A bit hacky, as your plugin will then need to read those again, but it should work.
And last: We should add a change record that documents that new feature I think and maybe also one that mentions that this now works (or a combined one).
Comment #90
vasikeAdd small commit for MR for the request from MR review.
Could we consider the threads there resolved?
Comment #91
larowlanLeft some comments in the MR
We also need a change record per comments above.
And did we decide to switch to key value expirable instead?
Comment #92
vasikesome updates for MR threads done.
it would be nice if we could close (resolve) some of them ... so we could focus on what's left ...
So NR ... "temporary"
Comment #93
smustgrave CreditAttribution: smustgrave at Mobomo commented@vasike can you update the MR for 11.x or start a new one.
Also appears to be 1 unanswered thread in the current MR.
Hiding patches
Comment #94
vasikeFor now, updated the current MR with some Kernel tests.
Let's review ... then, if all good, we could move to the current dev branch - 11.x
Comment #95
smustgrave CreditAttribution: smustgrave at Mobomo commentedThreads appear to be resolved. And tests look good. But think it would be good to add a test-only patch, then the updated MR. Hopefully that order doesn't trigger the bot.
Comment #96
vasikelet's try a patch file for 11.x
Comment #97
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince the MR for 10x was reviewed and feedback addressed. Think that carries over to the 11.x version of the patch.
Comment #98
larowlanupdating issue credits
Comment #101
larowlanCommitted 91f59e7 and pushed to 11.x. Thanks!
There is a new API here in the sense of a new render array key, but I don't think there is risk of that breaking anything, and this solves a valid bug in terms of missing feature parity for selection plugins depending on widget, so backported this to 10.1.x
Nice work everyone for getting this over the line
Comment #103
acbramley CreditAttribution: acbramley at PreviousNext for University of Adelaide commentedIt seems like there's still a difference between widgets. When using the OptionsWidget,
$this->configuration['entity']
contains the current revision being edited. When using the autocomplete widget it's the default revision (because we are loading it by entity id).Created #3381333: Entity autocomplete widget sends default revision to entity selection handler for that.