Widget settings to control removing & deleting existing references

Thank you for providing a way to just remove the reference and not the entity itself. I stumbled opon the same translation issue like you do.

I made a reroll of your patch on the current dev.

Tested the patch and it works as expected.Marking it as RTBC.

Agreed - works fantasticly. Hoping to see this in the next version!

Any support for having this change in D7? I know I would like to have the change.... not quite sure how to translate the patch back into 7 though.

There's quite a lot of ambiguity in this issue and #2833972: Widget setting to allow or disallow deleting entities from the system (now closed as a duplicate of this) about what we're talking about.

I'm going to use 'remove' to mean unlink, making an entity no longer referenced from the current entity reference field. I'll use 'delete' to mean really delete the entity so it's gone for everyone everywhere.

I agree that trying to infer from the 'allow_existing' setting (allows users reference existing ...) whether its possible to remove an entity and whether it should be deleted if is removed entities is painful. And I believe that because deleting entities is a dangerous business, it's good to allow fine grained control here.

I suggest we need 2 settings:
First 'Allowing users to remove existing ...'
Second, if that is checked, then offer a dropdown with a 3 way choice
'"Delete ... when removed", and 3 values: "Always", "Never" and "Optionally". "Optionally" would trigger the current checkbox, "Always" and "Never" bypass the need for it.

These setting should be the single source of truth about removal and deletion, the settings about adding new/existing should not be involved.

