Re-order + remove broken with the Entity Reference (and File) widget

I can confirm as of today this is still reproducible with latest -dev of all modules involved.

I stumbled across this issue and can confirm, that this is also a bug on the 8.x-2.x branch.

I can confirm this is still an issue on 8.x-2.1.

The issue is here:
if ($item == $id && $index == $row_id) {

this condition is never met after reorder, because the $row_id isn't updated, but the entity id is.

The row_id is an attribute on a dom element and isn't passed in the form data.

If there were no duplicate entity ids, this would work:
if ($item == $id) {
But unfortunately, if someone selects the same entity three times and reorders them and tries to delete the 3rd one, it would give unexpected results.

I think the proper solution is to add a weight form element.

I believe this is fixed in latest patch here: #2973457: Add option for field widget to display as table. Closing this issue in favor of that one. If you can help review that issue, please do. I don't think we can fix this properly without adding weight form field to each item, and that's a larger change.

Reopening as it may take a very long time before #2973457: Add option for field widget to display as table is merged in. I'll see if I can fix this separately.

I've been working on this bug. It seems there is a mix of using the data-row-id attribute to find the correct element to remove and using the 'target_id' element, and after sorting if you press remove, the data-row-id is wrong. Sometimes, it can't even find the correct triggering element: #3046416: Remove button conflict wrong triggering element

I was working for a while to add a 'weight' property, like the file widget has, but it's very tricky with the behavior changing depending on the context, and the entity browser returns data to the 'target_id' element. So there is a lot of code switching it back and forth and detecting when to use the current items' weight fields and when to use the 'target_id'. I think I've hit on a good solution, which avoids the problem, which is to always use the 'target_id' element, and remove the ajax update when pressing the remove button. The remove button can just update the 'target_id' element. Then there is no problem with the current items changing on a rebuild, which is causing this bug: #3046416: Remove button conflict wrong triggering element.

https://www.drupal.org/files/issues/2019-04-25/3040309-38.patch

After lots of trial and error and testing, this allows simplifying the logic in the field widget and removing a lot of the alternate ways of retrieving the data, which is where some of the bugs have come from.

We can drop this:

-    // We store current entity IDs as we might need them in future requests. If
-    // some other part of the form triggers an AJAX request with
-    // #limit_validation_errors we won't have access to the value of the
-    // target_id element and won't be able to build the form as a result of
-    // that. This will cause missing submit (Remove, Edit, ...) elements, which
-    // might result in unpredictable results.
-    $form_state->set(['entity_browser_widget', $this->getFormStateKey($items)], $ids);

and this:

-    elseif ($form_state->has([
-      'entity_browser_widget',
-      $this->getFormStateKey($items),
-    ])
-    ) {
-      $stored_ids = $form_state->get([
-        'entity_browser_widget',
-        $this->getFormStateKey($items),
-      ]);
-      $indexed_entities = $entity_storage->loadMultiple($stored_ids);
-
-      // Selection can contain same entity multiple times. Since loadMultiple()
-      // returns unique list of entities, it's necessary to recreate list of
-      // entities in order to preserve selection of duplicated entities.
-      foreach ($stored_ids as $entity_id) {
-        if (isset($indexed_entities[$entity_id])) {
-          $entities[] = $indexed_entities[$entity_id];
-        }
+        $items->setValue($value);
+        $entities = $items->referencedEntities();

Also, this code is unnecessary:

-    else {
-      foreach ($items as $item) {
-        if (isset($item->target_id)) {
-          $entity = $entity_storage->load($item->target_id);
-          if (!empty($entity)) {
-            $entities[] = $entity;
-          }
-        }
-      }
-      return $entities;
-    }

As it can be simplified to:

+    return $items->referencedEntities();

I decided to drop the buggy getEntitiesByTargetId

-  /**
-   * Get selected elements from target_id element on form.
-   *
-   * @param array $element
-   *   The form element.
-   * @param \Drupal\Core\Form\FormStateInterface $form_state
-   *   The form state.
-   *
-   * @return \Drupal\Core\Entity\EntityInterface[]|false
-   *   Return list of entities if they are available or false.
-   */
-  protected function getEntitiesByTargetId(array $element, FormStateInterface $form_state) {
-    $target_id_element_path = array_merge(
-      $element['#field_parents'],
-      [$this->fieldDefinition->getName(), 'target_id']
-    );
-
-    $user_input = $form_state->getUserInput();
-
-    $ief_submit = (!empty($user_input['_triggering_element_name']) && strpos($user_input['_triggering_element_name'], 'ief-edit-submit') === 0);
-
-    if (!$ief_submit || !NestedArray::keyExists($form_state->getUserInput(), $target_id_element_path)) {
-      return FALSE;
-    }
-
-    // TODO Figure out how to avoid using raw user input.
-    $current_user_input = NestedArray::getValue($form_state->getUserInput(), $target_id_element_path);
-    if (!is_array($current_user_input)) {
-      $entities = EntityBrowserElement::processEntityIds($current_user_input);
-      return $entities;
-    }
-
-    return FALSE;
-  }
-

and I replaced it with this code:

     // Find IDs from target_id element (it stores selected entities in form).
     // This was added to help solve a really edge casey bug in IEF.
-    if (($target_id_entities = $this->getEntitiesByTargetId($element, $form_state)) !== FALSE) {
-      return $target_id_entities;
+    if (empty($form_state->getValues()) && $form_state->isMethodType('POST')) {
+      $input_value = NestedArray::getValue($form_state->getUserInput(), array_merge($element['#field_parents'], [$this->fieldDefinition->getName()]));
+      if (!empty($input_value) && isset($input_value['target_id'])) {
+        $data = empty($input_value['target_id']) ? [] : explode(' ', trim($input_value['target_id']));
+        $values = [];
+        foreach ($data as $data_item) {
+          $values[]['target_id'] = explode(':', $data_item)[1];
+        }
+        $items->setValue($values);
+        return $items->referencedEntities();
+      }

I think this might need a bit of an update, since I dropped the NestedArray::keyExists. I might just put the function back and update it.

By eliminating the ajax submit of the remove button, we can remove this:

-  /**
-   * {@inheritdoc}
-   */
-  public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, FormStateInterface $form_state) {
-    if (($trigger = $form_state->getTriggeringElement())) {
-      // Can be triggered by "Remove" button.
-      if (end($trigger['#parents']) === 'remove_button') {
-        return FALSE;
-      }
-    }
-    return parent::errorElement($element, $violation, $form, $form_state);
-  }
-

And a few other lines that refer to the remove button. I leave the remove button submit handler there and deprecate it, also, it's still being used by the replace button, so I should probably remove the deprecation.

I noticed that the 'target_id' element has an ajax submit handler, but didn't have a '#submit' method, so I added it:

+        '#submit' => [[get_class($this), 'entityBrowserValueUpdated']],
+        '#limit_validation_errors' => [array_merge($element['#field_parents'], [$this->fieldDefinition->getName()])],
+        '#executes_submit_callback' => TRUE,

And a callback:

+  /**
+   * Submit callback.
+   */
+  public static function entityBrowserValueUpdated(&$form, FormStateInterface $form_state) {
+    $triggering_element = $form_state->getTriggeringElement();
+    $parents = array_slice($triggering_element['#parents'], 0, -1);
+    $array_parents = array_slice($triggering_element['#array_parents'], 0, -1);
+
+    $values = $form_state->getValue($parents);
+
+    $entities = empty($values['target_id']) ? [] : explode(' ', trim($values['target_id']));
+    $values = [];
+    foreach ($entities as $entity) {
+      $values[]['target_id'] = explode(':', $entity)[1];
+    }
+
+    // Set new value for this widget in the form_state.
+    $element = &NestedArray::getValue($form, $array_parents);
+    $form_state->setValueForElement($element, $values);
+
+    // Rebuild form.
+    $form_state->setRebuild();
+  }
+

I believe this helped to remove the stored ids code:

-      $stored_ids = $form_state->get([
-        'entity_browser_widget',
-        $this->getFormStateKey($items),
-      ]);

I need to go back and do some testing to see when this callback is called, so I can explain it better.

Here's the new javascript code that handles the item removal on the front-end:

+
+      $(context).find('.field--widget-entity-browser-entity-reference .remove-button').each(function () {
+        $(this).once('entity-browser-remove').on('mousedown', function(e) {
+          var $currentItems = $(this).parent().parent();
+          $(this).parent().remove();
+          Drupal.entityBrowserEntityReference.updateTargetId($currentItems);
+        })
+      });
+
+      $(context).find('.field--widget-entity-browser-file .remove-button').each(function () {
+        $(this).once('entity-browser-remove').on('mousedown', function(e) {
+          var $currentItems = $(this).closest('.entities-list');
+          $(this).closest('tr').remove();
+          Drupal.entityBrowserEntityReference.updateTargetId($currentItems);
+        })
+      });
+

And, I moved code that changes 'target_id' on the front end out of Drupal.entityBrowserEntityReference.entitiesReordered and into a new function, Drupal.entityBrowserEntityReference.updateTargetId, so that the remove buttons can call it too.

@@ -48,13 +65,24 @@
    *   Object with detailed information about the sort event.
    */
   Drupal.entityBrowserEntityReference.entitiesReordered = function (event, ui) {
-    var items = $(this).find('.item-container');
+    Drupal.entityBrowserEntityReference.updateTargetId($(this));
+  };
+
+  /**
+   * Updates the 'target_id' element.
+   *
+   * @param {object} $currentItems
+   *   Object with '.entities-list.sortable' element.
+   */
+  Drupal.entityBrowserEntityReference.updateTargetId = function ($currentItems) {
+    var items = $currentItems.find('.item-container');
     var ids = [];
     for (var i = 0; i < items.length; i++) {
       ids[i] = $(items[i]).attr('data-entity-id');
+      // If using weight field, update it.
+      $(items[i]).find('input[name*="[_weight]"]').val(i);
     }
-
-    $(this).parent().parent().find('input[type*=hidden][name*="[target_id]"]').val(ids.join(' '));
-  };
+    $currentItems.parent().find('input[type*=hidden][name*="[target_id]"]').first().val(ids.join(' '));
+  }
 
 }(jQuery, Drupal));

I don't think this is necessary any more, as I dropped the weight and target_id in each .item-container, that I had been working on:

-    $(this).parent().parent().find('input[type*=hidden][name*="[target_id]"]').val(ids.join(' '));
-  };
+    $currentItems.parent().find('input[type*=hidden][name*="[target_id]"]').first().val(ids.join(' '));

So I'll undo that change.

I added this so that as the items are re-ordered, the file widget will update the weight of each item. Otherwise they got out of sync. This is code I was using in the field widget when I was testing adding a weight field, but I was using 'weight' instead of '_weight'. I changed it be consistent with the existing naming convention in the file widget.

+      // If using weight field, update it.
+      $(items[i]).find('input[name*="[_weight]"]').val(i);

Here's patch that updates the entity browser widget to simplify the logic of how it updates after ajax buttons and submit buttons are pressed. Most importantly, it removes the ajax submit for the remove button and replaces it with a javascript method that updates the 'target_id' element.
This circumvents the issues around the remove button triggering element disappearing when the form rebuilds.

This patch depends on #3046416: Remove button conflict wrong triggering element, therefore I'm testing a combined patch.

Oops. The combined patch was incorrectly created. Here's an update.

Here's a video of the new behavior. The noticeable differences are:

• No ajax spinner
• it actually works as expected

https://www.drupal.org/files/issues/2019-04-26/sort-plus-delete-update.mov

Marking needs work, as I noticed a few things, see my notes above in comment #9.

Ignore this one.

The same as #11, except I undid this change:

-    $(this).parent().parent().find('input[type*=hidden][name*="[target_id]"]').val(ids.join(' '));
-  };
+    $currentItems.parent().find('input[type*=hidden][name*="[target_id]"]').first().val(ids.join(' '));
+  }

