Problem/Motivation
There should be an option to prevent entity reference plugins from referencing itself.
Steps to reproduce
Create a node with an entity reference field that can reference nodes. Observe that the current node can be referenced. It should be possible to prevent this.
Proposed resolution
Add a checkbox titled "Allow an entity to reference itself" to the field settings under "Manage fields". This is checked by default. When un-checked, prevent referencing itself.
Remaining tasks
None.
User interface changes
Add checkbox to field configuration.
API changes
None.
Data model changes
Add configuration for site builders to choose whether to prevent self-references.
Release notes snippet
Comment | File | Size | Author |
---|
Issue fork drupal-2280479
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 #1
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is a start :)
Comment #5
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedSeems sensible. Perhaps we could use this with term parent reference.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedWorkaround - https://www.drupal.org/project/entity_reference_validators by alexpott
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed the test fails and wrote a test specific for this new feature. Should be ready for thorough reviews now :)
Comment #9
jibranMoving to right component
Comment #10
amitaibuIs 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
Comment #11
RoySegall CreditAttribution: RoySegall at Gizra commentedThe 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.
Comment #12
RoySegall CreditAttribution: RoySegall at Gizra commentedI'm still checking it and when the autocomplete element is attached to the form the
$element['#host_entity_id']
inprocessEntityAutocomplete
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.
Comment #13
RoySegall CreditAttribution: RoySegall at Gizra commentedI've got something working for me. It's just a draft. Need more checking.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe 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 :)
Can you please provide an interdiff for your changes in #13?
Comment #15
RoySegall CreditAttribution: RoySegall at Gizra commentedSure.
Comment #16
RoySegall CreditAttribution: RoySegall at Gizra commentedI'm attaching the inner-diff.
Comment #17
pfrenssenI 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.
Comment #18
amitaibuThe 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 :)
Comment #19
pfrenssenI'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.
Comment #20
RoySegall CreditAttribution: RoySegall at Gizra commentedThe question is if the patch works or not. @pfrenssen have you tried the patch from #13?
Comment #21
RoySegall CreditAttribution: RoySegall at Gizra commentedFrom point of view - the patch is OK.
Comment #23
RoySegall CreditAttribution: RoySegall at Gizra commentedUploading again #13 patch
Comment #25
RoySegall CreditAttribution: RoySegall at Gizra commentedSorry, my bad. I uploaded the inner diff again.
Comment #28
RoySegall CreditAttribution: RoySegall at Gizra commentedMabe this will fix it.
Comment #29
RoySegall CreditAttribution: RoySegall at Gizra commented@amateescu the patch seems work OK. Can you give it a look?
Comment #30
pfrenssenDo we have a better name for
$host_entity
? I don't think we commonly use this term to indicate the referencing entity.This line of documentation is incomplete. We also document if statements above the
if
line, not inside it.Comment #31
kristiaanvandeneyndeFrom 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
).Comment #32
RoySegall CreditAttribution: RoySegall at Gizra commentedI fixed the comment. @amateescu set the name host_entity so I just rolled with it.
Comment #33
otherl CreditAttribution: otherl commentedAlmost good, but it doesn't work when you create an entity, only on edit. I made a quick fix for that.
Comment #34
pfrenssenI 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?
Comment #35
kristiaanvandeneyndeWell 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.
Comment #36
amitaibuThis issue one is blocking OG, so would love to see a fix go in.
Comment #37
BerdirThis 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
Comment #38
BerdirAs 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.
Comment #40
tacituseu CreditAttribution: tacituseu commented@Berdir: have you looked at #1699378: Allow tokens in entity reference views selection arguments ?
Comment #41
gpap CreditAttribution: gpap commented[deleted]
Comment #43
gpap CreditAttribution: gpap commented[deleted]
Comment #45
gpap CreditAttribution: gpap commentedComment #46
oriol_e9gBlocking Organic Groups: https://www.drupal.org/project/contrib_tracker/issues/2591017
Comment #47
dpiComment #48
rajanvalecha12 CreditAttribution: rajanvalecha12 commentedComment #49
dpiI 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:
DefaultSelection.php:364
has been broken up.$form['allow_self_reference']
default value was referring to a non-existent variable.Other changes:
Comment #50
dpiUpdated issue summary
Comment #52
jibranCan you please upload the combined patch as well?
Comment #53
kristiaanvandeneyndeKeep 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.
Comment #54
BerdirExactly. 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?
Comment #55
dpiPatch does not depend on #2826826: Entity autocomplete widget does not pass along entity to AJAX request
Comment #56
jibranDo we have to add 'entity' key to some schema as well?
We also need an upgrade path to add this to all existing ER FieldConfig. Something like
field_post_update_remove_handler_submit_setting
.Comment #57
jian he CreditAttribution: jian he commentedAgree 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.
Comment #58
dpi@ #56
The 'entity' key is not invented here. Selection settings is a bit of a dumping ground, its passed straight to the entity_autocomplete element.
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.
Comment #59
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIf 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 theallow_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 itdisallow_self_reference
, the default value needs to beFALSE
Comment #60
jibranThis 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.
Also we are adding a new key to schema so I'm sure we need upgrade path(and upgrade tests).
Comment #61
dpi@ #59
This patch works, but not with autocomplete widgets until #2826826 is committed. It works fine with the other entity reference widgets.
Yep, great.
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.
Comment #62
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRight, but autocomplete widgets are 80% use case, so most users will just see a setting which appears to be broken.
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.Comment #65
otherl CreditAttribution: otherl commentedUnfortunately #55 doesn't work correctly with new entities, so this is a quick fix based on 8.5.5 for now.
Comment #66
gpap CreditAttribution: gpap commented[deleted]
Comment #67
gpap CreditAttribution: gpap commented[deleted]
Comment #70
gpap CreditAttribution: gpap commented[deleted]
Comment #71
gpap CreditAttribution: gpap commented[deleted]
Comment #72
aleevasRerolled up to 8.9
Comment #73
aleevasSorry,I've forgot to check patch applying
This one should be better.
Comment #75
gpap CreditAttribution: gpap commentedComment #76
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #73 for 9.1.x branch, please review.
Comment #79
raman.b CreditAttribution: raman.b at OpenSense Labs commentedThis should resolve the failed test case from #76
Comment #80
Wim LeersI'm shocked this is not already the case! 🤯 I completely forgot about this obvious oversight…
Comment #81
amateescu CreditAttribution: amateescu as a volunteer commented#59 / #62 have not been resolved.
Comment #82
tonytheferg CreditAttribution: tonytheferg as a volunteer commentedGreat 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.
Comment #87
carolpettirossi CreditAttribution: carolpettirossi at ImageX commentedThis 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)
Comment #88
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedFor, d10 #79 patch not applied cleanly.
Fix for d10
Comment #89
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis 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.
Comment #91
BerdirThis 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.
Comment #94
Liam MorlandI have created a merge request with the patch in #88 ported to 11.x.
Comment #95
Liam MorlandThe 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.Comment #96
Liam MorlandComment #97
Liam MorlandPatch from merge request ported to Drupal 10.2.4.
Comment #98
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #99
Liam MorlandThe bot is referring to the patch in 97. The current version is the merge request.
Comment #100
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #101
Liam MorlandPatch from merge request, just to stop the bot from resetting the status.
Comment #102
smustgrave CreditAttribution: smustgrave at Mobomo commentedIf going to add a new UI setting will probably need usability sign off.
Also with schema change going to an upgrade path.