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

Created on 10 February 2017, over 7 years ago
Updated 20 March 2024, 3 months ago

Problem/Motivation

Re-ordering an item in the field widget then removing any item appears to be broken with the current dev release of Entity Browser. The behavior is not totally consistent, but I have noticed that clicking remove usually works the second time you try, after a re-order.

Replication steps:

  1. Install the Standard profile and the entity_browser_example module
  2. Visit /node/add/entity_browser_test
  3. Expand the first "Files" fieldset, then upload some files
  4. After uploading, re-order a file and attempt to remove it. If you don't see this the first time, try again with different files and remove buttons. Since there are still conditions where the remove button works, I've uploaded a GIF to illustrate this.

Proposed resolution

Rework remove buttons to update 'target_id' with ajax, and remove the item on the front-end.

Remaining tasks

test coverage

User interface changes

None. Except ajax spinner not visible.

API changes

None.

Data model changes

None.

πŸ› Bug report
Status

Needs review

Version

2.0

Component

Widget plugins

Created by

πŸ‡ΊπŸ‡ΈUnited States samuel.mortenson

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡¦Ukraine anton4uk

    Adapted patch #92 with patch from https://www.drupal.org/project/entity_browser/issues/2821917 ✨ Allow the edit button's form mode to be selected in Entity Browser field widget settings Needs review

  • πŸ‡§πŸ‡ͺBelgium joevagyok

    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 doesn ot apply, needs work plus it extends the scope with the issue: #2821917: Set form mode for edit button. I restore #92 patch into the list as it is in scope of this issue only.

  • πŸ‡ΊπŸ‡ΈUnited States bburg Washington D.C.

    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.

  • πŸ‡ΊπŸ‡ΈUnited States bburg Washington D.C.

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

  • πŸ‡§πŸ‡ͺBelgium svdhout

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

  • πŸ‡§πŸ‡ͺBelgium joevagyok

    @svdhout your patch is broken, has indentation issues. Please check it before uploading.

    +++ b/js/entity_browser.entity_reference.js
    @@ -50,13 +59,31 @@
         /** @var \Symfony\Component\Validator\ConstraintViolation $violation */
         foreach ($violations as $offset => $violation) {
         + $trigger = $form_state->getTriggeringElement();
         + if (!empty($trigger['#ajax']['event']) && $trigger['#ajax']['event'] == 'entity_browser_value_updated') {
         + // Skip validation if it is triggered by hidden target_id.
         + $violations->remove($offset);
         + continue;
         + }
         // The value of the required field is checked through the "not null"
         // constraint, whose message is not very useful. We override it here for
         // better UX.
         @@ -374,30 +386,21 @@ class EntityReferenceBrowserWidget extends WidgetBase implements ContainerFactor
         public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
     
         $entities = $this->formElementEntities($items, $element, $form_state);
         -
         - // Get correct ordered list of entity IDs.
         - $ids = array_map(
         - function (EntityInterface $entity) {
         - return $entity->id();
         - },
         - $entities
    

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

  • πŸ‡§πŸ‡ͺBelgium svdhout

    Updated the patch, and provided an interdiff

  • πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

    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.

  • πŸ‡ΊπŸ‡ΈUnited States weekbeforenext Asheville, NC

    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.

  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

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

    The table layout approach is cleaner.

  • πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

    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.

  • πŸ‡³πŸ‡±Netherlands Skip van 't Hof Gouda

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

  • πŸ‡ΊπŸ‡ΈUnited States weekbeforenext Asheville, NC
    +++ 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.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • πŸ‡ΊπŸ‡ΈUnited States weekbeforenext Asheville, NC

    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: πŸ› Re-order + remove broken with the Entity Reference (and Table) widget Active .

  • πŸ‡§πŸ‡ͺBelgium joevagyok

    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.

  • πŸ‡ΊπŸ‡ΈUnited States Dave Reid Nebraska πŸ‡ΊπŸ‡Έ

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

  • πŸ‡§πŸ‡ͺBelgium joevagyok

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

  • Pipeline finished with Failed
    7 months ago
    #62696
  • πŸ‡§πŸ‡ͺBelgium joevagyok

    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.

  • Pipeline finished with Failed
    5 months ago
    #81425
  • πŸ‡§πŸ‡ͺBelgium joevagyok

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

  • Pipeline finished with Failed
    5 months ago
    #81430
  • πŸ‡ΊπŸ‡ΈUnited States recrit

    Added the latest patch for the MR for composer builds

Production build 0.69.0 2024