Data loss: `ComponentTreeItem::setValue()` and `::onChange()` may inappropriately upgrade `component_version` to the latest version

This blocks fixing 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active .

This regression appears to have been introduced in 📌 Version component prop definitions for SDC and Code components Active .

So turns out this is literally doing what I was concerned about:

    // TRICKY: do this *only* when no version is specified, otherwise this would
    // unintentionally "upgrade" instances of older component versions to newer
    // ones!

😅

Test coverage pushed 👍

Test coverage is proving the problem. 👍

AFAICT the root cause is
this bit in \Drupal\experience_builder\Plugin\DataType\ConfigEntityVersionReference::setValue():

    if (\is_string($value)) {
      // If only a single value is passed, this is a config entity ID.
      $this->id = $value;
      // Reset the version ID as none was passed we default to the active
      // version.
      $this->version = NULL;
      $target = $this->getTarget();
      \assert($target instanceof ConfigEntityVersionAdapter || $target === NULL);
      $this->version = $target?->getValue()->getActiveVersion();
      $this->doNotify($notify);
      return

when it gets called with that string value from \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::onChange() where it does:

$this->writePropertyValue($property2, $this->get($property1)->getValue());

(at this point, $property2 == 'component' and $property1 == 'component_id', which of course does not include the component version, so the string ConfigEntityVersionReference::setValue() receives is indeed only a component ID)

in combination with

    if ($property_name === 'component_version') {
      // Reset the component reference property.
      $component_id = $this->get('component')->getTargetIdentifier();
      $version_id = $this->get('component_version')->getValue();
      if ($component_id !== NULL && $version_id !== NULL) {
        $this->writePropertyValue('component', [
          'target_id' => $component_id,
          'version' => $version_id,
        ]);
      }
    }

in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::onChange().

in combination with:

    // DX: if no version is specified, set it automatically to the active
    // version of the Component.
    // TRICKY: do this *only* when no version is specified, otherwise this would
    // unintentionally "upgrade" instances of older component versions to newer
    // ones!
    if (!array_key_exists('component_version', $this->values) && ($property_name === 'component_id' || $property_name === 'component')) {
      // Set the version ID based on the loaded component.
      $this->writePropertyValue('component_version', $this->getComponent()?->getLoadedVersion());
    }

in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::onChange() incorrectly concluding that no version is specified when in fact it was, but \Drupal\Core\TypedData\Plugin\DataType\Map::setValue()'s

    // Update any existing property objects.
    foreach ($this->properties as $name => $property) {
      $value = $values[$name] ?? NULL;
      $property->setValue($value, FALSE);
      // Remove the value from $this->values to ensure it does not contain any
      // value for computed properties.
      unset($this->values[$name]);
    }

does that unset() leading ComponentTreeItem::onChange() to reasonably deduce that in fact no version was specified 🫠

🤪 What a crazy crazy combination of circumstances. Death by a thousand cuts.

This is ready for final review — preferably by @larowlan, because he's the original author of these pieces, and I doubt we have anybody else on the team with the same level of Field API/Typed Data API expertise 😅

Awesome work @wim leers

Merged to 0.x

This was always going to be tricky, I remember how hard these were to get right for Dynamic Entity Reference and I think Entity Reference Revisions probably had similar challenges. I wonder if we should look to make version a required key when calling setValue - it would mean a lot of changes in tests but the 'automatically picking the latest version if not found' logic has bitten us a couple of times now.

Automatically closed - issue fixed for 2 weeks with no activity.