Here's test coverage. I tried to mimic the test case in the issue summary as closely as possible. The patches includes 3046416, so if that gets merged first, this will need a reroll.

Here's an interdiff between #18 and #19. I didn't change the fix at all, just added test coverage.

We still need test coverage for drag and drop, and drag and drop and then remove when using the entity_browser_file field widget, although that could be a follow up.

As I was trying to fix this schema issue that was causing #19 to fail, I noticed that the EntityBrowserWebDriverTestBase is already setting up the article content type very similarly to the new content type I was creating. So I updated the test to use that one, which means less new configs, and should fix the error in #19. The patches includes 3046416, so if that gets merged first, this will need a reroll. Or we can just merge this one.

Adds coverage for entity_browser_file field widget.

While working on this, I noticed another bug:

#3051318: keying on entity id in entity_browser_file causes item not to display.

I added a fix in this patch, by changing the entity_browser_file current items to key on delta instead of entity id. This is something I've changed back and forth a few times. While I think delta is better and matches the entity_browser_entity_reference field widget, I didn't want to make unnecessary changes. But now, I think, it's necessary to prevent this newly discovered bug.

oh yes, I forgot to update delta in ImageFieldTest from entity-based to delta-based. I had already updated this in another test and can copy it over.

OK, so this now has test coverage for both widgets, and I think a good solution.

