Deleting or renaming SDC prop will break xb pages using old version of the component

Funny enough, I opened 🐛 Follow-up for #3500386 Active less than 24 hours ago, and it’s closely related!

@heyyo What is the behavior that you would expect?
Thatd be really helpful 🙏😊

Also: what version of XB are you testing? Which commit?

There’s a whole range of ways we could go about this, but … that’d kinda be babysitting broken code, because this technically is a BC break in the SDC! 😅
Hence this is kinda a subset of 💬 ComponentNotFoundException: Unable to find component Active .

This morning around 6AM UTC time, I pulled last commit on 0.x after larowlan requested me to do so.
The bug was still present. I don't know the exact commit, I pulled another time after that.

I'm not sure I really know how it should behave, but it's quite critical in my mind.

We can't ask to clients to delete all their content using old sdc components, and recreate them.

But after my discussion with larowlan, I had the impression that larowlan thought it was possible to handle this scenario too with component versions. After that, it's above my knowledge :-)

If the prop is no longer needed, then having extra information in old content with nowhere for it to go seems like it should be possible to handle with component versioning. e.g. if you had an image SDC with alt and title tag, and you decided image titles are useless, then any existing titles in content can be ignored and eventually deleted.

If there's a new prop, then as long as it's optional then existing content should also continue to work.

So the case that seems like a real bc break would be adding a new required prop, or renaming an existing required prop. Then renaming an optional prop is borderline - that really needs the SDC to provide a mapping from old to new to protect against data loss, but also probably shouldn't fatal.

I also got an error, when adding a new prop(enum) to an existing sdc.
It's impossible to edit previous version of this same sdc.

Error in drupal logs when we want to edit this SDC:
Path: /xb/api/v0/form/component-instance/xb_page/10
Message:
TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in array_key_exists() (line 151 of /var/www/html/web/modules/contrib/experience_builder/src/Plugin/ExperienceBuilder/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php).

Is there any workaround ? except recreating the page using it ?

🌱 [META] Robust component instance error handling Active added protection for rendering components that broke/changed underneath us, but this is reporting something similar for the ComponentInputsForm, which did not get that treatment.

So: this title is kinda misleading/sounding unnecessarily scary. Pages are still rendering fine, "just" editing won't work. Retitled 👍

This is part of 🌱 [META] Production-ready ComponentSource plugins Active though. Added there 👍

Note this only affects sdc, not the js ComponentSource, due to code components disallowing such prop changes. Plan to allow that is at 🌱 Plan to allow editing props and slots for exposed code components Active , which ties into a lot of things that @catch wrote in #6.

However, there's one thing I don't understand yet: \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildConfigurationForm() does already do:

      $widget = $source->getWidget($component->id(), $component->getLoadedVersion(), $sdc_prop_name, $label, $field_widget_plugin_id);

IOW: it's already using the component instance's Component version. So what's going wrong here? 🤔

Could you please:

1. post the pre-change *.component.yml
2. post the post-change *.component.yml
3. post the SDC's Component config entity

?

Together, that should allow me to write a test and be confident we're fixing exactly what you reported 😊

Actually … I might actually understand (but still want that info, @heyyo! 🙏🙏🙏).