Some background:
The current logic was introduced by @bojanz in [#1617886]It attempts to infer from the widget settings ("allow existing" and "allow new") whether or not these entities are managed outside of this context and so whether removal should entail deletion. However, the introduction of form modes in D8 makes this inference (even more) questionable.

In my case, I have 2 different IEF widgets in 2 form modes. In one, for ordinary users, I don't allow referencing existing, and in the other for advanced users I do allow referencing existing. In neither case do I want removal to equal automatic deletion, but currently in the one for ordinary users I will get it without warning or choice.

I came looking for this issue after realizing a need for this feature myself. I believe the settings as proposed by @jonathanshaw in comment #8 would fix my problem completely.

I changed the text to "allow delete" so that it's more clear what the configuration is about.
I believe we do not need a new configuration about "Allow remove" as in "remove the reference but keep the entity". The 'Remove' button should be shown in either of 2 cases:

• We need the reference to be empty - the entity has not been saved yet, or the field is not required.
• We need the reference changed. That is allowed either if "allow_existing" is enabled, or we are allowed to delete the entity. `$entity->access('delete')` is enough for the 'delete' case, and we do not need to check for `$settings['allow_delete']`

Please note that #2833972: Widget setting to allow or disallow deleting entities from the system was incorrectly flagged as a duplicate of this issue, because it's the other way around - this issue is newer and it duplicates the older one. Whenever this task is completed, credit should be given to jsbalsera as well.

If deleting is not allowed the checkbox is disabled so that works fine.
But even do a user can not select the delete option the entity is still deleted by default.

• The value $form_values['delete'] is not set when users are not allowed to delete the entity
• Or the entity is only unlinked and not deleted when the $entity_id is empty

The if needs to be reversed in this use case.
The if statement checks when to unlink the entity, but we want it to check when to delete the entity.
Added patch to reverse if statement.

Re #10

I believe we do not need a new configuration about "Allow remove" as in "remove the reference but keep the entity"
...
The 'Remove' button should be shown
(if)
...
`$entity->access('delete')`
...

Inferring that a user *wants* to delete an entity from the mere fact that they *can* delete it seems dangerous and wrong.

This is the core of why I think more settings are needed.

Our use case is as follows;

• Users should not be able to delete the entity when editing a node.
• Users should be able to unlink the entity when editing a node.
• User should be able to delete the entity from /admin/content/media overview.

#10 got it right but did it wrong.
#11 gets it completely right:

We're now not longer infering whether a referenced entity is managed outside of this context. A user should is no longer able to (completely) delete a referenced entity from this widget merely because the user may be allowed to delete the same entity from a different context.

The only thing that is missing: If we really want to control what may happen from within this inline_entity_form, this should be a select rather than a checkbox. Finally the opposite may be true for referenced entities that are exclusively (or primarily) managed through this widget: if their reference is removed, we don't want the orphan entity to stay in the system anymore. So we usually don't want the end user to be able to decide whether it is removed-only or deleted.

This clearly is a major feature in terms of data integrity, I'd even say a task, because the current assumption is flawed.

Major I said. Here's a patch that supports either of these three paradigms:

• Always keep unreferenced entity (recommended whenever these are managed in other contexts, too)
• Let users decide whether to keep or delete unreferenced entities
• Always delete unreferenced entity (recommended whenever these are exclusively managed by this form)

I need your feedback on the most appropriate and concise naming/wording of all options, help texts, config variable etc.
Patch is manually tested in all possible permutations. Fixed some minor coding standard issues as well.

And here's a screenshot of the widget settings:

Just a stupid schema error. To simplify manual testing, I'm also changing the internal order of the config schema, so the patch doesn't conflict with a couple of others here in the issue queue.

I love what you're doing here @Pancho.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -371,10 +420,12 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+        // Allow removal of the reference if it can be changed or empty.
+        $allow_empty = empty($entity_id) || !$element['#required'];
+        // The default removal operation is unlink and the access check for
+        // deleting happens inside the controller buildRemoveForm() method.
+        $allow_change = $settings['allow_existing'] || $entity->access('delete');
+        if ($allow_empty || $allow_change) {
           $row['actions']['ief_entity_remove'] = [
             '#type' => 'submit',
             '#value' => $this->t('Remove'),

@@ -729,7 +780,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    if (!empty($entity_id) && $this->getSetting('removed_reference') === self::REMOVED_OPTIONAL && $entity->access('delete')) {

This role given for $entity->access('delete') no longer makes sense to me.
I don't see why we're always allowing remove simply because the field is not required.

It seems to me that the remove button should not be displayed if:
1. This is a required field and it currently has only ONE reference
2. Removal will cause automatic deletion, but the current user does not have delete permission
3. The allow_existing setting is not true

Do you agree?
I may well be misunderstanding how the current implementation works, but I wanted to agree on basic logic first.

[X-post, still submitting it before turning to Jonathan's valid interjection.]

There it is, finally. Got my local testing environment working again, so I

• fixed the remaining test fail and added extensive test coverage for the new setting
• fixed the settings summary
• returned to self::REMOVED_OPTIONAL as default for compatibility with prior versions and therefore expectations

Apart from that I fixed some minor codestyle issues such as variable type-hints, properly use $this->nodeStorage rather than doing a plain Node::load in one instance, and properly use NodeInterface rather than Node when declaring a parameter. It's just a test, but still...

Locally all tests are passing, so the bot should be happy, too.
The looks are still like in the screenshot in #16. I'm still awaiting some feedback on the optimal wording of settings, help texts, config etc!

Now re Jonathan in #20:

Indeed, this needs a rehaul as well. To get it completely right, however, is even more complicated:

IMHO, the remove button should only be shown if (pseudo-code):

$unsaved = reference is unsaved
$may_remove = removal won't cause automatic deletion || user has permission to delete child entity
$may_add_another = (add_existing is true && another referenceable child entity exists) || (add_new is true && user has permission to create child entity)
$ensure_required = field isn't required || (field is multiple && has one more reference)
if ($unsaved || ($may_remove && $may_add_another && $ensure_required) {
  show button;
}

Did I miss something?
While I'm not gonna find the time to implement this today, as you said, we should first agree on the logic and then we're certainly going to get it implemented and properly tested ASAP. Unassigning for now.

[Cross-posting as well!]

Keep or delete unreferenced entities
- Keep always
- Let the user decide
- Delete always

Select whether a @label should be kept or deleted upon removing the reference.
Keep always is recommended whenever @label are managed in other contexts, too.
Delete always is recommended whenever @label are exclusively managed by this form.

Your wording is generally good. I'm responding on that because you've asked for it, not because I think it needs it badly.

However, I'm not sure about the advice under the selector, it's so simplified that I'm not sure they're helpful. It's not hard to imagine circumstances in which items are managed only by this form but you don't want them removed if deleted (e.g. they can be referenced by multiple parent entities using this widget, or you want to transfer from one to another)

I would probably have done:

Delete if removed
- Always
- Never
- Optionally

Select whether a @label should be deleted altogether if removed as a reference here.<br />
If 'Optionally' is selected then when a user removes a reference, they will be presented
with a checkbox to use if they want the @label deleted.

And given that I'd be tempted by

REMOVED_DELETE_ALWAYS
REMOVED_DELETE_NEVER
REMOVED_DELETE_OPTIONALLY

and

removed_delete:
  type: string
  label: "Delete items if removed"

$unsaved = reference is unsaved
$may_remove = removal won't cause automatic deletion || user has permission to delete child entity
$may_add_another = (add_existing is true && another referenceable child entity exists) || (add_new is true && user has permission to create child entity)
$ensure_required = field isn't required || (field is multiple && has one more reference)
if ($unsaved || ($may_remove && $may_add_another && $ensure_required) {
  show button;
}

Awesome!

But not quite I think. If the field is unrequired, we can remove even if we can't add another. So:

$unsaved = reference is unsaved
$may_remove = removal won't cause automatic deletion || user has permission to delete child entity
$may_add_another = (add_existing is true && another referenceable child entity exists) || (add_new is true && user has permission to create child entity)
$is_not_only_reference = (field is multiple && has one more reference)
$is_not_required = field isn't required || $is_not_only_reference || $may_add_another

if ($unsaved || ($may_remove && $is_not_required) {
  show button;
}

Re #23:

True. How about that:

Select whether a @child should be deleted altogether if removed as a reference here.
Keep always is recommended whenever @child may be referenced by other @parents.
Delete always is recommended whenever @children are exclusively owned by the @parent.

We certainly have to be 100% correct here, and indeed I think we should give the sitebuilder an advice. If they don't quite understand it, they will either figure out or intuitively choose to let the user decide. However, in many if not most cases one or the other restriction will be much better for data integrity then leaving the decision to the user.

Re #24:

Awesome! But not quite I think. If the field is unrequired, we can remove even if we can't add another.

Meh, you're right. I'm gonna work that in ASAP.

Here's a patch. Parts of this were rather complicated... :/

I wonder if there's an easier or better solution to get (and parse) the array with all referenceable entities. Feel free to propose changes! Otherwise note that we have these available now, so let's see (in a followup) if we can use them for other things, too. The "simple" form comes into my mind. Furthermore we could offer radios/checkboxes if there are only few items to choose from, a select if there are some more, and an autocomplete if there are really many.

With the new logic in place I fixed a few more bugs in the "complex" form:

• "Add existing" button was shown even if no referenceable entity exists.

In "simple mode" (complex form, but no reference and only a single operation allowed):

• Autocomplete is shown just because it's the only option, though there's nothing to reference.
• Blank form is not shown, even if creating a new entity is the only option because there's nothing to reference.

Probably this is why there's now a test failure and a couple of subsequent exception. However I wanted to post the patch as is, before I'm going to get hopefully the tests fixed (or otherwise my code)... :)

Nope, I didn't get the "simple mode" right, but now I do. Here's another patch that still fails but fails the right way.

Wow, that was quite some work. A total of three tests were verifying incorrect behaviour, so I rewrote and expanded one of them, while fixing the two others.

Apart from that, I tested all relevant permutations manually to make sure everything works as expected, and from what I could see, it does. Locally it tests green, so the testbot should be happy, too.

To push this forward, we still need a thorough code review, finalization of class method and config names as well as user-facing strings, and some additional test coverage on the "Remove" button. Otherwise this should be ready to go.

Great feature improvement, and nice tests. I'll keep reviewing if you're happy to keep patching.

1. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -145,6 +145,50 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +        'plural' => $definition->getLabel() . 'entities'
   

   Now that we have plurals for entity types (see #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed) maybe we should use that instead of 'entities'.

2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -101,16 +117,16 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +        '#title' => $this->t('Allow users to add new @label.', ['@label' => $child_labels['plural']]),
   ...
   +        '#title' => $this->t('Allow users to add existing @label.', ['@label' => $child_labels['plural']]),
   
   @@ -127,10 +143,24 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +        '#title' => $this->t('Allow users to duplicate @label.', ['@label' => $child_labels['plural']]),
   

   Unintended addition of a leading space?

3. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -127,10 +143,24 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +<em>Keep always</em> is recommended whenever a @child may be referenced by another @parent, too.<br />
   +<em>Delete always</em> is recommended whenever a referenced @child is exclusively owned by the @parent.', [
   

   Actually it could be referenced by something other than a @parent. And I'm not sure I'd go as far as always 'Recommending' keep always. Perhaps "Delete always is recommended whenever each @child is exclusively managed within a single @parent. Otherwise Keep Always is safest."

4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -229,12 +284,32 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +    $referenceable_entities = $all_entities;
   

   I don't see a point to $all_entities.

5. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -229,12 +284,32 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
        $entities = $form_state->get(['inline_entity_form', $this->getIefId(), 'entities']);
   +    $entities_count = count($entities);
   +    foreach ($entities as $value) {
   +      /** @var EntityInterface $entity */
   +      $entity = $value['entity'];
   +      $id = $entity->id();
   +      if (in_array($id, array_keys($referenceable_entities))) {
   +        unset($referenceable_entities[$id]);
   +      }
   +    }
   

   I'm not sure we need this. We only seem to care about the existence of some not yet referenced entities, not what they are. Can we just do something like
   $haveUnreferencedEntities = $entities_count < count($all_entities);
   And even if $entities_count could be inflated by duplicates, it would still be faster to deduplicate it to get $unique_entities_count than to iterate through all possible referenceable entities; there could literally be a million of those.

6. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -229,12 +284,32 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +    $allow_duplicate = $settings['allow_duplicate'] && $this->canAddNew();
   
   @@ -355,7 +430,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -        if ($settings['allow_duplicate'] && !$cardinality_reached && $entity->access('create')) {
   +        if ($allow_duplicate && !$cardinality_reached) {
   

   This seems to be an unrelated change in logic.

7. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -371,10 +446,14 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +        $may_add_existing = $allow_existing && count($referenceable_entities) > 1;
   
   @@ -420,16 +499,19 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +      $may_add_existing = $allow_existing && !empty($referenceable_entities);
   
   @@ -494,7 +576,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +      if ($allow_existing && !empty($referenceable_entities)) {
   

   There's both a variation and a repetition in the logic here. Is the variation intended? If not, should we DRY out the repetition?

8. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -820,21 +902,25 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +    // Entity hasn't been saved yet or we may not remove it, so unreference it.
   ...
   +    // Delete entity if configured so or if the user chose to.
   

   These comments don't quite make sense as a pair, especially the first one.

   Maybe:
   'Remove the reference'
   and then
   'If the entity has been saved, delete it if either the widget is set to always delete, or the widget is set to let the user decide and the user has decided to delete.'

9. +++ b/src/Tests/ComplexWidgetWebTest.php
   @@ -277,6 +310,7 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
   +    $this->updateSetting('removed_reference', 'optional');
   

   If this is the default, we should avoid explicitly setting it here, in order to ensure BC?

10. Changes to the tests of assertions that are unrelated to this issue is quite a concern. It requires a paranoid level of review scrutiny, as the default assumption must be that we accidentally broke something, not that the test is wrong. If the test fixes can't be split into another issue, I suggest you post a comment explaining what had to be changed and why it had to be done here?

11. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
    @@ -472,7 +533,7 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
    -   * @param $entity_form
    +   * @param array $entity_form
    
    +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
    @@ -43,19 +58,19 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
        *   The plugin implementation definition.
    ...
    +   * @param FieldDefinitionInterface $field_definition
    ...
    -   * @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info
    +   * @param EntityTypeBundleInfoInterface $entity_type_bundle_info
    ...
    -   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    +   * @param EntityTypeManagerInterface $entity_type_manager
    ...
    -   * @param \Drupal\Core\Entity\EntityDisplayRepositoryInterface $entity_display_repository
    +   * @param EntityDisplayRepositoryInterface $entity_display_repository
    ...
    -   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   * @param ModuleHandlerInterface $module_handler
    
    @@ -420,16 +499,19 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -    $create_bundles_count = count($create_bundles);
    
    @@ -459,7 +541,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -        if ($create_bundles_count > 1) {
    +        if (count($create_bundles) > 1) {
    
    @@ -774,9 +856,9 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -   * @param $form
    +   * @param array $form
    ...
    -   * @param $form_state
    +   * @param FormStateInterface $form_state
    
    @@ -799,9 +881,9 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -   * @param $form
    +   * @param array $form
    ...
    -   * @param $form_state
    +   * @param FormStateInterface $form_state
    
    @@ -820,21 +902,25 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * @param FormStateInterface $form_state
    
    @@ -891,9 +977,9 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -   * @param $form
    +   * @param array $form
    ...
    -   * @param $form_state
    +   * @param FormStateInterface $form_state
    
    @@ -925,9 +1011,9 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
    -   * @param $form
    +   * @param array $form
    ...
    -   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * @param FormStateInterface $form_state
    
    +++ b/src/Tests/ComplexWidgetWebTest.php
    @@ -2,7 +2,7 @@
    -use Drupal\node\Entity\Node;
    +use Drupal\node\NodeInterface;
    
    @@ -162,12 +195,12 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -    $this->assertTrue($node->get('first_name')->value == 'John', 'First name in reference node set to John');
    -    $this->assertTrue($node->get('last_name')->value == 'Doe', 'Last name in reference node set to Doe');
    +    $this->assertEqual($node->get('first_name')->value, 'John', 'First name in reference node set to John');
    +    $this->assertEqual($node->get('last_name')->value, 'Doe', 'Last name in reference node set to Doe');
    ...
    -    $this->assertTrue($parent_node->multi->target_id == $node->id(), 'Refererence node id set to ' . $node->id());
    +    $this->assertEqual($parent_node->multi->target_id, $node->id(), 'Refererence node id set to ' . $node->id());
    
    @@ -211,16 +244,16 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -   * @param \Drupal\node\Entity\Node $node
    +   * @param NodeInterface $node
    ...
    -  protected function checkNestedEntityEditing(Node $node, $ajax_submit = TRUE) {
    +  protected function checkNestedEntityEditing(NodeInterface $node, $ajax_submit = TRUE) {
    ...
    -    /** @var \Drupal\node\Entity\Node $level_1_node */
    +    /** @var NodeInterface $level_1_node */
    ...
    -    /** @var \Drupal\node\Entity\Node $level_2_node */
    +    /** @var NodeInterface $level_2_node */
    
    @@ -285,7 +319,7 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -    /** @var \Drupal\node\NodeInterface $node */
    +    /** @var NodeInterface $node */
    
    @@ -475,7 +626,7 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -    /** @var \Drupal\node\NodeInterface $node */
    +    /** @var NodeInterface $node */
    
    @@ -496,7 +647,7 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -    $duplicate = Node::load(4);
    +    $duplicate = $this->nodeStorage->load(4);
    
    @@ -690,9 +841,9 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -   * @param $existing_node_text
    +   * @param string $existing_node_text
    ...
    -   * @param $expected_error
    +   * @param string $expected_error
    
    @@ -752,11 +903,11 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    -   * @param $nested1_title
    +   * @param string $nested1_title
    ...
    -   * @param $nested2_title
    +   * @param string $nested2_title
    ...
    -   * @param $nested3_title
    +   * @param string $nested3_title
    

    There's an awful lot of minor fixes going into this patch that are unrelated to this issue. My understanding is this practice is highly discouraged, as it makes reviewing much harder, increases collisions with other patches, and drastically reduces the chance of an issue every actually getting resolved.

@jonathanshaw: Thank you so much for your thorough review! :)
I will go through it one by one within the next days, probably next week, so we get the remaining issues tackled.

#28 works with me but it returns a fatal error at:

$definition = $this->entityTypeManager->getDefinition($bundle);

I think this is because getDefinition function takes entity type as argument not bundle!

The following patch fixes this issue:

#31 Seems to be working see screenshot:

The default option is 'Select' so there is no behaviour change if this is added to the module and configuration is not updated.

In the screenshot, I changed the default to keep always just to test it.

After applying the patch, and setting it to keep always, I don't see the option in the entity inline form to delete the entity anymore.

Looks good to me +1

I did not test setting the value to always delete. So setting the status to needs review.

Patch at #31 works for me against 1.0 rc2.
+1

Helps us with another problem we had where references are being set incorrectly behind the scenes and this patch allows us to remove the reference without deleting the referenced entity.

#31 applies cleanly to 1.0 rc2 but not to rc3 or dev

Thanks for working on this. As currently most active maintainer i read this issue in whole, and i'm not decided how to feel about it. Also no other maintainer commented yet on this.
This is a huge (41k) patch. My first feeling is, this adds a huge maintenance burden to an already quite bloated module. We're basically re-implementing paragraphs. But i see the point of it.

Looking into the patch, a substantial part of it are tests. Which is a good thing.
Another part of it is unrelated typehinting. Which is as separate patch good, but here bad as it makes the patch harder to review.

So i'm honest that i'm skeptical if this or all of this should go in. But if someone rerolls the patch to current dev, AND removes all unrelated changes, i'll review and ask other maintainers to.

I've rerolled the patch to current dev and removed some of the unrelated changes. Seems to be working for me now but it's definitely possible that I've missed something or not removed something that needed removing.

One test failure in 36 is because of this unnecessary removal:

use Drupal\node\Entity\Node;

I've added that back in.

The other test fail is the newly added testRemoveReference() test, which was added earlier in this issue. The test code there is designed to work in Simpletest, but since writing the test, ief has moved from simpletest to javascript functional tests, see https://www.drupal.org/project/inline_entity_form/issues/2974544.

Therefore we need to update testRemoveReference() to work with the javascript functional tests.

For now, I've removed it completely so we can check whether everything else still works.
interdiff is comparing 36 with 37. I'll work on updating the test next...

38 corrects a typo in 37...

Uploaded the wrong files! Doh.

Here's the correct version of the patch, based on 36, that adds in use Drupal\node\Entity\Node and removes the simpletest test (which will need to be re-added as a functional js test). Hopefully this will go green.

Re-uploading patch from 41 first, to trigger testbot.

Great, I'll now work on porting the test.

I've stripped out a few more unrelated changes, and refactored testRemoveReference to work with our new functional javascript suite.

Thanks for working on this @nicrodgers! As you're now very familiar with the patch, it would be great if you could have a look at #29 and at least say which of those points are still valid.

Sure, no problem - I'll take a look this afternoon and come back with an update...

Addressing each point in #29:

1. I've implemented your suggestion and have refactored the code as the if/else was redundant.
2. This has been addressed in a previous version of the patch.
3. I've gone with your suggested wording.
4. Agreed, deleted.
5. I'm not sure about this, I've left it as-is and would welcome any further input.
6. I believe this is related. A few extra lines have changed as a result to make it more DRY.
7. This has been addressed in a previous version of the patch.
8. Agreed, changed.
9. Agreed, changed (and added a comment to replace it, for clarity).
10. This has been addressed in a previous version of the patch, there are no longer any changes to existing tests.
11. This has been addressed in a previous version of the patch, all unrelated changes should be gone now, but if you spot any I've missed please let me know.

One other thing I spotted is that we're using \Drupal::service to load plugin.manager.entity_reference_selection within getReferenceableEntities() but we should be using dependency injection here. However, we also appear to be doing that in existing code (in isTranslating()) so I've left that as-is.

@jonathanshaw if you have time to re-review this, hopefully we can get it towards the finishing line?

I think #29.5 is still valid, though I'm unsure of how important it is. All we do with the $referenceable_entities is establish later on whether count($referenceable_entities) > 1.

I'd hope there is a better way of finding out if the total count of the referencable entities is greater than the count of already referenced entities, other than loading a monster array of all possible referencable entities.

1. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -245,12 +300,29 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +    $allow_existing = $settings['allow_existing'];
   +    $allow_duplicate = $settings['allow_duplicate'] && $this->canAddNew();
   +    $allow_new = $settings['allow_new'] && $this->canAddNew();
   

   I think it would be better if we called these $may_add_existing, $may_duplicate and $may_add_new, both to indicate they are not simply identical to the comparable $settings['allow_ , and to conform with the variable naming we use later.

2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -245,12 +300,29 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -    $cardinality = $this->fieldDefinition->getFieldStorageDefinition()->getCardinality();
   -    $cardinality_reached = ($cardinality > 0 && $entities_count == $cardinality);
   +    $field_storage_def = $this->fieldDefinition->getFieldStorageDefinition();
   +    $cardinality = $field_storage_def->getCardinality();
   +    $cardinality_reached = ($cardinality !== $field_storage_def::CARDINALITY_UNLIMITED && $entities_count == $cardinality);
   

   This seems like an unrelated change.

3. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -387,10 +459,14 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +        $always_delete = $settings['removed_reference'] !== self::REMOVED_DELETE;
   

   I don't understand why the negation. And if this is wrong, that suggests our test coverage is incomplete.

4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -387,10 +459,14 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +        $may_add_existing = $allow_existing && count($referenceable_entities) > 1;
   

   Let's move this to where we currently define $allow_existing earlier

5. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -624,9 +700,9 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -            'target_type' => $element['#target_type'],
   -            'handler' => $element['#selection_handler'],
   -          ] + $element['#selection_settings'];
   +              'target_type' => $element['#target_type'],
   +              'handler' => $element['#selection_handler'],
   +            ] + $element['#selection_settings'];
   

   Unrelated indentation change.

6. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -825,7 +901,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -      '#allow_existing' => $this->getSetting('allow_existing'),
   +      '#removed_reference' => $this->getSetting('removed_reference'),
   

   I'm hesitant about this change, whether it has BC implications. It might be better to simply add #removed_reference, not remove #allow_existing.

7. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -889,13 +965,19 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   +    // The entity hasn't been saved yet (or we may not remove it), so remove the
   +    // reference.
   

   I think this should say the opposite: "The entity hasn't been saved yet or is being deleted, so remove the reference."

Thanks again for your review @jonathanshaw. I think I have been able to further simplify things. Lets see if the tests still pass with this version...

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -387,10 +459,14 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
-        // If 'allow_existing' is on, the default removal operation is unlink
-        // and the access check for deleting happens inside the controller
-        // removeForm() method.
-        if (empty($entity_id) || $settings['allow_existing'] || $entity->access('delete')) {
+        // Check if reference may be removed. If yes, show "Remove" button.
+        // The default removal operation is unlink and the access check for
+        // deleting happens inside the controller buildRemoveForm() method.
+        $always_delete = $settings['removed_reference'] !== self::REMOVED_DELETE;
+        $may_remove = !$always_delete || $entity->access('delete');
+        $may_add_existing = $allow_existing && count($referenceable_entities) > 1;
+        $is_not_required = !$element['#required'] || $entities_count > 1 || $allow_new || $may_add_existing;
+        if (empty($entity_id) || ($may_remove && $is_not_required)) {
           $row['actions']['ief_entity_remove'] = [
             '#type' => 'submit',
             '#value' => $this->t('Remove'),

You seem to have removed this hunk. That undoubtedly makes things simpler, but I don't think you can.

Pancho and I discussed this a lot in #22 to #25. When deciding whether the remove button can be shown there seems to be a real need to consider
(1) whether removing would cause a deletion that the user doesn't have permission to do
(2) whether removing would make it impossible for the user to satisfy the requirements of the field

You could argue that (2) is an unrelated improvement but the logic is so inter-tangled here I'm not sure we help anyone by punting a couple of lines to a separate issue. It's debatable.

An interdiff really really help when reviewing, maybe I've not understood the sutlety of what you've done.

I don't see a connection between this issue and when the remove button should be displayed or not. The existing logic around that is unchanged in my latest patch.

This issue is about what happens when the remove button is pressed - does it remove the reference to the entity and the entity itself, or just the reference. This is why I removed the changes from the last patch.

The interdiff for the last patch was almost as big as the patch itself, so I didn't bother. Hope that didn't cause too much trouble for you.

I'd be good to get feedback from some more people here. Especially around manual testing. We are about to deploy the patch to production on our site.

This issue is about what happens when the remove button is pressed - does it remove the reference to the entity and the entity itself, or just the reference.

That's not what the issue summary & title say. We used to have seperate issues for removal and deletion but we merged them into this one as the considerations were so intertwined. Since #8 and #12 we've been pursuing a joint solution to giving more granular control to removal and deletion.

#50 may well be genius, but it doesn't have any explanation or interdiff and seems to change the issue scope significantly, so it's hard to understand how it relates the previous work.

It's possible that the issue has now matured to the point where we can split remove & delete into separate issues again, that's an intriguing idea. I'll try to review later with that in mind.

Here's the interdiff

OK, so #50 does the following:

1. It removes the trivial unrelated changes identified in #49.2 and #49.5, and makes the comment change suggested in #49.7
2. It makes the trivial BC accomodation of retaining #allow_existing as suggested in #49.6
3. It removes the hunk identified in #51 and therefore is able to revert various other minor refactorings that supported that.

Having looked at this more closely, I'm now sure that is not OK to remove the hunk identified in #51. The problem is that if 'allow_existing' is set, and the removed reference setting is 'delete', but the user does not have delete permissions, then the remove button will be shown, and the entity will be deleted upon removal. So this changes leads to a permissions bypass.

You could argue that we should fix this by adding a permissions check in submitConfirmRemove, or wherever deletion actually happens. But I think this leads to confusion: we would have a setting that suggests it means "Alway delete on removal" but actually there would be a hidden caveat "(if you have delete permission)". I think if a sitebuilder has set this setting, then probably their data model is such that they don't want removed-but-undeleted entities floating around; and therefore if akways-delete-on-remova is specified then we should not allow removal if deletion is not possible. Therefore I think this change needs to be walked back.

This is a partial revert of #50 to #48 as discussed in #55.

RTBC for maintainer review, though possibly NW for #29.5

Addressing Axel @geek-merlin's #35:

this adds a huge maintenance burden to an already quite bloated module. We're basically re-implementing paragraphs. But i see the point of it.

So i'm honest that i'm skeptical if this or all of this should go in. But if someone rerolls the patch to current dev, AND removes all unrelated changes, i'll review and ask other maintainers to.

I hope that with the unneeded changes removed, this patch seems like less of a maintenance burden.

My perspective is that the feature of optionally deleting references definitely does add complexity to this module. But this issue doesn't add that feature. It simply rationalises it, removing it's unnecessary coupling with the 'allow existing' feature.

Thanks for working on this! 42k => 20k looks good. I'll review this soon-ish.

And: As we seem to have some knowledge and experience round here, i'd be interested in thoughts if and how this relates to #3105261: Inline Entity Form: Optional #save_form as field config option?.
(I thought that currently we references are only saved with parent entity so why would i reference an existing entity just to dereference-and-delete it. Or is there something i do not get?)

Here's a first review.

1. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -145,6 +145,42 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  protected function getParentEntityType() {
   +    return $this->fieldDefinition->getTargetEntityTypeId();
   +  }
   

   Shouldn't this better be inlined?

2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -145,6 +145,42 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  protected function getParentBundle() {
   +    return $this->fieldDefinition->getTargetBundle();
   +  }
   

   Shouldn't this better be inlined?

3. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -145,6 +145,42 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  /**
   +   * Gets the human-readable names of the entity type.
   +   *
   +   * @todo Remove workaround as soon as #2765065 lands.
   +   *
   +   * @return array
   +   *   Array of labels with the keys 'singular' and 'plural'.
   +   */
   +  protected function getParentBundleLabels() {
   +    $definition = $this->entityTypeManager->getDefinition($this->getParentEntityType());
   +    return [
   +      'singular' => $definition->getSingularLabel(),
   +      'plural' => $definition->getPluralLabel(),
   +    ];
   +  }
   

   This has only one reference, so let's inline it too.

4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -184,6 +220,23 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  /**
   +   * Gets all referenceable entities for this form.
   +   *
   +   * @return string[]
   +   *   Array of labels of all referenceable entity labels keyed by entity ID.
   +   */
   +  protected function getReferenceableEntities() {
   +    $bundles = \Drupal::service('plugin.manager.entity_reference_selection')->getSelectionHandler($this->fieldDefinition)->getReferenceableEntities();
   +    $entities = [];
   +    foreach ($bundles as $bundle) {
   +      foreach ($bundle as $id => $label) {
   +        $entities[$id] = $label;
   +      }
   +    }
   +    return $entities;
   +  }
   +
   

   That was the code criticised.
   We do not need this if we omit the below logic...

5. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -385,10 +455,14 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -        // If 'allow_existing' is on, the default removal operation is unlink
   -        // and the access check for deleting happens inside the controller
   -        // removeForm() method.
   -        if (empty($entity_id) || $settings['allow_existing'] || $entity->access('delete')) {
   +        // Check if reference may be removed. If yes, show "Remove" button.
   +        // The default removal operation is unlink and the access check for
   +        // deleting happens inside the controller buildRemoveForm() method.
   +        $always_delete = $settings['removed_reference'] !== self::REMOVED_DELETE;
   +        $may_remove = !$always_delete || $entity->access('delete');
   +        $may_add_existing = $allow_existing && count($referenceable_entities) > 1;
   +        $is_not_required = !$element['#required'] || $entities_count > 1 || $allow_new || $may_add_existing;
   +        if (empty($entity_id) || ($may_remove && $is_not_required)) {
              $row['actions']['ief_entity_remove'] = [
   

   So here's the code that uses $referenceable_entities. TBH i do not thoroughly understand it, it might benefit from better comments.
   It looks to me like "Remove" button is not shown when an entity is "required"(in some sense), which is not the case when we have more than one referencable entity (why >1?).
   This may be wrong. What if we always show the remove button, and only rely on validation?

$is_not_required = !$element['#required'] || $entities_count > 1 || $allow_new || $may_add_existing;
+        if (empty($entity_id) || ($may_remove && $is_not_required)) {
           $row['actions']['ief_entity_remove'

The idea is that if the element is required, and it currently has only one entity, then the remove button should not be shown unless it's possible for the user to add an entity to replace the one they remove.

My concern with ditching this and relying on validation is: might it not be possible for a user to get the form into an unrecoverable state? After validation, won't the form be reloaded with the field marked as having an error, but with the entity no longer present in it and potentially the user not able to restore it or add another?

It's an edge case no doubt, but that seems like a serious UX and data loss concern.

Ah OK!

So if user can delete but not create, they can get an unrecoverable state.
So may be we leave it up to the sitebuilder if they want to shoot their foot.

And IF we want to keep it, the code must be easy to understand and review. That's possible.
In that case, let's describe this unrecoverable state and its exact conditions in a comment.
Or even better, in code: Rewrite the condition as something like $form_gets_unrecoverable to be more intuitive.
Also, that condition only relies on the count. So go leverage \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::countReferenceableEntities
That way we kill another method, and need not have huge-count concerns.

What u think?

I haven't tested this, even manually, but it attempts to address #59 and #29.5.

I used \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::countReferenceableEntities but it doesn't actually reduce the code count, because to do this properly I think we need to consider the possibly that widget is currently referencing the same entity multiple times, so we have to iterate over and deduplicate the current references.

So may be we leave it up to the sitebuilder if they want to shoot their foot.

I'm doubtful about this, because it's not simply a matter of misconfiguration by the sitebuilder. It's possible that there are legitimate uses cases for a required field where some users can remove items but not add new ones, but other users can do both. Without this logic, there's no way to configure the site to handle this case.

But even without this, putting warnings in the UI about this edge case might end up being more of a maintenance headache than handling it properly.

Doh

Failures are because $element doesn't have the keys we need at this point:

    $options = [
      'target_type' => $element['#target_type'],
      'handler' => $element['#selection_handler'],
    ] + $element['#selection_settings'];

Need to get the settings another way.

OK, this is what #62 should have been, sorry for the noise.

The test fail puzzles me. I can't reproduce the failure when I reproduce testEntityCreation manually. Anyone got any ideas?

Our team have been complaining about slow build times over the last few weeks. It's taken a while, but we traced it back to this issue. Our builds (which run a comprehensive Behat test suite of around 20,000 steps) take 25% longer with this patch (we're using #56) installed.

To put it in to perspective, our builds using IEF rc6 without the patch take 4 hours. With this patch (and no other changes) it takes 5 hours.

I haven't had a chance to debug why yet, or to look at a fix, hopefully I'll get some time later this week, unless anyone beats me to it.

@nicrodgers: It really helps if you can cross-profile the countReferenceableEntities() approach from #66.

Sure, I'll try a build with patch 66 overnight and will feedback in the morning.

I ran two test builds (to get an average) of IEF RC6 with patch #66 for comparison. It's a big improvement over the earlier patches.

Average build time, IEF6, no patch: 4 hours
Average build time, IEF6, patch #55: 5 hours
Average build time, IEF6, patch #66 4 hours 8 minutes

I'll take a look at the failing tests in #66 now...

That test was failing because there were already some error messages showing, so it wasn't waiting long enough for the new message to appear. Whilst not strictly related to the fail, it did highlight the underlying error, the cause of which was a typo in the selection_handler/handler key/option value.

Here's an updated patch that fixes that, which will hopefully turn this green again...

Great!

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -244,9 +299,32 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    // Determine if there are unreferenced entities.
+    $entity_ids = [];
+    foreach ($entities as $key => $value) {
+      if (!empty($value['entity']->id())) {
+        $entity_ids[] = $value['entity']->id();
+      }
+    }
+    $unique_entities_count = count(array_unique($entity_ids));
+    $selection_settings = $settings = $this->getFieldSetting('selection_settings') ? $this->getFieldSetting('selection_settings') : [];
+    $options = [
+      'target_type' => $this->getFieldSetting('target_type'),
+      'selection_handler' => $this->getFieldSetting('handler'),
+    ] + $selection_settings;
+    /** @var \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface $handler */
+    $handler = $this->selectionManager->getInstance($options);
+    $have_unreferenced_entities = $handler->countReferenceableEntities() > $unique_entities_count;

@@ -385,10 +463,22 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+        $can_add_existing = $allow_existing && $have_unreferenced_entities;
+        $reference_is_not_required = !$element['#required'] || $entities_count > 1 || $allow_new || $can_add_existing;

I think I've been dumb. We don't need to deduplicate to get unique entities, because the count only matters if we're down to the last referenced entity (because all we're doing is ensuring that the required property is satisfiable). All we need to know is that countReferenceableEntities is greater than 1!

Ha, yes - that makes sense. Do you want to try re-rolling with the simplified logic? The final patch will be 1kb at this rate :)

I noticed an issue with the remove logic, fixed.

#73 makes me think there might be an unrelated bug in extractFormValues:

          $options = [
            'target_type' => $element['#target_type'],
            'handler' => $element['#selection_handler'],
          ] + $element['#selection_settings'];
          /** @var \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface $handler */
          $handler = $this->selectionManager->getInstance($options);

Maybe this should be 'selection_handler' too.

#73 was in the right direction, but not quite right, and therefore #78 was a mistake. This is what #73 should have been, plus #77.

The CR from Entity reference selection handlers should extend a base class is the best source of truth I've been able to find about Entity Refeence selection plugin configuration keys.

@jonathanshaw thanks for your continued work on this. Setting to NR to run the tests on patch #80.

Can you elaborate on what was wrong in 73? Some manual steps to reproduce the issue, and/or an automated test would be great. I am happy to help expand the test coverage if needed.

My process was this:
- I was manually testing #77 and found that the remove button was present when it shouldn't be
- debugging showed it was because countReferenceableEntities was returning a count of all nodes not just nodes from the target bundle
- I tried to fix that in #78 but got that weird failure I don't understand, although it did fix the problem I'd discovered manually
- Looking at it more, I noticed that there is in the config no field setting called 'selection_settings' so that line must be wrong
- Finding the CR I realised that the correct field setting is called 'handler_settings'
- But I also realised from the CR that the other setting is called 'handler' not 'selection_handler'
- implementing these the tests pass and it works right on manual testing

I've hesitated to raise the issue of more test coverage, but ...
The remove button logic is the most complex bit of this patch and it's effectively untested. To do this right I think we really need a test like

public function testRemoveButtonPresence($existing_entities, $referenced_entities, $allow_new_setting, $allow_existing_setting, $removed_reference_setting, $field_is_required,  $userHasDeletePermission, $userHasCreatePermission, $expectRemoveButtonInitially, $expectRemoveButtonAfterAddNew) {
  // Create the existing entities.
  // Create and save a node that references the referenced entities
  // Set the required property on the field.
  // Set the settings on the widget
  // Set the permissions on the user
  // Load the node and for each referenced entity assert that presence/absence of the remove button
  // If $allow_new, add a new entity and assert the presence/absence of the remove button on all referenced entities
}

public function removeButtonPresenceDataProvider() {
  // A rather long data provider.
}

But I don't think I can undertake to create it.

Wow! Rock on! 💪

Thank you guys for the amazing work so far for this feature, it's really nice.

I spent some time to thoroughly review the patch and the functionality itself.
I have found couple of minor issues that I have fixed:

• Removed unused variables from the code.
• Rephrased the widget description to highlight that the Keep always option is the safest if the Create new revision option is used. (If deleting the entity and the parent entity gets reverted, the parent entity field will hold invalid reference ids to the deleted entity.)
• Extended the test scenario to ensure the delete permission is taken in account, this part was missing.
• Made sure protected methods are after public test methods in the ComplexWidgetTest.php.
• Fixed many typos.
• Fixed coding standard issues.
• Made the code more readable.

Regarding the tests @jonathanshaw I have checked all the tests so far and the presence of the Remove button is asserted in multiple test scenarios. It's true, there is no dedicated test for Remove button presence but what we have currently and with the test in the patch, I think it covers the cases you have mentioned in your comment.

I think the patch is now ready for a final review.

Thanks @joevagyok! I think this is test coverage enough.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -386,10 +459,25 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+        // Determine if a reference may be removed.
+        // Unless the user has permission to delete the entity, then they should
+        // not be able to remove it if that will lead to its deletion.
+        $always_delete = $settings['removed_reference'] !== self::REMOVED_DELETE;
+        $may_remove_existing = !$always_delete || $entity->access('delete');
+

I think this might not be right.

There's a double negative which means that with this patch you have access to remove the reference when the widget is set to ::REMOVED_DELETE OR the user has access to delete the entity.

This should be: If the widget is setup to possibly 'unlink' without deletion or the user has access to delete the referenced entity.

$may_remove_existing = $settings['removed_reference'] !== self::REMOVED_DELETE || $entity->access('delete');

I cannot seem to get an interdiff of the two patches, but the uploaded patch is only to update the issue mentioned above in #87

I guess this should have test coverage as well.

Many thanks @nuez for pointing out this problem!
I added more tests to prove this problem and extracted it into a separate patch that should fail.
The second patch contains the solution from #88 with the tests.

#89 works good for me.

Only one issue I would like to rise - there can be lots of orphaned entities in the system if we decide to keep referenced entity when the reference is removed. Should we mention it in README and might be suggest some solution to remove orphaned entities of specific types?

Thank you for your review @sinn.

Answering your comment, those entities are not necessarily orphaned. They are referenced with a revision to the entity. So let's say you have an event node that references a venue entity and you remove only the reference but keep the venue, the venue entity is still not orphaned, because if you want to revert your event node, the venue reference with it's revision should be there. Otherwise it would be a data lost and you would not be able to revert your event having the venue back in it.
But once you delete the event node itself, then you could use the module "composite_reference" to ensure, the referenced entities are deleted when the event is deleted, but this is not in the scope of this module.

Yes, I agree that it is out of scope of this module but I concern that this solution can have side effects and would be great to outline ways to resolve them.

I've tested "composite_reference" module and it doesn't remove entities that are linked to parent entity by revision when parent entity is removed. So orphaned entities will appear.

Might be we should create enhancement ticket in the "composite_reference" module to handle this case...

It has been created https://www.drupal.org/project/composite_reference/issues/3188675.

I've read through this issue and patch and reviewed it locally. I'm inclined to +1 RTBC it as is and commit it for the sake of tagging a full release of this module. However, I want to be sure there isn't an issue outlining what remains to be done before other maintainers considered this module ready for a full release. (The reality is with 80k+ installations and reasonable stability, it's ready for a full release and security team coverage.)

However, I want to raise two usability points in the current interface text:

1. When "Delete always" is selected, it seems reasonable for the interface text in the confirmation form to read, "Removing the reference to this node will also delete it from the site." Can't think of a good reason not to make that abundantly clear to the user, but it also would require a conditional to ensure we don't show that message for new inline created entities.
2. When "Let the user decide" is selected, the checkbox that appears to prompt them whether or not the entity should be deleted might be better phrased as, "Also delete this node from the site." This has two changes: adding "also" so it's clear the checkbox is for an additional action and using "site" instead of "system".

I don't think this represents a change in behavior but would love some confirmation: doesn't it seem weird to anyone else that you can delete an entity from the site when you remove its reference via an IEF widget even if it's referenced by other pieces of content? Is that a change in behavior? If not, perhaps we should at least indicate, "Hey, this is referenced by X other entities."

Finally, the general ambiguity of "Remove" seems to be exacerbated by this optional deletion. We have space in the interface ... why not actually use the language of "Remove the reference to..." For example, I created a book node type with a reference / IEF to an author node type, creating The Hobbit and J.R.R. Tolkien as sample content. When I edit the book and click to remove the reference to Tolkien, why shouldn't the confirmation form read, "Are you sure you want to remove the reference to J.R.R. Tolkien?" instead of "Are you sure you want to remove J.R.R. Tolkien?" Makes a lot more sense.

Again, I'm happy for all of the above points to become follow-up issues to this one, as landing the feature is more important than fine-tuning the UX ... but these would all seem like good things to change. 🙂

Hey @rszrama, thanks for taking a look at this.
Definitely, I agree with your UX proposals as this is complicate enough, except that I would be careful using specifics like "node" instead of "entity" in those texts as they may vary case by case. I think it would make sense to take these changes into a well written follow-up issue and address those points separately from this new feature as this issue thread is quite long and complex already I think.

doesn't it seem weird to anyone else that you can delete an entity from the site when you remove its reference via an IEF widget even if it's referenced by other pieces of content? Is that a change in behavior?

Actually, IEF module by default deletes reused entities upon removal without this patch. So I think there is no change in behavior. On the other hand the fact that the created entities through IEF can be reused implies your proposal to notify the user about the re-usability by mentioning the number of referencing entities during removal confirmation. But this has no strict connection with this patch as it is IEF behavior general. This patch opens the door to keep those reused entities on the site if the user wishes to do so.

@rszrama Thanks a lot for that thorough writeup. Your scepsis on UX made me take some time thinking deeper about this. So let me think a bit aloud.

Every new feature brings us a maintenance burden. And some of the features are not my itch, but i see no better place for them than the IEF widget. Not so with this. All the dependent-entity stuff is a big itch of mine, but (TLDR) i doubt this feature should go into the widget.

So we have a growing number of dependent entities, and the different approaches taken are a fruitful source of insight.
We have paragraphs, that are always dependent entitites, in the sense that they behave as a fieldable field, and never exist on their own (which is why they use revision references).
We have blocks, that have a "reusable" field so they know if they are dependent in this sense.
Media currently are treated independent, but some people have itches about this, and maybe we adopt the blocks pattern to this.

Probably the core point is "let's not encourage or support inconsistent data models" (and yes i confess: i also did some errm pragmatic ones in tightly budgeted projects...).
Which means to me:
- Every entity is either dependent or independent. WRT its data model AND mirrored in the UI.
- If dependent, we need ERR for sane revisionability.
- An independent entity should not be deleted in the referencing widget.
- A dependent entity should not be deleted in the referencing widget, as this should always and automatically be done by a garbage collector
- (we may easily craft a "reusable" field that does that GC)

Or maybe stated more concise, i dislike (apart from the maintenance burden) that this feature mixes the data model into widget settings. Which means:
- If the referenced entity is independent, nothing needs to be deleted.
- If the referenced entity is dependent, deleting an unreferenced entity must be automatic and not depend on any widget settings

(And: If IEF already behaves bad in above sense, i suppose for the original product/variant use case, let's discuss cleaning that up, too. The same for other existing code.)

So the only remaining "sane" use case is managing a Library of independent entities (think: Media library).
While i have bad experiences ux-wise with mixing library selection with library management (adding & deleting), there may be valid use cases for this.

And here also, we never must delete entities referenced by others. If this is possible, we should fix it (e.g. by adding a "(deprecated:) allow users to delete entities referenced by others" setting). And imho we must do it in this patch, as we're giving the deletion functionality a much bigger advertisement area.

Thoughts?

To avoid unnecessary SQL queries, countReferenceableEntities() should only be called if the result is actually needed.

@geek-merlin #98

I think the broad direction of what you say makes sense; the widget should not enforce the data model. Therefore always deleting the referenced entity on removal cannot be IEF's job.

However:

If the referenced entity is independent, nothing needs to be deleted.

I think this is not completely true. There are circumstances where it is convenient to allow some users to manage the referenced entities lifecycle mostly through the widget even though it is not strictly a dependent entity. In these cases the setting to allow users to optionally delete on removal makes sense. It can save editors a lot of clicks.

@jonathanshaw Thanks a lot for making explicit this use case.
Given these two sides, i want at least one other maintainer buy-in on this before we put this in.

@prudloff Thanks for the improvement, but tests are not running because your patch does not apply anymore. I have left a small review of the interdiff.

1. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -319,6 +316,16 @@
   +
   

   This empty line is not necessary.

2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -319,6 +316,16 @@
   +      $have_multiple_existing_entities = false;
   

   False should be all capital here.

I will reroll the patch with your improvement in it and I will address my comments.

I have re-rolled the patch to work with the latest changes over dev branch.
I have addressed the comments I have left on the previous submission, #98.
I did not include interdiff as it would be meaningless after the re-roll.
The only difference between the previously working and reviewed patch in #89 and this one is the improvement from comment #98 has been implemented.

I have re-rolled the patch over the latest dev, so it applies on the RC9 release.

Re-rolled the patch to make sure it applies to the latest release.

Works well

catarinavclemente rerolled and updated patch for to D9 core 9.4.x.

Patch 106 gets applied with no error by composer, but leaves some errors in InlineEntityFormComplex.php on D9.4.5 and IEF 8.x-1.0-rc12.
It leaves a "+" sign on line 242 (see screenshot)
And also it seems there is a missing "+" sign line 314 before $selection_settings (see screenshot)

Patch #104 works well with latest IEF.

Very useful patch.

Thank you!

Patch rerolled with fixes from #108

Sorry, I don't understand why did we need a re-roll of the patch if the latest patch #104 is based on the latest dev branch and works perfectly? @tostinni and @catarinavclemente
If these patch re-rolls target specific releases, please properly indicate that intention in the post and the uploaded patch file. This issue is already big enough, and managing the patch files is getting messy. Also, an interdiff would be nice in these cases at least.

Review of patch #112:

+++ b/config/schema/inline_entity_form.schema.yml
@@ -72,3 +75,6 @@ field.widget.settings.inline_entity_form_complex:
+      label: "Keep or delete unreferenced items"  ¶

End of line spaces should be removed.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -145,6 +161,20 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+        $description = $this->t('Select whether a @child should be deleted altogether if removed as a reference here.<br />

Wrong indentation.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -247,6 +303,31 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    ¶

New line with spaces.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -832,6 +927,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    ¶

New line with spaces.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -842,9 +938,11 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    ¶

New line with spaces.

+++ b/tests/src/FunctionalJavascript/ComplexWidgetTest.php
@@ -660,6 +661,156 @@ class ComplexWidgetTest extends InlineEntityFormTestBase {
+    /**

Wrong indentation.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -247,6 +303,31 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
     $entities_count = count($entities);

This line was removed in patch #104 since it's double defined.

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -247,6 +303,31 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+    $entities_count = count($entities);

This variable is also double defined and was addressed in #104.

I see that now comparing #104 and #112 based on #107 that the intentions were to fix certain phpcs issues like line indentations that I fail to find in #104 patch, the interdiff submitted under #107 tells it all. Beside that certain changes were left out. I ran drupal phpcs over the whole module and it all passed without a problem. I don't see any point to re-roll patch #104.

Reuploading the patch as well beside the merge request made from the issue fork to the upstream.

Thanks for sorting this Joe

@joevagyok your patch in #104 was 2 monthes old and @catarinavclemente updated it for 9.4.x but with 2 PHP mentioned in #108, that's why I uploaded an updated patchin order to use this functionality in 9.4 branch.

I only made a quick functional review that this patch was fixing the issue for me and then uploaded it.

Thanks for the code review.

#107 patch had problems and #112 brought them along, that's not good. We are using #104 on production sites without any issues.
@tostinni as I understand you have been experiencing some kind of problem with 9.4.x. Could you point out and describe those issues please, because I haven't seen that in the issue queue.
To make things smooth, you can make a new branch on the issue fork and open a merge request against branch: issue-2875716, or post an interdiff, so we can review the changes made to the patch that was marked as RTBC.

Tested patch with Drupal core 10.0.2 version with IEF. The patch was applied successfully and the issue is fixed as expected.

After patch applies:

++RTBC

Please everyone: Bring in more order than chaos!
If a patch is improved, explain what you do and why.
If a patch does not apply in the first place, question if it made sense in the first place, instead of continuing the lore.
Adjusted credits.

So it looks like the chaos started in #107, and we have #104, which is identical to #119 and !60.
@joevagyok, is this correct?

Also @joevagyok you say this is in production, i suppose in the EU context, correct?

We have some test coverage that i did not scrutinize, but looks well done.
Fed !60 to the testbot.

@geek-merlin, that is correct! The #119 is basically identical to #104 and the !60 merge request is in sync with the patches.

Yes, we have over 100 sites using the patch mentioned above in production environment on the European Commission platform with the D9.4.11 version.

PS.: @geek-merlin I tried to get in touch with you on slack in hope that we can push this issue forward since you have become a maintainer on this project.

So thanks for pushing this forward and helping with order over chaos ;-). And of course for bearing with my initial scepsis.

Merged!