Additionally, this provides test coverage for drag and drop functionality that was largely missing before.

+++ b/tests/src/FunctionalJavascript/EntityBrowserWebDriverTestBase.php
@@ -181,4 +182,72 @@ abstract class EntityBrowserWebDriverTestBase extends WebDriverTestBase {
+	    for (i = 0 ; i < moves ; i++ ) {
+			  centerX += xOffset / moves;
+			  centerY += yOffset / moves;
+		    fireMouseEvent('mousemove', dragElement, Math.round(centerX), Math.round(centerY));
+		  }

Extra spaces should be removed

Fixing spacing, per #33.

Fixing the spacing a bit more. There were some tabs that needed to be replaced by spaces.

RTBC, the solution in #35 fixes the issue.

That was part of my effort to simplify the widget. But now that you point it out, anyone extending the module and still using the remove ajax would still want that, so maybe better to leave it in and mark it a deprecated.

Oh, and thank you for testing!

Actually, with the limit validation errors, that should be unnecessary, so leaving that change in.

Committed, thanks.

Giving credit to kellyimagined for helping to test.

I reverted this change.

See #3053126: Entity reference widget remove button's new behavior uses too strict selectors

@hctom found a bug that when you add a single item and remove it, the Select Entities button no longer appeared, so you were no longer able to select a different item.

Use a single value field with an entity browser entity reference widget. Add one item and click the "Remove" button afterwards. From that point on, you won't be able to fill the with a new item, because you do not have the button to open the entity browser again (which is removed by the Ajax callback when selecting an item via entity browser).

I set up a simplytest.me site with that: https://d07n8.ply.st/node/add/article (User: admin / Pass: admin). Just add a new article and fill the "Entity browser test" field and you'll see what I mean.

So the no ajax approach needs to be dropped or reworked.

The tests were failing after reverting the change. I tracked it down to one additional change that needed to be reverted.

diff --git a/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php b/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
index c3124f0..79f6284 100644
--- a/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
+++ b/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
@@ -163,6 +163,7 @@ class EntityReferenceWidgetTest extends EntityBrowserWebDriverTestBase {
 
     // Test the "Remove" button on the widget works.
     $this->assertSession()->buttonExists('Remove')->press();
+    $this->waitForAjaxToFinish();
     $this->assertSession()->pageTextNotContains('Alpaca');
 
     // Test the "Replace" button functionality.

Here's an updated patch.

It adds a fix and test coverage for the bug found by @hctom in the original patch. If the number of selected items is equal to the cardinality and the selection mode isn't edit mode, the entity browser element is hidden. So we need a way to bring back the 'Select entities' link or button when an item is removed. I still think the non-ajax method is better, but to restore the entity browser form element, I think we need to do an ajax refresh. So I added code that handles that. If the number of selected items drops below the cardinality and the entity browser element isn't visible, I trigger an ajax refresh to restore the entity browser element, i.e. 'Select entities' link or button.

I added test coverage for two scenarios:

1. That removing an item when cardinality is 1 restores the link.
2. That removing an item when cardinality is 2 and there are two current selections restores the link.

I also incorporate fixes suggested by #3053126: Entity reference widget remove button's new behavior uses too strict selectors. The biggest one, and one that could break BC for people extending the widget is I'm using a new data attribute to select the entities-list.

diff --git a/js/entity_browser.entity_reference.js b/js/entity_browser.entity_reference.js
index c1f7f67..836bf5f 100644
--- a/js/entity_browser.entity_reference.js
+++ b/js/entity_browser.entity_reference.js
@@ -20,18 +20,10 @@
         });
       });
 
