Problem/Motivation

There should be an option to prevent entity selection plugins from referencing the entity. Where an entity selection plugin is called because it is attached as a field to an entity

Remaining tasks

None

User interface changes

Add checkbox to field configuration

API changes

None

Config/Data model changes

Add configuration for site builders to choose whether to prevent self references.

CommentFileSizeAuthor
#55 2280479-entityselection-prevent-self-reference-53.patch9.12 KBdpi
#55 interdiff-2280479-48-53.txt3.65 KBdpi
#49 2280479-entityselection-prevent-self-reference-48.patch9.41 KBdpi
#49 interdiff-2280479-43-48.txt13.58 KBdpi
#43 2280479-43-entity-should-not-reference-itself.patch9.28 KBgpap
#41 2280479-41-entity-should-not-reference-itself.patch9.16 KBgpap
#33 interdiff-2280479-32-33.txt980 bytesotherl
#33 2280479-33-entity-should-not-reference-itself.patch9.18 KBotherl
#32 interdiff-32.txt705 bytesRoySegall
#32 2280479-32-entity-should-not-reference-itself.patch9.14 KBRoySegall
#28 2280479-26-entity-should-not-reference-itself.patch9.09 KBRoySegall
#28 2280479-26-innetdiff.txt1.21 KBRoySegall
#25 2280479-13-entity-should-not-reference-itself.patch9.4 KBRoySegall
#23 2280479-13-inner-diff.patch3.96 KBRoySegall
#16 2280479-13-inner-diff.patch3.96 KBRoySegall
#13 2280479-13-entity-should-not-reference-itself.patch9.4 KBRoySegall
#7 interdiff.txt3.1 KBamateescu
#7 2280479-7.patch10.66 KBamateescu
#7 2280479-7-test-only.patch2.55 KBamateescu
#1 2280479.patch7.63 KBamateescu
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

amateescu’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
7.63 KB

This is a start :)

Status: Needs review » Needs work

The last submitted patch, 1: 2280479.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

moshe weitzman’s picture

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

moshe weitzman’s picture

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.55 KB
10.66 KB
3.1 KB

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

The last submitted patch, 7: 2280479-7-test-only.patch, failed testing.

jibran’s picture

Component: entity_reference.module » entity system

Moving to right component

amitaibu’s picture

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

RoySegall’s picture

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.

RoySegall’s picture

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.

RoySegall’s picture

Status: Needs review » Needs work
FileSize
9.4 KB

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

amateescu’s picture

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?

RoySegall’s picture

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

Sure.

RoySegall’s picture

I'm attaching the inner-diff.

pfrenssen’s picture

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.

amitaibu’s picture

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 :)

pfrenssen’s picture

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.

RoySegall’s picture

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

RoySegall’s picture

Status: Needs work » Needs review

From point of view - the patch is OK.

Status: Needs review » Needs work

The last submitted patch, 16: 2280479-13-inner-diff.patch, failed testing.

RoySegall’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

Uploading again #13 patch

Status: Needs review » Needs work

The last submitted patch, 23: 2280479-13-inner-diff.patch, failed testing.

RoySegall’s picture

Status: Needs work » Needs review
FileSize
9.4 KB

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

The last submitted patch, 13: 2280479-13-entity-should-not-reference-itself.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2280479-13-entity-should-not-reference-itself.patch, failed testing.

RoySegall’s picture

Mabe this will fix it.

RoySegall’s picture

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

pfrenssen’s picture

  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.

kristiaanvandeneynde’s picture

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

RoySegall’s picture

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

otherl’s picture

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

pfrenssen’s picture

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?

kristiaanvandeneynde’s picture

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.

amitaibu’s picture

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

Berdir’s picture

+++ 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

Berdir’s picture

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tacituseu’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gpap’s picture

Status: Needs review » Needs work
gpap’s picture

oriol_e9g’s picture

dpi’s picture

Assigned: Unassigned » dpi
rajanvalecha12’s picture

Assigned: dpi » rajanvalecha12
dpi’s picture

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.
dpi’s picture

Title: Add option to entity reference so entity cannot reference itself » Add setting to EntityReferenceSelection plugins to prevent references to referencing entity
Issue summary: View changes

Updated issue summary

Status: Needs review » Needs work
jibran’s picture

Can you please upload the combined patch as well?

kristiaanvandeneynde’s picture

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.

Berdir’s picture

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?

dpi’s picture

  • 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

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path, +Needs upgrade path tests
+++ 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.

jian he’s picture

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.

dpi’s picture

Assigned: dpi » Unassigned
Issue tags: -Needs upgrade path, -Needs upgrade path tests

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

amateescu’s picture

+++ 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

jibran’s picture

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

dpi’s picture

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

amateescu’s picture

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.