Problem/Motivation

There is an issue with the EntityReferenceBrowserWidget where the target_id is not being reset correctly when an entity is removed, and the modal does not open correctly when adding a new entity if the field's cardinality is set to 1.

Steps to reproduce

  1. Create a field with EntityReferenceBrowserWidget and set its cardinality to 1.
  2. Add an entity using the entity browser.
  3. Remove the entity.
  4. Try to add a new entity.

Expected Behavior:
The target_id should be reset when an entity is removed.
The modal should open correctly when adding a new entity.
Actual Behavior:
The target_id is not reset when an entity is removed.
The modal does not open correctly when adding a new entity.

Proposed resolution

  1. Ensure that the target_id is reset when an entity is removed.
  2. Update the #default_value of the target_id element to handle the case when no entities are present.
  3. Ensure the modal opens correctly when adding a new entity.
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:

Comments

v.dovhaliuk created an issue. See original summary.

v.dovhaliuk’s picture

v.dovhaliuk’s picture

Status: Needs work » Needs review
v.dovhaliuk’s picture

The patch has been updated to fix the warning:
Warning: Undefined array key -1 in Drupal\entity_browser\Plugin\Field\FieldWidget\EntityReferenceBrowserWidget->formElementEntities() (line 779 of /var/core/www/modules/contrib/entity_browser/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php).

v.dovhaliuk’s picture

v.dovhaliuk’s picture

Version: 8.x-2.10 » 8.x-2.13
v.dovhaliuk’s picture

The patch has been rerolled for the compatibility with the 2.13 version.

recrit’s picture

@v.dovhaliuk - you may want to try the fix on #2851580: Re-order + remove broken with the Entity Reference (and File) widget instead since it has some cardinality fixes as well.

benstallings’s picture

Status: Needs review » Needs work

Claude Code says (emphasis added):

Change 1: isset guard around $is_relevant_submit (line 785)

Before: The $is_relevant_submit &= expression accesses $trigger['#parents'][$field_name_key] inline within the isset(), but the entire expression (including the array_slice comparisons) is evaluated as part of one &= statement even when the key doesn't exist.

After: Wraps the whole block in if (isset($trigger['#parents'][$field_name_key])).

Issue: This changes the logic subtly. Before, if $trigger['#parents'][$field_name_key] was not set, the isset() returned FALSE, the &= made $is_relevant_submit falsy. After the patch, if the key is not set, the if block is skipped entirely, and $is_relevant_submit stays TRUE (from line 779). This means a submit from an unrecognized trigger structure would now be treated as relevant when it shouldn't be. This looks like a bug introduction — the original code's behavior of falling through to FALSE was correct.

Change 2: Broadened $ief_submit detection (line 882)

Before: Only triggers starting with 'ief-edit-submit' were treated as IEF submits.

After: For cardinality-1 fields, also treats any triggering element containing 'entity_browser_entity_browser' as an IEF submit.

This is the core fix — when entity browser is used inside a cardinality-1 field, the triggering element name doesn't start with ief-edit-submit but does contain the entity browser name. Without this, getEntitiesByTargetId() returns FALSE too early, and the widget loses track of selected entities.

Concern: str_contains($triggering_element_name, 'entity_browser_entity_browser') is a very broad match. It matches any triggering element that happens to contain that string, not just the entity browser's own submit. And the substring entity_browser_entity_browser looks like it assumes a specific browser machine name — what if the browser is named something else? This should probably match on a more general pattern, or the condition should be explained in a comment.

Change 3: NULL check on $current_user_input (line 890)

Before: if (!is_array($current_user_input)) — if the value is NULL (key exists but value is NULL), this passes the NULL to EntityBrowserElement::processEntityIds().

After: if ($current_user_input !== NULL && !is_array($current_user_input)) — NULL values now fall through to return FALSE.

This is a reasonable defensive fix. If the target_id key exists in user input but has a NULL value, processing it as entity IDs would fail or produce unexpected results.

Verdict

Change 3 (NULL guard) is clean and correct. Change 2 (broader IEF detection) addresses a real problem but the matching string 'entity_browser_entity_browser' feels fragile and under-documented — is that always the pattern, or only for browsers with a specific naming convention? Change 1 (isset guard) appears to introduce a logic regression where $is_relevant_submit can remain TRUE when it shouldn't. That change needs reworking — the original fallthrough-to-false behavior should be preserved while still avoiding the PHP warning that likely motivated it.

benstallings’s picture

Version: 8.x-2.13 » 8.x-2.x-dev
Assigned: Unassigned » benstallings

benstallings’s picture

Assigned: benstallings » Unassigned
Status: Needs work » Needs review
anybody’s picture

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

For the future we should have a test here IMHO.

benstallings’s picture

Assigned: Unassigned » benstallings
benstallings’s picture

Assigned: benstallings » Unassigned
Status: Needs work » Needs review