Add setting to EntityReferenceSelection plugins to prevent references to referencing entity

This is a start :)

Seems sensible. Perhaps we could use this with term parent reference.

Workaround - https://www.drupal.org/project/entity_reference_validators by alexpott

Fixed the test fails and wrote a test specific for this new feature. Should be ready for thorough reviews now :)

Moving to right component

Is there a real world use case, one would like to reference itself? Can't think of one, and if so it's probably an edge case?

In other words, I think we don't need to expose a UI for that

The patch is for 8.3.x version but I'm testing it with 8.2.x and the entity object is null. Even when editing.

I'm still checking it and when the autocomplete element is attached to the form the $element['#host_entity_id'] in processEntityAutocomplete have the expected values - null when creating and holds the entity iD when editing.

But no matter what the configuration don't contain the entity object.

I've got something working for me. It's just a draft. Need more checking.

Is there a real world use case, one would like to reference itself? Can't think of one, and if so it's probably an edge case?

The behavior in HEAD is to allow an entity to reference itself, so we can not change that assumption. I introduced the UI option in order to preserver backwards compatibility :)

I've got something working for me. It's just a draft. Need more checking.

Can you please provide an interdiff for your changes in #13?

Can you please provide an interdiff for your changes in #13?

Sure.

I'm attaching the inner-diff.

Is there a real world use case, one would like to reference itself? Can't think of one, and if so it's probably an edge case?

I love coming up with arbitrary use cases, got one :) Imagine you are holding a promotion for your webshop "Buy one item, get a related item of your choice for free" - this could provide an entity reference to a list of related items which could include itself.

provide an entity reference to a list of related items which could include itself.

The thing is you will have to create the content, and then you must edit it and set the reference, because there was no entity ID before. So I would argue that in this case there are probably better ways to model the problem.

If you think it should remain it's possible, I was just thinking a UI for this could be redundant. But I can sleep very well at night even if there was a self-reference + UI :)

I'm not arguing for keeping it or not, I just provided a possible use case :)

The actual argument for keeping it is because of B/C reasons as @amateescu mentioned in #14.

The question is if the patch works or not. @pfrenssen have you tried the patch from #13?

From point of view - the patch is OK.

Uploading again #13 patch

Sorry, my bad. I uploaded the inner diff again.

Mabe this will fix it.

@amateescu the patch seems work OK. Can you give it a look?

