Entity autocomplete widget does not pass along entity to AJAX request

This is being fixed in #2280479: Add setting to EntityReferenceSelection plugins to prevent references to referencing entity :)

I 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.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -88,7 +88,10 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
+      'entity' => $entity,

So 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.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -88,7 +88,10 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
+      'entity' => $entity,

We can just send the uuid.

The 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.

#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.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -88,7 +88,10 @@ class EntityReferenceAutocompleteWidget extends WidgetBase {
+      'entity' => $entity,

Do we want to check for #entity->isNew() before passing?

@ #16

Accounted for unsaved entities.

Fixed entity_type being set despite not having any ids.

As 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.

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.

+++ b/core/modules/system/system.routing.yml
@@ -501,9 +501,10 @@ system.admin_content:
-  path: '/entity_reference_autocomplete/{target_type}/{selection_handler}/{selection_settings_key}'
+  path: '/entity_reference_autocomplete/{target_type}/{selection_handler}/{selection_settings_key}/{host_entity_type_id}/{host_entity_uuid/host_entity_id}'

If that's the case then passing it in the URL seems much better than storing it.

@ #22

If that's the case then passing it in the URL seems much better than storing it.

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.

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.

Yes, 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 the system.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.

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.

That 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.

As 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}':

• Route is very neat. (Compared to new route in above patch)
• Get target entity type from field settings.
• Get host entity type via field info
• Get live selection settings from field settings.
• Avoids KV entirely.
• Patch doesnt consider base fields yet.
• Extra access control: checks if currentUser can edit the field via field access system.
• Method is coupled to field system. Non entity reference autocompletes dont have a "host entity", and can still use existing route..

Patches are experimental only

Seeking direction before thorough code review.

I 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.

Tagging as this is blocking OG Vocab.

from #25:

I can't think of a scenario where the user can't have access to host entity because the user is creating/editing it.

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.

+++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
@@ -106,4 +109,41 @@ class EntityAutocompleteController extends ControllerBase {
+    $host = \Drupal::entityTypeManager()
+      ->getStorage($host_entity_type)
+      ->load($entity_id);
+    if (!$host) {
+      // Triggers on field setting form.
+      return [];
+    }
+
+    if (!$host->{$field_name}->access('edit')) {

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.

Did add a test for php7.2.

PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Don't know why its failing...

will work on comment from #36

Reroll of #32.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
@@ -99,4 +101,49 @@ public function testEntityReferenceAutocompleteWidget() {
+    $this->assertSession()->statusCodeEquals(200);

.

[deleted]

Modification 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...

Reroll of #45 for 8.7.x and 8.8.x.

set issue to "needs review". So we can get a RTbtC :)

@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.

...and #3073970: Add AJAX support to fields using Dynamic Views Arguments via Tokens.

Ugh, we need to reroll this for 8.9 :/

[deleted]

Rerolled patch #47 for 9.1.x, please review.

This should resolve the failed test case from #56

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -173,6 +173,12 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
   +      $element['#autocomplete_query_parameters']['host_entity_type'] = $selection_settings['entity']->getEntityTypeId();
   +      $element['#autocomplete_query_parameters']['host_entity_id'] = $selection_settings['entity']->id();
   +      unset($selection_settings['entity']);
   
   +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -99,6 +99,12 @@ public function handleAutocomplete(Request $request, $target_type, $selection_ha
   +      $host_entity_type = $request->query->get('host_entity_type');
   +      $host_entity_id = $request->query->get('host_entity_id');
   ...
   +        $selection_settings['entity'] = $this->entityTypeManager()->getStorage($host_entity_type)->load($host_entity_id);
   

   Behind the key 'entity' we have the entity loaded from the key 'host_entity_id', which isn't consistent. Let us remove the 'host_' prefix.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
   @@ -103,6 +103,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (!$entity->isNew()) {
   +      $selection_settings['entity'] = $entity;
   

   Why?

3. +++ b/core/modules/system/tests/modules/entity_reference_selection_test/config/schema/entity_reference_selection_test.schema.yml
   @@ -0,0 +1,3 @@
   +# Schema for the entity reference 'entity_test_all_except_host' selection handler settings.
   

   Please stick to 80 char limit. Also we add an empty line after the comment.

4. +++ b/core/modules/system/tests/modules/entity_reference_selection_test/entity_reference_selection_test.info.yml
   @@ -0,0 +1,9 @@
   +name: "Entity Reference Selection plugin test"
   +type: module
   

   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?

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
   @@ -150,6 +152,50 @@ public function testEntityReferenceAutocompleteWidget() {
   +   * rendered on, and is available in the autocomplete results AJAX request.
   

   Shouldn't we use an apostrophe for the genetive case here?:

   results => results'

@hchonov, thanks for the review

Addressed all the pointers

1. +++ b/core/modules/system/tests/modules/entity_reference_test/src/Plugin/EntityReferenceSelection/AllExceptHostEntity.php
   @@ -0,0 +1,40 @@
   + *   entity_types = {"entity_test"},
   ...
   +    if ($entity instanceof EntityTest) {
   

   We do not need those limitations.

2. +++ b/core/modules/system/tests/modules/entity_reference_test/src/Plugin/EntityReferenceSelection/AllExceptHostEntity.php
   @@ -0,0 +1,40 @@
   +    // The host entity should be the same type as the entity type this plugin
   +    // supports.
   

   It should not. We just need to get active if it is the same.

Other than that it looks good to me.

We just need to get active if it is the same.

Not sure if I understood this bit correctly 🤔

I'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.

To 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

Modification of patch #62 to apply to D9.3.

Slight fix in patch.

Fixed CS errors.

Looks good. Moving to RTBC.

Switched 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.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/EntityReference/EntityReferenceAutocompleteWidgetTest.php
@@ -150,6 +157,50 @@ public function testEntityReferenceAutocompleteWidget() {
+    $other = EntityTest::create(['name' => 'dark blue']);
+    $other->save();

This is the only used of $other, can we just not create the variable?

Added the patch #70 and the proposed change in #72. Please review.

Re-roll for 9.3.14

Added patch against #76 in Drupal 10.0.x

#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.

rebased ... and passed ... so i think we're back to review

For 9. patches ... i think it will be backported after ...

Was previously tagged for an issue summary update, if that can be completed please.

Did not review.

Updated the issue summary with the proposed solution, implemented by #80. So back to Need Review.

Clone of the MR to see if it passes 11.x

Passed 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.

Left a few comments on the MR.

I 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).

Add small commit for MR for the request from MR review.

Could we consider the threads there resolved?

Left 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?

some 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"

@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

For 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

Threads 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.

let's try a patch file for 11.x

Since the MR for 10x was reviewed and feedback addressed. Think that carries over to the 11.x version of the patch.

updating issue credits

Committed 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

It 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.