Problem/Motivation

Note: AI assisted issue, but human investigation and fix tested within Display builder.

When a Drupal entity with a ui_patterns_source field is created, the TypedData prototype mechanism injects a null placeholder item into the field list. FieldItemList::filterEmptyItems() is supposed to remove it (called from applyDefaultValue() and preSave()), but it doesn't, because SourceValueItem does not override isEmpty().

The inherited Map::isEmpty() checks $property->getValue() !== NULL for each sub-property. The source and third_party_settings sub-properties are MapDataDefinition instances whose getValue() returns [] (empty array), not NULL. Since [] !== NULL, isEmpty() returns FALSE for a fully null/unset item, and the phantom survives.

Additionally, setValue() silently bails when source_id is empty, so calling $entity->set('field', []) does not clear the phantom — it just no-ops.

Steps to reproduce

$entity = MyEntity::create([]);
$values = $entity->get('my_ui_patterns_source_field')->getValue();
// Expected: []
// Actual: [['node_id' => NULL, 'source_id' => NULL, 'source' => [], 'third_party_settings' => []]]

Proposed resolution

Override isEmpty() in SourceValueItem to delegate to source_id, the only property that makes an item valid. Read $this->values/$this->properties directly rather than calling $this->get('source_id') to avoid the side effect of materializing the property object, which changes the key order returned by getValue().

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ui_patterns-3604317

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:

About issue forks

Comments

mogtofu33 created an issue.

mogtofu33 opened merge request !514

pdureau’s picture

pdureau commented

The previous implementation (until #3584856: Source field model and storage) of ComplexDataInterface::isEmpty() for SourceValueItem was:

  public function isEmpty() {
    return empty($this->values);
}

It was working well with Display Builder, and still is, and its simpler than the proposal in the MR. What do you think, Jean?

mogtofu33’s picture

mogtofu33 commented

This was not enough when testing with Display Builder, did you test with DB?

mogtofu33’s picture

mogtofu33 commented

Map::setValue() contains this:

$this->values = $values;

foreach ($this->properties as $name => $property) {
    $value = $values[$name] ?? NULL;
    $property->setValue($value, FALSE);
    unset($this->values[$name]); // ← key removed from values
}

When setValue() is called on an item where source_id has already been materialized into $this->properties (by a previous $this->get('source_id') call anywhere in the call stack), source_id is set on the property object and then deleted from $this->values. After that, $this->values['source_id'] is undefined.

So empty($this->values['source_id']) would return TRUE — incorrectly marking ailterEmptyItems() would then delete it during save.

The array_key_exists check distinguishes two reasons source_id might be absent from $this->values:

  • Phantom item (never set): not in $this->values, not in $this->properties → return TRUE
  • Real item, source_id moved to properties: not in $this->values, present in $this->properties → must check properties
  • Real item, never accessed via get(): present in $this->values → check there directly

Without the guard, the phantom case and the "moved to properties" case are indistinguishable by $this->values alone, and a valid item gets deleted.

pdureau’s picture

pdureau commented

This was not enough when testing with Display Builder, did you test with DB?

Yes, it was looking enough on my local environment. But it was just a proposal.