It's this:

    // To ensure the order of the fields always matches the order of the schema
    // we loop over the properties from the schema, but first we have to
    // exclude props that aren't storable.
    foreach (PropShape::getComponentProps($component_plugin) as $component_prop_expression => $prop_shape) {

… that intent is nice, but should've been revised when we introduced Component versions, because … that is actually reading the props from the on-disk SDC, not those of the component version!

… which together with

    prop_field_definitions:
      type: sequence
      orderby: key

is actually a problem. 😅 🫠 That means we're literally losing the order. So this will require a (slight) config schema change to be able to respect the order in the SDC:

diff --git a/config/schema/experience_builder.schema.yml b/config/schema/experience_builder.schema.yml
index 29e80c630..388da3019 100644
--- a/config/schema/experience_builder.schema.yml
+++ b/config/schema/experience_builder.schema.yml
@@ -414,7 +414,7 @@ experience_builder.generated_field_explicit_input_ux:
   mapping:
     prop_field_definitions:
       type: sequence
-      orderby: key
+      orderby: ~
       sequence:
         type: mapping
         mapping:

In working to fix this, I discovered a data loss bug 😱

While manually testing the fix for the reported bug (see @heyyo's #7), I couldn't get to the point where I had 2 component instances of different version in the component tree. First I thought "PEBKAC". And it was. While editing, I got 2 different versions 👍 But then upon saving, they both changed to the same version! 😱

IOW: the component version is always overwritten to the active component version, instead of retaining the original component version.

This appears to be a bug in the Auto-save functionality because the client sends the correct Component versions:

but the corresponding auto-save entry contains this:

                    [field_xb_demo] => Array
                        (
                            [0] => Array
                                (
                                    [uuid] => 89b1e8ac-ac5d-459a-94b9-f7a6d2f0b257
                                    [inputs] => {"text":"The og heading element","style":"primary","element":"h1"}
                                    [component_id] => sdc.experience_builder.heading
                                    [parent_uuid] => 
                                    [component_version] => acbd38ffd9d42d04
                                    [slot] => 
                                    [label] => 
                                )

                            [1] => Array
                                (
                                    [uuid] => c0d4e9e1-23c0-46ee-9a71-a75d6a5494cb
                                    [inputs] => {"style":"primary","text":"Dramatic title!","element":"h1"}
                                    [component_id] => sdc.experience_builder.heading
                                    [parent_uuid] => 
                                    [component_version] => acbd38ffd9d42d04
                                    [slot] => 
                                    [label] => 
                                )

                            [2] => Array
                                (
                                    [uuid] => 770f670f-5526-4704-9c55-8ac28b022751
                                    [component_id] => sdc.experience_builder.heading
                                    [component_version] => acbd38ffd9d42d04
                                    [inputs] => {"text":"drama","style":"primary","element":"h1"}
                                    [parent_uuid] => 
                                    [slot] => 
                                    [label] => 
                                )

                        )

… i.e. everything on acbd38ffd9d42d04 aka the latest version.

Will open a separate issue for that… 😬

I've traced #12 to a bug in \Drupal\experience_builder\ClientDataToEntityConverter::convert(), specifically:

$item_list->setValue(self::convertClientToServer($layout['components'], $model, $entity, $validate));

… but not through a flaw in Auto-save functionality! It appears to be a bug in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::setValue() and ::onChange() 🫠

Moved #12 and #13 to 🐛 Data loss: `ComponentTreeItem::setValue()` and `::onChange()` may inappropriately upgrade `component_version` to the latest version Active . That now blocks this.

🐛 Data loss: `ComponentTreeItem::setValue()` and `::onChange()` may inappropriately upgrade `component_version` to the latest version Active is green. Let's get an MR going here, because preferably we'd land configuration schema changes before a beta. (We can do it later, but ideally we wouldn't.)

Plus, this is kind of a glaring oversight from 📌 Version component prop definitions for SDC and Code components Active 🙈

Can you give this a try, @heyyo?

Also, there are 2 questions for product manager @lauriii:

1. https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
2. https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Applying the 2 patches on last dev of XB(#e9d0d1c6d)
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Rebuild of the bundle + clear cache of drupal + disable cache in browser console

1. Rename a prop from paddingInline to padding_inline + drush cr
Viewing or editing a page using previous prop name we got the error:
`OutOfRangeException

'padding_inline' is not a prop on the component 'Single-directory component: Linno Container'.`

  private function getDefaultStaticPropSource(string $prop_name, bool $validate_prop_name = TRUE): StaticPropSource {
    assert(isset($this->configuration['prop_field_definitions']));
    assert(is_array($this->configuration['prop_field_definitions']));
    $component_schema = $this->getSdcPlugin()->metadata->schema ?? [];
    if ($validate_prop_name && !array_key_exists($prop_name, $component_schema['properties'] ?? [])) {
      throw new \OutOfRangeException(sprintf("'%s' is not a prop on the component '%s'.", $prop_name, $this->getComponentDescription()));
    }

2. Deleting a prop from a SDC with content using this prop + drush cr
Exact same error than in 1.

3. Adding a new prop to an existing SDC with existing content + drush cr
- Visiting a previous page, the page is ok
- Editing the page, then selecting the SDC to edit: impossible we got the error "An unexpected error has occurred while rendering the component's form. SyntaxError: Unexpected token '<', "

🐛 Data loss: `ComponentTreeItem::setValue()` and `::onChange()` may inappropriately upgrade `component_version` to the latest version Active landed just now.

@heyyo That is incredibly fast testing! 😮 Thanks!

Looks like I missed a bunch of cases; sorry — absolutely didn’t mean to waste your time 🫣

Next up: decisions from Lauri.

@heyyo That is incredibly fast testing! 😮 Thanks!

You are the superman, man ! @wim
You are in all issues, commenting, testing, coding, patching, reviewing always nicely and with humor

Looks like I missed a bunch of cases; sorry — absolutely didn’t mean to waste your time 🫣

Waste my time ? When you and all developers are making such a great product, giving from your time for so many intense days.

🎤 is back to @laurii 😂

#19 looks like @lauriii provided the info on the MR