1. +++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
   @@ -49,13 +51,14 @@ public function __construct(SelectionPluginManagerInterface $selection_manager)
   +  public function getMatches($target_type, $selection_handler, $selection_settings, $string = '', EntityInterface $host_entity = NULL) {
   

   Do we have a better name for $host_entity? I don't think we commonly use this term to indicate the referencing entity.

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -100,7 +101,12 @@ public function handleAutocomplete(Request $request, $target_type, $selection_ha
   +      if (empty($selection_settings['entity'])) {
   +        // No hos entity
   +        $selection_settings['entity'] = NULL;
   +      }
   

   This line of documentation is incomplete. We also document if statements above the if line, not inside it.

Do we have a better name for $host_entity? I don't think we commonly use this term to indicate the referencing entity.

From what I've seen, core seems to name the host entity $entity and the target entity $target_entity. Mainly because the former is usually available as a function argument or object property ($this->entity).

I fixed the comment. @amateescu set the name host_entity so I just rolled with it.

Almost good, but it doesn't work when you create an entity, only on edit. I made a quick fix for that.

I actually don't think this is the right approach. This doesn't prevent an entity from referencing itself, it simply prevents a reference to an entity from being listed in the autocomplete form. It's still possible to create self references for example by importing data or through REST.

Wouldn't it be better to solve this through a validator like is done in the Entity Reference Validators module?

Well you'd need both. The selector plugin makes sure the UI does not offer invalid options and the constraint makes sure you do not get to save an entity with an invalid reference.

Keep in mind that $entity->validate() needs to be called and handled when importing data through REST or the constraints will not fire at all. Core takes care of that for you when using an entity form by calling $entity->validate() in the form validation.

This issue one is blocking OG, so would love to see a fix go in.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -88,7 +88,10 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
 
     // Append the match operation to the selection settings.
-    $selection_settings = $this->getFieldSetting('handler_settings') + ['match_operator' => $this->getSetting('match_operator')];
+    $selection_settings = $this->getFieldSetting('handler_settings') + [
+      'match_operator' => $this->getSetting('match_operator'),
+      'entity' => $entity,
+    ];
 

This means we serialize the whole entity object into expirable key value, which most likely is going to be unique on every request and a lot of data.

Plus, it's not even the entity with the updated values at the moment, because how content entity forms work, so we can just as well put entity_type and ID in there, then at least we don't have to put the whole serialized entity in there.

Related: #2776571: EntityAutocomplete should pass the original URI to the selection handler

As I think mentioned by others, this is a very narrow use case of the possible things that users want to do.

Here's one that I think is actually more likely: http://drupal.stackexchange.com/questions/225249/entity-reference-field-...

And the referenced issue above wants to filter based on a query argument given to the edit/add form.

Yes, once you have the entity, you can solve those use cases as well with custom

What about adding token support for some or any configuration option, for example views arguments? (which Im not sure are working right now, I don't see where views arguments are expanded to an array when saving..)

It will be more work build this specific use case, as you need to create a new, with an argument and properly configure it. But we'd also be much closer or already support many other things (Filter by another field still ajax stuff for changing it at the same time/new entities, but it would be a start). And token module has tokens for query arguments too.

@Berdir: have you looked at #1699378: Allow tokens in entity reference views selection arguments ?

[deleted]

[deleted]

Blocking Organic Groups: https://www.drupal.org/project/contrib_tracker/issues/2591017

I have moved portions of the patch attempting to solve the autocomplete widget not passing the referencing entity over to the appropriate issue #2826826: Entity autocomplete widget does not pass along entity to AJAX request. The patch there has been improved and is ready for review.

This issue should be about adding the option to DefaultSelection to make it and subclassing selection plugins be able to omit the referencing entity from results

I've rewritten most of the patch as at #43, removing anything to do with the autocomplete widget and route.

Some issues came up while I was reviewing #43, which are solved in attached patch:

• The megaconditional in DefaultSelection.php:364 has been broken up.
• The selection plugin must not exclude entity ID if it is not the same entity type. A test also is added to check this.
• $form['allow_self_reference'] default value was referring to a non-existent variable.

Other changes:

• Refactored exclusion logic in DefaultSelection.php
• Added a bunch of tests
• Had to move precreation of entities in test to not infect the other tests.

Updated issue summary

Can you please upload the combined patch as well?

Keep in mind that this approach still only hides the invalid self-reference from the list of options.

If OG is truly relying on the fact that entities don't reference themselves, it should also add a custom (or new core) constraint to the ER and enforce entity validation to make sure the criteria is always met, not only when using the entity form.

Exactly. The important part is adding a validation constraint for that field, which is pretty easy to do right now without any core changes. Not showing it in the UI is then more a UX improvement and not so much a blocker?

• Fixed/simplified which entity type is referencing what.
• Config is both empty value for *unset* and *boolean-false*, no need to test null

Can you please upload the combined patch as well?

Patch does not depend on #2826826: Entity autocomplete widget does not pass along entity to AJAX request

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -352,6 +360,18 @@ class DefaultSelection extends SelectionPluginBase implements ContainerFactoryPl
+    $entity = $this->configuration['entity'] ? $this->configuration['entity'] : NULL;

Do we have to add 'entity' key to some schema as well?

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -68,6 +69,7 @@ class DefaultSelection extends SelectionPluginBase implements ContainerFactoryPl
+      'allow_self_reference' => TRUE,

We also need an upgrade path to add this to all existing ER FieldConfig. Something like field_post_update_remove_handler_submit_setting.

Agree with Berdir with #54.
Adding a validation constraint is the correct way, and we need to provide a UI to setting validation constraint using for a field.

@ #56

Do we have to add 'entity' key to some schema as well?

The 'entity' key is not invented here. Selection settings is a bit of a dumping ground, its passed straight to the entity_autocomplete element.

We also need an upgrade path to add this to all existing ER FieldConfig.

An upgrade path wouldnt cover instances where entity_autocomplete is used outside of entity forms.

I think it was implied by previous patches/comments that this should be the new default. I'd personally prefer the setting to be 'disallow_self_reference' so the default is falsy. No upgrade path required, and is better for non-entity forms.

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -197,6 +199,12 @@ class DefaultSelection extends SelectionPluginBase implements ContainerFactoryPl
+    $form['allow_self_reference'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Allow an entity to reference itself'),
+      '#default_value' => $configuration['allow_self_reference'],
+    ];

If the patch does not depend on #2826826: Entity autocomplete widget does not pass along entity to AJAX request, then we can't expose this setting in the UI, because it won't work as we won't be receiving anything in $configuration['entity'].

I agree with the latest comments here, so here's how I see this issue moving forward:

- we implement a validation constraint that doesn't allow self-references (best to just steal it from https://www.drupal.org/project/entity_reference_validators)
- we add it conditionally to EntityReferenceItem (in \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::getConstraints()) based on the allow_self_reference handler setting
- we need to keep full BC with this change, so if we name the setting 'allow_self_reference', the default value needs to be TRUE, and if we name it disallow_self_reference, the default value needs to be FALSE

If the patch does not depend on #2826826: Entity autocomplete widget does not pass along entity to AJAX request, then we can't expose this setting in the UI, because it won't work as we won't be receiving anything in $configuration['entity'].

This is a feature and that one is a standalone bug so I think we can postpone this on that one.
Agreed with the rest of the plan.

we need to keep full BC with this change

Also we are adding a new key to schema so I'm sure we need upgrade path(and upgrade tests).

@ #59

If the patch does not depend on #2826826: Entity autocomplete widget does not pass along entity to AJAX request, then we can't expose this setting in the UI, because it won't work as we won't be receiving anything in $configuration['entity'].

This patch works, but not with autocomplete widgets until #2826826 is committed. It works fine with the other entity reference widgets.

we add it conditionally to EntityReferenceItem

Yep, great.

'allow_self_reference', the default value needs to be TRUE, and if we name it disallow_self_reference, the default value needs to be FALSE

Ive discussed with @jibran, it appears we need an upgrade path regardless of default value. I think its safest to change it to `disallow_self_reference` in case the upgrade path is not applied to configuration for whatever reason.

This patch works, but not with autocomplete widgets until #2826826 is committed. It works fine with the other entity reference widgets.

Right, but autocomplete widgets are 80% use case, so most users will just see a setting which appears to be broken.

I think its safest to change it to `disallow_self_reference` in case the upgrade path is not applied to configuration for whatever reason.

Regardless of the configuration being updated or not, the default value will be provided by \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::defaultConfiguration(), so it doesn't matter which name we choose as long we provide the correct default, as mentioned in #59.

Unfortunately #55 doesn't work correctly with new entities, so this is a quick fix based on 8.5.5 for now.

[deleted]

[deleted]

[deleted]

[deleted]

Rerolled up to 8.9

Sorry,I've forgot to check patch applying
This one should be better.

Rerolled patch #73 for 9.1.x branch, please review.

This should resolve the failed test case from #76

I'm shocked this is not already the case! 🤯 I completely forgot about this obvious oversight…

#59 / #62 have not been resolved.

Great to see this on the agenda. It would be really useful to be able to set some parameters on.

For example, I am working on a module where you can reference default product variations from a default product variation. It would be nice in our use case to be able to distinguish whether this restricts the entity types itself or the entities of the entity type.

I tried applying the last patch, but it didn't seem to restrict either.

This would be really really useful. It's very common to have Content referencing other content of the same type to create blocks like "Related Articles", "Recommended products" and editors being responsible to curate these blocks by choosing the specific nodes in the entity reference field.

I tested the patch from #79 and it did not work as I show in the video attached:
Screenshare - 2023-02-07 12_05_04 PM.mp4 (3.96 MB)

For, d10 #79 patch not applied cleanly.
Fix for d10

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

Issue summary should include the proposed solution to the problem.

Also since we are adding a new setting this seems like it could affect existing sites. Think we would need an upgrade path.

This needs to be redone on top of #2826826: Entity autocomplete widget does not pass along entity to AJAX request. That issue already added a test implementation for this, we just need to convert that to a setting like this issue is doing. We can probably remove the test implementation there again.

I have created a merge request with the patch in #88 ported to 11.x.

The upgrade path already works. I installed this on my 10.2.4 site. In my test field, allow_self_reference was already checked and self-reference was allowed, as before. I unchecked it and self-reference was prevented.

Patch from merge request ported to Drupal 10.2.4.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The bot is referring to the patch in 97. The current version is the merge request.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Patch from merge request, just to stop the bot from resetting the status.

If going to add a new UI setting will probably need usability sign off.

Also with schema change going to an upgrade path.

Who does the usability sign-off?

Does this need an upgrade hook or would it be OK to write it so that if allow_self_reference is not set, is is treated as true to preserve existing behaviour?

Can post in #ux for them to take a look, they also keep an eye on the tag.

If a configuration file is being updated it will need an upgrade path.

I rebased the merge request on 11.x. The patch is the current state of the merge request.

Test are now passing.

@smustgrave thanks for the feedback.

The only non-PHP file that this issue updates is core/config/schema/core.data_types.schema.yml. Does that require an upgrade path? If so, what would the upgrade path be? I thought it would pick-up the schema change on the next cache clear.

When you say "post in #ux", is that referring to a Slack channel? I haven't used Slack for Drupal.

I rebased the merge request and test are passing.

If this is a new key that will appear in config during an export then the upgrade path would be to add that key to all entity reference selection plugins.

So, an update hook to set allow_self_reference = TRUE on all entity reference fields? Would it be OK to have it be that if allow_self_reference is NULL, it because as if it were TRUE? That would preserve existing behaviour without the need for an update hook.

Is this key going to appear in config yml file on config export

allow_self_reference will appear in config export.

Gotcha. So that key needs to be set in the upgrade hook. Probably needs to be a configUpdateImporter like views does it so it can be in batches.

Probably has to be set to FALSE to maintain existing behavior

The current behaviour is that self-referencing is possible, which is the meaning of allow_self_reference = TRUE.

Then update hook can set to TRUE. Whichever one keeps existing behavior so people’s sites don’t randomly break