Problem/Motivation

This is very niche, but a showstopper for us. If there happens to be two different entity forms, (not subforms) with the same field name (the field that is using the media library widget), then the field_widget_id that is used to attach the selected media entities will be identical.

In MediaLibraryFieldWidgetOpener.php

    $response
      ->addCommand(new InvokeCommand("[data-media-library-widget-value=\"$widget_id\"]", 'val', [$ids]))
      ->addCommand(new InvokeCommand("[data-media-library-widget-update=\"$widget_id\"]", 'trigger', ['mousedown']));

This will simply be, for example, "field_image".

I understand that this is not technically a core issue as the only way to currently have these embedded forms is with some sort of contrib module. I do believe it's worth talking about, for a few reasons:

  • There is no way to fix this in a contrib module, they cannot alter the id that's used here
  • There are a number of feature requests that will eventually have this functionality in core, in a number of different ways. I imagine it will come up at some point. #1728816: Ensure multiple entity forms can be embedded into one form
  • Even if this should be addressed in contrib, it would be nice to have a more centralized reference.

Steps to reproduce

The simplest way to re-create is with the contact_block module, (very high usage). Install Drupal demo_umami (the easiest way) with contact_block enabled.
Add a media reference field to the "Website feedback" contact form. "field_image", using the media library widget.
Add an instance of the website feedback contact block to the content area.
Add another instance of the website feedback contact block to the content area. (two separate blocks)
The forms themselves have proper unique IDs contact-message-feedback-form--2, contact-message-feedback-form--1, etc.
Adding media to the second block, however, will always add an image to the first block.

Proposed resolution

One potential solution is the way the id is formed. In MediaLibraryWidget::formElement the $id_suffix could include more criteria. I attached a patch with the entity uuid, but this isn't meant to be a real solution, this is just to fix it for now and use in composer.

Another potential, (and maybe better) solution is the InvokeCommand used in MediaLibraryFieldWidgetOpener. Perhaps this selector should have a wrapper element ID.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3345064

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

asherry created an issue. See original summary.

asherry’s picture

Issue summary: View changes
asherry’s picture

eglaw’s picture

Saved my day !!!
Ya da man!

droath’s picture

Status: Active » Reviewed & tested by the community

This fixed an issue I was having in layout paragraphs where the same media entity was getting added into multiple fields that had the same machine name.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1.15 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

hudri’s picture

Priority: Normal » Major

A media library field_image on both, a node and a paragraph, seems a quite natural and common use-case and not that niche to me. Bumping to major.

rpayanm made their first commit to this issue’s fork.

super_romeo’s picture

justin2pin made their first commit to this issue’s fork.

jnettik’s picture

I can also confirm the patches here fix the issue. It also doesn't feel trivial to me as well.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ant1 made their first commit to this issue’s fork.

ant1 changed the visibility of the branch 10.2.x to hidden.

ant1’s picture

Status: Needs work » Needs review

ant1 changed the visibility of the branch 3345064-10.2.x to hidden.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, as a bug will need a test case or simple assertion added.

J-Lee’s picture

However, it is theoretically possible for $this->fieldDefinition->getUniqueIdentifier() to return an empty string.

J-Lee’s picture

The tests use \Drupal\Core\Field\BaseFieldDefinition, which returns a non-unique identifier for the same field name.

Did someone have a suggestion for the best way to test this?