-      $(context).find('.field--widget-entity-browser-entity-reference .remove-button').each(function () {
+      $(context).find('[data-entity-browser-entities-list] .remove-button').each(function () {

I could possibly drop this change, but I don't like using the field name in the selector, because we're using this for EntityReferenceBrowserWidget, FileBrowserWidget and when it's ready, the TableWidget (#2973457: Add option for field widget to display as table). Although this might break the remove button for developers who extend these widgets (it would if they replace the entities list element), I think '.entities-list .remove-button' is not namespaced well, and I think this is a good change to make.
If a developer replaced the entities list in a subclass, they probably replaced the remove button too, so they would have an ajax remove button.

If you feel differently, and that we should use '.entities-list .remove-button', let me know, or if you have a better idea.
The change in question is just to add a data attribute to the .entities-list wrapper:

@@ -585,7 +605,10 @@ class EntityReferenceBrowserWidget extends WidgetBase implements ContainerFactor
 
     return [
       '#theme_wrappers' => ['container'],
-      '#attributes' => ['class' => $classes],
+      '#attributes' => [
+        'class' => $classes,
+        'data-entity-browser-entities-list' => 1,
+      ],
       'items' => array_map(
         function (ContentEntityInterface $entity, $row_id) use ($field_widget_display, $details_id, $field_parents, $replace_button_access) {
           $display = $field_widget_display->view($entity);

The logic added to fix the bug is to trigger an 'entity_browser_value_updated' event if the number of selections dips below the cardinality:

diff --git a/js/entity_browser.entity_reference.js b/js/entity_browser.entity_reference.js
index c1f7f67..836bf5f 100644
--- a/js/entity_browser.entity_reference.js
+++ b/js/entity_browser.entity_reference.js
@@ -82,7 +74,14 @@
       // If using weight field, update it.
       $(items[i]).find('input[name*="[_weight]"]').val(i);
     }
-    $currentItems.parent().find('input[type*=hidden][name*="[target_id]"]').val(ids.join(' '));
+    var $target_id_element = $currentItems.parent().find('input[type*=hidden][name*="[target_id]"]');
+    $target_id_element.val(ids.join(' '));
+
+    // Trigger ajax submission to restore entity browser form element.
+    var cardinality = parseInt($target_id_element.attr('data-cardinality'));
+    if (ids.length < cardinality && $target_id_element.attr('data-entity-browser-visible') === "0") {
+      $target_id_element.trigger('entity_browser_value_updated');
+    }

Minor coding standard fixes.

User hctom tested #52, see https://www.drupal.org/project/entity_browser/issues/3053126#comment-131....

That one looks really good! Re-ordering and removing/replacing selected entities works like a charm. It works for multi-value fields and for single-value fields.

#52 doesn't apply anymore, here's a re-roll.

I glanced over the patch, but realized that it touches parts of the code I'm not very familiar with, and I recognize I don't fully understand all modifications going on here... :(

I tested it however, both manually and triggered our suite of automated tests (in our current custom project) with the patch applied. We aren't really doing any fancy re-ordering in the entity-reference widget, but we do have several tests interacting with the widget, so at least it's good to hear that all green here.

Nice work!

Thank you for testing! I hope that after this issue is resolved, the field widgets will be much simpler.

Reroll against current head 52b48b1571d6f8924b5c8bafebe9882ee10f782e.

I updated #60 to make enitiy_browser work with inline_entity_form again, now checking for values and inputs. The code should now have minimal side effects.

Needs a reroll @volkerk.

Re-rolled #61 (applies to 8.x-2.2).

Tested this, it also fixes the File entity browser widget with the prepend mode. However, it also breaks the file entity browser widget visually pretty badly.

The patch adds the item-container to the tr, and as a result, the CSS appies and makes this a display: inline-block, which looks then completely broken.

I was able to fix it by changing the CSS like this:

diff --git a/css/entity_browser.entity_list.css b/css/entity_browser.entity_list.css
index 94fb4df..f2359a7 100644
--- a/css/entity_browser.entity_list.css
+++ b/css/entity_browser.entity_list.css
@@ -8,19 +8,19 @@
  * These styles are compatible with Bartik, Seven, and Classy themes.
  */
 
-.entities-list .item-container {
+.entities-list div.item-container {
   display: inline-block;
   margin: 10px 10px 15px;
   vertical-align: text-bottom;
 }
 
-.entities-list .item-container > img,
-.entities-list .item-container > input {
+.entities-list div.item-container > img,
+.entities-list div.item-container > input {
   display: block;
   margin: 5px auto 0;
 }
 
-.entities-list.sortable .item-container:hover {
+.entities-list.sortable div.item-container:hover {
   cursor: move;
   opacity: 0.6;
 }

However, custom CSS might be affected too and I think it might be better to add a new class to both widgets that does not yet exist, possibly with a js- prefix? Or a data attribute? See https://www.drupal.org/project/drupal/issues/2431671, don't really know anything about this stuff :)

I have also tested this patch in Thunder. And it works great!

Setting to needs work for #64.

Unfortunately I found a regression regarding the new remove button handling.

* Create node type with a entity reference media field, set field required.
* Use entity browser widget, enable "Rendered entity" entity display plugin, enable "Display Remove button"
* Create a new node, select an image using entity browser.
* Click remove button on image.

There are two error messages, it is not possible to remove the image.

The remove button mechanism was switched from having its own ajax submit callback to removing the value via javascript and using the hidden target_id element to submit.
Unfortunately, this means form validation will try to validate a form without existing target_id value, which fails and therefore submit callback is not executed.
The ajax callback then returns the unaltered form, where the last reference is still shown in current but not target_id value is set.

This problem is reproduceable on required fields and fields using the inline_entity_form entity display plugin.
While using the stand-alone ajax submit button, the value was removed on server side after validation.

I reverted the remove button mechanism to its previous behaviour.

retagging

The patch here is provided with the distribution thunder and the widget deletes all entities in a reference field. See #3099950: Entity Browser deletes all entities instead of one for some animations showing the problem. So I tested on a blank drupal with the patch 2851580-67-stable and it's deleting all selected entities when deleting one.

None of the patches apply right now, so it needs:

• a reroll, (it looks like we need to reroll #63 perhaps, since #67 is having issues in the thunder theme.)
• we need to address the regression in #67
• we need to address the css bug in #64

I'm thinking we might want to break this up too, since it's a large patch.

Added reroll.

The issue I am having is not being able to delete items from a list of items. When I click on remove button for the first time, it works, but in the second time it roll back the deleted item.

I was able to solve the problem by applying the patch in comment #6 provided in https://www.drupal.org/project/entity_browser/issues/3068327.

Still have a problem after re-ordering and trying to remove an item.

Another reroll.

Another reroll, now directly from #63

It's was the same file ...

Fixing the test.

I have no idea on how to fix the test. On my local installation, the test does not fail. Maybe this is due to different versions of chromedriver on testbot.
@berdir: regarding #64, I am not sure how to reproduce this. Maybe I am looking at the wrong thing, but file entity browser widget seems to look fine for me.

After some lesser headaches, I was able to get drupalci run locally, this new patch does not fail there as well. Let's see, what the official tests are thinking about it.

#83 worked for me to solve a different issue that occurs when updating an image on a media image field inside layout builder. Basically you couldn't Select an image or Upload a new one after hitting the Remove button and then hitting the Select Media button.

I am trying to reroll this. It's not easy. Let's see what the testbot thinks...

Hopefully, this will fix the tests.

I just ran into this issue on a client site, based upon thunder, which uses patch #88. For that site the removal of items broken when updating to thunder 3 and this patch.

The site has extended the entity browser and customized this logic, so it's even another layer more complex. On that site, for some reason the jquery behavior is not fired, so it's submitting via a regular drupal ajax. I must say, I think this patch seems unnecessarily complex:
- why have another drupal behaviour that just triggers another drupal ajax once again? seems an unnecessary complication layer when just directly ajax can be triggered
- generally, that \Drupal\entity_browser\Plugin\Field\FieldWidget\EntityReferenceBrowserWidget::formElementEntities $is_releveant_submit detection seems wrong. It's the job of the submit callback to do changes and to handle button specific logic.
- The button specific logic should update form values as suiting for the button logic, e.g. re-order or remove items. Then in formElementEntities() all we need to do is check whether form values (from a form rebuild) are available and use them.

I got my complex use-case working with a custom #ajax button handler and a bit off additional logic in formElementEntities() to apply form values. I'm attaching that additional hunk here. I think with a hunk like that the whole $is_relevant_submit can and should be dropped.

Unrelated, the following hunk of formElementEntities also seems useless, is that really needed?

    if (!$value_exists && $input_exists && !is_array($input_value['target_id'])) {
      $data = empty($input_value['target_id']) ? [] : explode(' ', trim($input_value['target_id']));
      $values = [];
      foreach ($data as $data_item) {
        $values[]['target_id'] = explode(':', $data_item)[1];
      }
      $items->setValue($values);
      return $items->referencedEntities();
    }

Going with the pre-existing $items should be jsut fine if no form values are provided imo.

This patch solved another issue for me:
- a node references a media, using IEF and IEF preview widget from IEF preview module
- the media has a file field, using File EB widget with a custom EB widget to select files from S3 and create file entities
- creating a media and selecting a file worked fine
- but if (before saving the node) I try to click "Edit" to edit the newly created inline media, the EB File widget says "No files yet"

Debugging I found that:
- when I pressed the "Create" button in the IEF to finish creating the new media, this triggered a rebuild of the EB element but with no $items
- this resulted in an empty array of entity ids getting stored into the uuid:field form state key
- when I went to edit the inlne entity, that empty array overrode the actual $items when getting the form element entities.

Patch #88 works for me to resolve https://www.drupal.org/project/entity_browser/issues/3068327.

Solution #88 doesn't work if EntityReferenceBrowserWidget is used in the required field that is shown inside Paragraph Classic or Paragraph Experimental widgets - when I want to remove item validation error `@name field is required.` is shown. But it works well inside Inline Entity Forms widgets.

Schema:
Node -> Paragraph (Paragraph Classic or Paragraph Experimental widgets) -> Entity reference field, required (EntityReferenceBrowserWidget) - doesn't work, validation error.

Node -> Paragraph (Inline entity form - Complex, Inline entity form - Simple widgets) -> Entity reference field, required (EntityReferenceBrowserWidget) - works.

Possible solution is attached.

Tested #92 and it fixes the original issue and the problem with the required field in paragraphs that was discovered in #88. Thanks @sinn!

I have applied patch #92 , but it introduces another issue with layout builder that is if we add a block with entity browser media field and add media item then save, if we edit the block and try to remove media item, the ajax on remove button does not work.

Steps to reproduce:

• Apply this patch #92
• Create a block type with entity browser media field
• Go to layout builder and add the above block and add a media via entity browser and save
• Now edit the block and try to remove the media item.

It seems like in the patch the #ajax for removed button is removed,

-              '#ajax' => [
-                'callback' => [get_class($this), 'updateWidgetCallback'],
-                'wrapper' => $details_id,
-              ],

The patch in #92 also resolves https://www.drupal.org/project/entity_browser/issues/2685029.

I also can't reproduce the issue described in #94. The entity browser remove button on a layout builder custom block with media reference works fine for me with this patch applied.

Can anyone else confirm this?

We are experiencing the same issue as mentioned in #94. The ajax on the remove button is not working anymore after applying the patch.

Update
In our case the issue seems to occur in Safari but not in Chrome.

Update 2
It apparently was a very local issue (i.e. only in one specific browser window). Removing some stuff from the browser cache solved the issue. Confirmed the patch is working both in Chrome and Safari now.

I ran into the exact same issue as @ricovandevin but the patch eventually worked to resolve #2685029: Ajax error when clicking select files within IEF referenced entity. I had to open a completely new session or delete the local/session storage from Dev Tools for the changes in the patch to work. @arshadkhan35 might want to retry.

Patch #92 needs a reroll against 8.x-2.8.

Patch #92 is still applied on 8.x-2.8

ok, then something went wrong on my side, setting back to NR.

I was running into an issue with having a multiple-value Paragraphs with an Entity Browser widget, causing the loss of a newly-selected entity when I got to click the "Add another item" button to add another paragraph item, and this patch seems to resolve it. I wonder if I could pull out a subset of the patch that applies to that issue, because this conflicts with another patch we have applied on our project for the configurable form mode (#2821917: Allow the edit button's form mode to be selected in Entity Browser field widget settings).

Adapted patch #92 with patch from #2821917: Set form mode for edit button

Please, whoever contributes to the issue, maintain order and don't introduce mess.
If you provide a patch, please make sure to provide interdiff with an explanation what was changed, and make sure the patch applies without errors and tests are passing.

#102 does not apply, needs work, plus it extends the scope with the issue: #2821917: Set form mode for edit button. I restore #92 the last stable patch into the list as it is in scope of this issue only.

Attached is a rebase of patch from #92 onto the latest 8.x-2.x, as that batch no longer applied cleanly on the 8.x-2.9 branch.

Actually, disregard that, I think I have some multiple conflicting patches.

Updated the patch in #92 to no longer use jquery once so that it can be used in D1O

$(this).once('entity-browser-remove').on('mousedown', function(e) {
becomes
$(once('entity-browser-remove', $(this))).on('mousedown', function(e) {

The patch is broken, has indentation issues from top to bottom. The CI also fails to apply. Please make sure to check before upload. I kindly refer to the #103 comment here.

Updated the patch, and provided an interdiff

Correctly applied the patch in #108, there was some context moved in between the patch this was derived from and the current HEAD.

Please don't credit me for this trivial change.

I tried applying the patches in #92 and #108 to version 2.9.0 of entity_browser and Drupal 9.4.12. Full disclosure, the field I tested with is also using a field widget from entity_browser_table version 1.4.0.

For both of these patches, editing a node and adding a new entity reference to a field with existing references caused the order of the references to change, which I don't think is expected.

This has some overlap and potential collision with #2973457: Add option for field widget to display as table. It would be best to settle on one solution.

The table layout approach is cleaner.

I was running into something that seemed similar to this where just having a single field was broken in d10. both replace and remove buttons triggered this error:

TypeError: Drupal\Core\Form\FormState::setValueForElement(): Argument #1 ($element) must be of type array, null given, called in /app/web/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php on line 546 in Drupal\Core\Form\FormState->setValueForElement() (line 79 of /app/web/core/lib/Drupal/Core/Form/FormStateValuesTrait.php).

Since this looked similar I applied it and it helped but I noticed a couple our weird things with the replace button.
1) Most of the time it just breaks with the same error.
2) Sometimes it seems to replace the current widget with a different entity reference from the page.

The primary field being tested is a media entity using a media entity browser with a cardinality of 1.

Not sure if this even is the right issue but if it is let me know if there are things I can help test to finalize this.

Patch #108 works for me. Tested in Drupal version 9.5.9 and Entity browser 2.9.0

+++ b/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php
@@ -491,9 +504,6 @@ class EntityReferenceBrowserWidget extends WidgetBase implements ContainerFactor
-    elseif ($trigger['#type'] == 'submit' && strpos($trigger['#name'], '_remove_')) {
-      $parents = array_slice($trigger['#array_parents'], 0, -static::$deleteDepth);
-    }

The removal of this code is causing the "Remove" button to not work properly on my site.

This fixes another similar issue where if you Add a new item from the browser and then try to remove it, it relaunches the browser.

Also much nicer UX as it appears to remove things straight away.

Tested with an overly complex data model of: Node -> Custom entity form with IEF -> Paragraph -> Paragraph -> Entity browser widget multi cardinality field for a custom entity.

Tested adding, removing, reordering, and tried my best to break it but it all worked well.

Figured out the issue I reported in #110.

I'm using the Entity Browser - Table Layout project, which extends the EntityReferenceBrowserWidget.php plugin. The changes from this issue caused breaking changes for the other module's widget.

I created a child issue in that project to provide necessary changes if you apply this patch and are also using that module: #3395042: Re-order + remove broken with the Entity Reference (and Table) widget.

A new release is published and the dev branch is changed.
I re-rolled #109 patch in order to apply to the latest release and the 2.x dev branch.

Can we get a merge request version posted so that we can start running the new GitLab CI testing against this?

Opened a merge request with the patch from #117. However, I am leaving the patch for composer patching.

Please note that, tests are failing on core v9.5 which I am not sure how relevant at this point, since it will not be supported further I believe. I leave that to the maintainers.

And as for the test failures with ajax, see this change record. We also started to have the same failures, and we are asserting on elements appearing on the screen instead of using waitForAjaxToFinish().

Added the latest patch for the MR for composer builds

After using the above patch with Drupal 10 + layout builder, I am getting the following error
RuntimeException: The subform and parent form must contain the #parents property, which must be an array. Try calling this method from a #process callback instead. in Drupal\Core\Form\SubformState->getParents() (line 76 of /Users/maria.mcdowell/Sites/devdesktop/eemmcdowell-test/docroot/core/lib/Drupal/Core/Form/SubformState.php).

I have created a separate issue with all the details "Error when trying to place a new block: RuntimeException: The subform and parent form must contain the #parents property, which must be an array."

Updated patch #123 to fix the above error.

This one worked for us!

D11 tests are failing atm, but 10.4 and previous major tests should be passing.

Was seeing some issues with the Gin admin theme. Worked on in Claro.

When the entity references render as a table, gin wraps the table in 2 div elements. Removing those divs, it all worked again.
Unsure if this is immediately related, but combining this patch plus:

/**
 * Implements hook_theme_suggestions_HOOK_alter().
 */
function MYMODULE_theme_suggestions_table_alter(array &$suggestions, array $variables): void {
  if (!empty($variables['attributes']['data-entity-browser-entities-list'])) {
    $suggestions[] = 'table__simple';
  }
}

Has restored functionality in the Gin admin theme.

Re-roll over latest 8.x-2.x.

Thanx, @joevagyok. Your patch is necessary!

We just encountered this issue on our Drupal 11.2.8 site when we had a combination of a LB SDC + an image field using a custom entity browser.

I applied this patch and it fixed the issue for SDCs using standard fields (e.g. an Image SDC with a single image field that uses entity browser), so that's good.

However, if the SDC uses a complex field type (e.g. Custom Field), there are issues. For example, there is a SDC called "Card Group" and within that SDC there is a Custom Entity Field field type for individual cards. Each card has its own image field. When using the block configuration form in Layout Builder, the image in the first card entry successfully removes and re-adds the image, but the second card on still displays the original issue.

Note that there are no issues with adding an image for the first time to any of the Custom Field nested fields. This is specifically when removing an existing image and trying to re-add a new one.

I'm certain this is just a matter of updating the patch to account for the nested structure of Custom Field field types, but did want to flag it here for others who might be using entity browsers within Custom Fields on LB.

Belay that last comment. Looking back at the Custom Entity module, I see that custom_field_entity_browser uses a modified COPY of the EntityReferenceBrowserWidget, so my issue might fall more on the Custom Entity module having an outdated copy of the EntityBrowserWidget.

Ignoring the Custom Field problems, the patch worked for me :)