Version component prop definitions for SDC and Code components

.

This is very similar to the outline → I wrote on 🌱 [META] Production-ready ComponentSource plugins Active .

The key difference: I tried to do it generically, not restricted to SDCs & code components. So, for example, Block plugins may change (evolve) their settings and provide an update path, and so XB should be able to apply such an update path. Layout Builder hasn't solved this yet either — see the 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active core issue

Specifically doing this for only the sdc and js component sources does not make that much sense to me yet. What problem does that solve? 🤔

To avoid the need to look up in some central place what field type + storage settings + instance settings + widget should be used for a given SDC prop is … exactly why StaticPropSources contain all that information, and why that is stored in the inputs field property of the XB field type.

And … I think that is what you're aiming to solve here: storing less data in inputs. Even though you omitted it from the issue summary: you only showed the before vs after for the tree field property, not for the inputs field property 😄 Updated issue summary based on this interpretation. But I'd like you to confirm it. 🙏

Very closely related, and one of the oldest XB issues: 📌 [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed .

From @larowlan's issue summary:

1. Adding a new required prop - this is relevant if we go down the non JSON storage approach
2. Removing a value from an enum
3. Changing the mapped field type
4. Removing a prop - although the net outcome here is redundant data in the database for old components - so we might not care here

I don't think I agree with stating these are things we have to support:

1. This is sort of a BC break, except we could automatically fix this: detect the stored version not matching the current version, and add the missing default — both at render time and at edit time, which means after saving it'll have been automatically fixed.
2. This is a BC break we cannot recover from: either we drop the user's choice (data loss) or the rendering crashes — which is exactly why 📌 Improve server-side error handling Active has explicit test coverage for this.
3. This is what hook_storage_prop_shape_alter() allows and what #5 is about. And it's what allows the storage efficiency improvement described in #4.
4. Equivalent to point 1, except that here too there would be data loss — especially if a future revision of the SDC restores this. Note that redundant data (aka prop sources for non-existent props) will not be accepted by \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::validateComponentInput().

So: what is your purpose with this issue, @larowlan? Storage efficiency, input schema evolvability, both, or something else still?

The goal is both. At present we store all of this in the prop source (and in the inputs column) but the repetition of e.g. storing all of the instance settings each time we use it is very verbose. Consider this

'element' => [
          'sourceType' => 'static:field_item:list_string',
          'value' => 'h1',
          'expression' => 'ℹ︎list_string␟value',
          'sourceTypeSettings' => [
            'storage' => [
              'allowed_values' => [
                [
                  'value' => 'div',
                  'label' => 'div',
                ],
                [
                  'value' => 'h1',
                  'label' => 'h1',
                ],
                [
                  'value' => 'h2',
                  'label' => 'h2',
                ],
                [
                  'value' => 'h3',
                  'label' => 'h3',
                ],
                [
                  'value' => 'h4',
                  'label' => 'h4',
                ],
                [
                  'value' => 'h5',
                  'label' => 'h5',
                ],
                [
                  'value' => 'h6',
                  'label' => 'h6',
                ],
              ],
            ],
          ],
        ],

That's a lot of data to store for every instance of the element prop in the heading component. All that is unique to this instance is the value , expression and the source type.

When we have a dynamic prop source we're only storing this

'image' => [
          'sourceType' => 'dynamic',
          'expression' => 'ℹ︎␜entity:node:article␝field_hero␞␟{src↝entity␜␜entity:file␝uri␞␟url,alt↠alt,width↠width,height↠height}',
        ]

which is much more succinct.

The intention here is to try to hoist some of the repeated elements (e.g source type settings) to somewhere we can rely on them being fixed. The idea is the versioning gives us a mutable place to store them. And in doing so it also opens up some more flexibility with regards to recovery for stored data in previous iterations.

I agree that slimming down the inputs to remove a lot of duplicated data is worthwhile, although this feels like it adds a huge amount of complexity - and would this be better off as some kind of new core API?

We also still have to handle error cases: because config is mutable we might still end up with references to versions that no longer exist in the config object.

Could this be different config entities for different 'versions' of the component, instead of trying to store all version information in a single config entity?

That would potentially make it easier to determine whether any content is using the old version, allow old versions no longer in use to be deleted etc.

@larowlan in #7:

both

Hah! I should've known 😅

@longwave in #8:

I agree that slimming down the inputs to remove a lot of duplicated data is worthwhile, although this feels like it adds a huge amount of complexity

+1 to both those statements.

would this be better off as some kind of new core API?

🤔 I'm not sure I follow. Are you thinking of ✨ Add way to "intern" large field item values to reduce database size by 10x to 100x for sites with many entity revisions and/or languages Active ? If so: I don't see how that applies here, because this is not about "interning", but about compacting? If not that: can you elaborate? 🙏

@catch in #9: I don't think there will be a big number of "versions" of a particular component. So storing all versions in a single Component config entity seems reasonable. Especially because we aim to allow instances using the older version to be able to transition/update to the newer versions (until none of the old version are left, and the old version can be deleted).

That being said … making the version part of the Component config entity ID would make what I summarily described → in 🌱 [META] Production-ready data storage Active trivial:

Expand on what 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active did: omit Component config entity dependencies from deps_config, and introduce a separate component_versions field prop, to store <component config entity ID>:<version> <component config entity ID>:<version> … → allows finding which revisions are using not the latest version of a component

So that is definitely worth considering, too.

I think @larowlan has a good (implied) point: if it's okay for us to rely on "external data" for DynamicPropSources (aka the FieldConfig entity, the host entity an instance lives in, etc.), then why is it not for StaticPropSources?

So: let's get a working PoC and then re-assess 👍

@larowlan Given that this will touch the Component config entity implementation, its config schema, and (Static|Dynamic)PropSource, I think it probably makes sense for me to take this one on. Then you can tackle 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed at the same time? 🤓

If so: I don't see how that applies here, because this is not about "interning", but about compacting? If not that: can you elaborate?

If we have some concept of versioned config entities then maybe that can help with 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active as well?

@larowlan Given that this will touch the Component config entity implementation, its config schema, and (Static|Dynamic)PropSource, I think it probably makes sense for me to take this one on.

Whole heartedly agree, was hoping you might be interested in taking this on ❤️

@longwave: yep, see my response to @catch above :)

Getting this going 👍

Discussed this with @effulgentsia and there is another third option where we split the 'field union' bits of the component config entity (aka the bits that store the field types and settings) into a separate config entity that is similar to what field union config entities look like.
And then the component config entity can reference this, as can the component tree field type.

So the data model would go from

[
        'parent_id' => 'parent-uuid',
        'slot' => 'column_one',
        'component_id' => 'sdc.experience_builder.image',
        'uuid' => 'some-uuid',
        'inputs' => [
          'image' => $stuff_here,
        ],
      ],

to

[
        'parent_id' => 'parent-uuid',
        'slot' => 'column_one',
        'component_id' => 'sdc.experience_builder.image',
        'inputs' => $some_new_config_entity_id, // 👈️ This is new and would be NULL for blocks
        'uuid' => 'some-uuid',
        'inputs' => [
          'image' => $stuff_here,
        ],
      ],

We can use typed data setValue to detect when 'component_id' is set and default 'inputs' to the current version of the field union-y thing.

That gives us scope to have something 'dynamic field-union' like later

#15 sounds very encouraging.

I like it! Wish I thought of it! 😄

Consider it done. I'll be spending my AM getting this going! 🤓🕺

Got the foundations ready:

1. type: versioned_config_entity, a subtype for type: config_entity
2. an accompanying \Drupal\experience_builder\Entity\VersionedConfigEntityBase
3. moved the settings.local_source_id to the top level at source_local_id (sibling to HEAD's source) — which makes
4. moved settings.* (aka everything besides the intra-source plugin ID) to versioned_properties.settings.* — which makes them versionable 👍
5. moved fallback_metadata (currently only slot definitions) to versioned_properties.fallback_metadata
6. thanks to this, there's no more need for changing source — we can use the version itself to determine when to use the Fallback component source plugin

The 4 crucial tests largely (but not completely) pass ((Block|SingleDirectory|Js)ComponentTest + ComponentValidationTest). Initially ~58% was passing , then ~84% passing and at end of day TBD % passing.

idempotently computing the version hash.

I said was the next step. I forgot about one other one: validation of the new structure, to ensure >=1 version exists, that the active version points to an existing version, that versions have a consistent format, and … making source_local_id immutable.

Now then, the computing of that version hash: implemented (including validation, again), but the validation is giving me headaches. 😅
I begrudgingly added recursive key sorting (copy/pasted from AutoSaveManager::recursiveKsort()), which allowed me to get tests to pass more for SDCs, but still not completely. We shouldn't do that sorting here though, because the order could be meaningful.
Worse, that wasn't enough. Root cause AFAICT: the configuration system upon saving sometimes converts true to 1 and false to ''. This then unsurprisingly results in the hash not being the one that was expected.
I have one bit of hope: applying the same trick as \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::fixBooleansUsingConfigSchema() — i.e. use config schema to perform the appropriate normalization. We'll see if that works out. If it doesn't, I think deferring validating the generated versions to a follow-up issue is reasonable. All of the validation at the very start is what truly matters for data integriy — this was
going the extra mile.

🤯
Core's got this:

field.value.boolean:
  type: mapping
  mapping:
    value:
      type: integer
      label: 'Value'

… but that should obviously be type: boolean.

Fixing that alone makes a big difference: before (83.29% passed) vs after (86.89% passed).

I'm trying to understand what the implications of this are if someone changes things then changes them back again.

Will it be:

version 1
version 2
version 3 (identical to version 1)

Or... will it be:
version A
version B
version A

?

For an upgrade path, version 1 would need to be updated to version 2, then to version 3, but Version A would be able to remain version A if it hasn't already been updated to version B.

Apologies if this is hard to parse, that's the third rewrite..

Now using config schema as described in #20.2. That brings us down to 86.18% passing.

To avoid a single piece of (very detailed) validation to cause an already big MR to grow a lot in size (currently: 29 files, +862, −422), I commented out a single config schema line which gets us back to ~98% passing 👍

We could live without this validation although it IMHO would be preferable not to.

I have to switch to other work now. To be continued, probably later this week. Let's hear from @larowlan if this is what he had in mind. 😊

Next steps here (beyond getting all tests to pass) are to update the tree and inputs data structures — although preferably that'd happen after @larowlan's 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is merged, to avoid huge conflicts and significant rework. Marking [PP-1] for that.

@catch in #22: for the actual upgrade path logic, see:
📌 Component Source plugins: support for schema changes of explicit inputs Postponed , with within that:

• 📌 [SPIKE] Prove that it's possible to apply block settings update paths to stored XB component trees Active → your core issue: 🐛 Block plugins need to be able to update their settings in multiple different storage implementations Active
• changing decisions about which field types/storage+instance settings/widget to use: 📌 [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed
• … more to follow

This issue should IMHO be focused solely on:

1. Making the Component config entity know about/remember different versions
2. Updating the stored component instances to also store the version that was active at the time of their creation
3. … and creating that version hash solely based on the data stored in type: experience_builder.component_source_settings.(block|js|sdc), which itself actually is the result of a computation in the respective ComponentSource plugins

Observations for those 3:

1. It's not about updating existing component instances. This is what you're referring to AFAICT.
2. Perhaps this second bit ought to be split out — even if it's just another MR on this same issue. Because as just explained in #23: this is a pretty big MR already.
3. It's also not about making ComponentMetadataRequirementsChecker and friends detect that the explicit input schema has changed, judge whether that's feasible or not, and then for example disable the affected Component config entity. This is why my 3rd "solely about" point is so narrow.

@WimLeers, well this has implications for the upgrade path.

There's a huge difference between:

version 1
version 2
version 3 (identical to version 1)

and:

hash 1
hash 2
hash 1 (identical to version 1).

in that there'd be no need to update hash 1 to hash 1, which would be good. I think that's what this issue results in, but it would be good to confirm.

in that there'd be no need to update hash 1 to hash 1, which would be good.

That's indeed what I'm aiming for, but per #20.2 and #23, I'm running into challenges that AFAICT are caused by the config system modifying the contained values 😅

Deterministic hashing also requires a deterministic input, and that's where the config system is currently getting in the way. That's the bit I'd want to push to a follow-up issue, because that likely requires complex tweaks and shenanigans. This MR in and of itself already moves us to a far better place.

Plus, we can completely transparently recompute the hashes at a future time, given they DO only rely on the stored settings. 👍 The problem is that those settings themselves are neither ordered nor casted deterministically today. AFAICT \Drupal\Core\Config\StorableConfigBase::castValue() should help us get there, but that is AFAICT not available to me for use on arbitrary config subtrees: \Drupal\Core\Config\Config::save() calls it and nothing else does. I'd have to do some shenanigans for XB to be able to use that. That could easily end up being a day-long and multi-100-LoC endeavour, which is why I'm advocating for focusing on getting the mechanics right in this MR.

wim leers → changed the visibility of the branch 3484682-handle-update-and to hidden.

@larowlan in chat:

In comment #17 you mentioned that you liked this approach proposed by alex and I (#15) - and catch was also in favour of it (#16) but that doesn’t seem to match what’s in the current MR.

Hah, I think it’s actually very simple: I wrote that on May 16 (Friday). Then I didn’t get to spend my AM that day on it like I said/hoped. 😑

I actually started on Monday, May 19. And I literally started from the POV: “how can we do this generically?” — because we likely want more versioned config at a later point. For example, product requirement 37. Revisionable templates — but I know that @lauriii has expressed the intent to do it for more config than that.

Then I looked at the issue summary, and what I'd done was perfectly on track to achieve that.

tl;dr: I simply forgot about #15 🙈🙈🙈

Having worked on both this and 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed , I now have some questions about #15 (which looks super familiar having spent the entire day reviewing #3468272 🤠):

1. the "to" example lists the inputs key twice. That's accidental, right? I think It'd essentially be:
   

   …
       component_id:
         type: string
         label: 'Component config entity ID'
         constraints:
           ConfigExists:
             prefix: experience_builder.component.
       shape_matched_field_union_id:
         type: string
         label: 'Shape-matched auto-generated field union, if any'
         constraints:
           ConfigExists:
             prefix: experience_builder.shape_matched_field_union.
         # NULL for block components, used only for ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase-powered Component Sources
         nullable: true
   …
      

2. What would sensible identifiers for the new config entity type (as you can see, I suggest ShapeMatchedFieldUnion) be?

   Is it the component ID that it was generated for? That'd make it behave much like the FilterFormat–Editor twin/sibling pairing. In that case, I'd imagine you'd want to use type: versioned_config_entity and VersionedConfigEntityBase .

   Or would the ID be something that's based on the contents of the field union? The appealing thing about that is that it'd allow multiple Components to use the same ShapeMatchedFieldUnion. But: how often would that even happen?

3. But … actually that shape_matched_field_union_id key-value pair that references another config entity … must actually itself be versioned, because we need to be able to know for a given Component config entity what all of its previously used field types/instance+storage settings were (aka all of the ways their explicit inputs have ever been stored).
   Without that, we won't be able to do 📌 Component Source plugins: support for schema changes of explicit inputs Postponed (adding as related) nor 📌 [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (already linked).

Thanks, yeah I actually think with the versioned config entity we still have the pieces we need to generate a dynamic field union
I'll have a think about how we can actually use that, I suspect it will be via typed data reference computed properties on the inputs column.

i.e. I'm thinking $entity->get('component_tree')->get(2)->inputs->some_prop_that_is_modeled_by_a_link_field->uri might be possible.

I think that's the kind of appeal we're looking to borrow from field_union (rather than the 1 table per field etc) - can you confirm @catch

I'm behind on the past handful of comments and only replying to #29 and #30 right now, hopefully doesn't make things more confusing.

The two things I think we benefit from something field union-ish:

1. A coherent representation of what field types are in use for the values that now get stored per row with component_id | inputs in 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed e.g. component A includes field types foo and bar. This ought to help with dependency calculation and various other things.

I think that's what this issue is explicitly about.

2. The potential to use field unions as part of an overall data model in general

e.g. field unions being available as structured fields which can be laid out by XB, without having to be in exposed slots (the album track listing and other examples). It feels very likely that if this isn't available, people will hack together similar functionality via exposed slots, or continue to use paragraphs for that. It may not be necessary for XB iin tself, because for XB a field is a field, but it might be necessary for XB to displace paragraphs and/or circumvent people coming up with horrible workarounds for this use case.

I had hoped in #3440578-80: JSON-based data storage proposal for component-based page building → that XB could 'build upwards' from a dynamic field union so that the XB field would be something like a compound field that included the dynamic field union + the extra slot and parent data. This is also a bit like what some of the approaches discussed in 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active were like.

Now that 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is in, it's much closer to this, but not quite the same, but it feels like this issue could bring it a step closer again, and #15 seems like doing that.

e.g. I could imagine a dynamic_field_union module having a field where the two columns are 'field_union' (config entity reference) and 'values' (JSON).

And I think that #15 is saying that if XB splits the config entity, then there would be no difference between those config entities, so that they could potentially even be shared between the XB field and a field union module, or one could extend from the other, or something like that. And then XB would then have a second config entity for any extra bits that aren't explicitly about the field types.

But … actually that shape_matched_field_union_id key-value pair that references another config entity … must actually itself be versioned, because we need to be able to know for a given Component config entity what all of its previously used field types/instance+storage settings were (aka all of the ways their explicit inputs have ever been stored).

Couldn't this information be determined by querying the field table itself - e.g. SELECT DISTINCT(field_union) FROM xb_field WHERE component = %component (but a nice entity aggregate query).

Now that 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is in, it would be helpful if the before and after examples in this issue summary were updated to be based on the new schema that landed in that issue.

AFAICT, the "after" in this issue will result in the inputs column being reduced to containing only the value key for each input, is that correct? For example, for an instance of a heading component with 3 props, the inputs column would look like:

{
    "text": {
        "value": "Example heading",
    },
    "style": {
        "value": "primary",
    },
    "element": {
        "value": "h1",
    }
}

If I'm understanding that correctly, then would it be reasonable for the scope of this issue to also include removing the value key since it would become superfluous? In other words, could we simplify the above to:

{
    "text": "Example heading",
    "style": "primary",
    "element": "h1"
}

Comment #32 is based on the assumption that we're versioning the union of the inputs, not each input individually, and therefore the "version" (or "field union ish" config entity reference) would be pulled out into its own column, but please correct me if that's an incorrect assumption.

#32 shows an example that only includes scalar values, but it would work for objects/arrays as well. For example, if you have an input that's an array of links, you could still remove the superfluous value key. I.e., instead of:

{
  "links": {
    "value": [
      {
        "uri": "https://example.com/page1",
        "options": []
      },
      {
        "uri": "https://example.com/page2",
        "options": []
      }
    ]
  }
}

you could have:

{
  "links": [
    {
      "uri": "https://example.com/page1",
      "options": []
    },
    {
      "uri": "https://example.com/page2",
       "options": []
    }
  ]
}

e.g. field unions being available as structured fields which can be laid out by XB, without having to be in exposed slots (the album track listing and other examples). It feels very likely that if this isn't available, people will hack together similar functionality via exposed slots, or continue to use paragraphs for that. It may not be necessary for XB iin tself, because for XB a field is a field, but it might be necessary for XB to displace paragraphs and/or circumvent people coming up with horrible workarounds for this use case.

I think we might be talking about two problems here and how we solve them requires coming at this problem from the two different angles.

On one hand the 'a field is a field' is indeed the domain of custom field or a (completed) field union module (or tools like drush generate that will create you a custom plugin). This is the best solution for structured data - I think the album track listing falls under that use-case. I think modelling that as components is wrong as you lose all of the strengths of Drupal. If you need to do presentation of that field with XB, dynamic prop sources can cover it.

The other problem where there's repeating multi-column data is purely presentational - e.g. something like a carousel with an image field, a caption and a blurb, there's little re-use of that so structured data isn't as applicable. I think 📌 [PP-1] Support `{type: object, …}` prop shapes that require *multiple* field types Postponed is how (as I understand it) XB plans to tackle that. That in itself does involve either borrowing from or leveraging field union. We already support array structures in SDC definitions when the array items match an existing field type (i.e multi-value text field, link field) but not for arbitrary object shapes - and that's where field union (or borrowing from) is useful - effectively deriving fields from thin air. I think that works with the pieces of field union that already exist (data structure and widgets). The bits of field union that are still in progress (UI to manage all the things) aren't needed here because we don't need people to build them from the UI, we just need the config entity and the widget to exist.

I think this discussion would be easy if we had have had enough time in the last 7 years or so to finish field union. But we haven't - the most feature complete work is in the branch of 📌 Add UI for adding unions (field_union_ui.module) Needs work - it contains the pieces we need (widget, config entity) but no formatter - which in an XB world we don't need. I think there is merit in a spike to lift those pieces out of Field Union into XB with #15 as the data model. I think that is probably 2-3 days work to validate it. If that happened and this ended up living in XB rather than Field Union I would be OK with that. Field Union has never even had an alpha release and hence if it lives on in a different form that is totally fine. If XB ends up in core in the future, even better. Field Union then just becomes 'field union UI' module and includes the forms/routes etc for creating the config entities and a formatter for anyone not using XB for entity rendering.

Updated the issue summary per #32 to #34.

If we do pivot to the approach in #35 and #15 then the work in the MR here isn't wasted because as @wim leers points out (I thought here, but perhaps elsewhere) there is a product requirement to version content templates and patterns.

Answering the questions in #29

1) yes that was an accident, there shouldn't be two inputs, the first one should have read 'field_type' or 'schema' or whatever we call the new secondary config entity.

2) I would think 'DerivedFieldType' because I think there's a use case to use it with arrays of objects per 📌 [PP-1] Support `{type: object, …}` prop shapes that require *multiple* field types Postponed (what a neat nid, just needs a 5 in there). If we know we're going to need this functionality to make that happen, it makes sense to evaluate if it is feasible here and smooth the path to that in the future.

3) I think it could just be a new config entity, because it is effectively a new field type

So do we think there is merit in doing a spike on #15 / #35 ?

So do we think there is merit in doing a spike on #15 / #35 ?

Yes, please! 😊🙏

The more concise structures in #32/#34 look great, it's not a massive reduction in storage requirements but it's definitely worth having, and also more readable for me. Also 100% agreed with #35.

Thinking about this some more, the minimal dataset in #32 and #34 omits the sourceType and expression. I have an idea about how we might do that for static prop sources here. We'll need to retain that for non static props (Dynamic, adapted) but I think that's ok as they are less common.

Did an early peek at @larowlan's in-progress branch — observations:

1. That proxy functionality+architecture is super interesting — looking forward to reviewing that in detail when there's an MR up :)
2. experience_builder.composite_field.*:fields.<key>.field_type won't itself allow referencing a composite field (type), right? Related: 📌 [PP-1] Support `{type: object, …}` prop shapes that require *multiple* field types Postponed .
3. I have doubts about type: experience_builder.composite_field:.*.third_party.experience_builder. Why not keep that information on type: experience_builder.generated_field_explicit_input_ux? 🤔 The new CompositeField config entity type must by definition not have a reference back to the Component config entity that will use it, which means I don't see any way for being able to validate (aka ensure data integrity). Whereas it'd be trivial to validate if these key-value pairs remained on type: experience_builder.generated_field_explicit_input_ux!
4. CompositeFieldType::isInUse() will only work for content entity types, so it won't work for e.g. an XB ContentTemplate config entity — won't this be problematic if this component (and its associated FieldComposite config entity) happen to only be instantiated in config entities? I see that ::preSave() explicitly relies on ::inUse(), so that seems like a date integrity problem? Or … I bet you're going to address this using a pre_save hook implementation that makes it also respect XB's (atypical!) use of fields in config entities? 🤓

1) yes I hope we can add typed-data integration to easily set/get inputs by prop name on the inputs column in ComponentTreeItem so folks dont need to learn the ins and outs of it being JSON encoded, see comment #30 for the type of DX I'm thinking about
2) my hope is that it will, I've got one eye on that as I'm working on this. The code I touched in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin I hope to be able to refactor further into a service (or perhaps method on the existing matcher service) that takes an array of props rather than a component plugin. Because then we could also use it for object shapes and nest them.
3) if we store this in the source plugin settings, we need to be able to version it still. Either way I'm still not sure about that and the interdependency between the two entities, I may end up moving it back and adopting the versioning logic you have in your MR. I should know today/tomorrow if the current approach will work or not and will pivot as needed.
4) At present this is also looking ahead to object shapes and more generic (structured data) uses of the composite field type where we have a fixed table schema and those changes can't be made if a dedicated field table has been created. I don't think it matters when we're talking about JSON or config mappings. But yes, its very rudimentary at this point and could be expanded to consider the dependencies data if we need it to. The structured data approach would require a separate module for adding these via the UI and would likely live at https://drupal.org/project/composite_field - a namespace we grabbed a while back after discussions amongst Drupal CMS team in relation to a possible requirement for 'field_union' in the long term in Drupal CMS but with a better name. If XB has the config entity and field type, then that module would just be the UI. I've left a few todos in the code to date referencing that end goal.

Today's progress:

• was able to collapse done the values stored in inputs to #32 and #34
• added a new column to ComponentTreeItem to store a reference to the composite field type
• starting on some failing tests
• at this point I can see \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testWithDraftCodeComponent is failing because previously we were relying on prop field definitions being stored in autosave, will see what the options are for that tomorrow

I think its probably a good point to stop and decide if we should keep going with this approach

Remaining tasks:

• Fix any tests
• New validation for the third party settings

Reviewed this 34 files, +2899, -166 diff at a high level.

1. My go-to test for making changes to the Component config entity is unsurprisingly ComponentValidationTest. But unfortunately that currently fails hard 😅
2. ComponentTreeItemListTest nicely illustrates the simplification for the stored data 👍
3. I have concerns about ::applyComponentTreeItemDefaults(), and I'm especially concerned about how it's used by ClientServerConversionTrait::clientModelToInput(). Left a lengthy comment there.
4. 👏 ComponentInputsEvolutionTest is fantastic! But … see the aforementioned comment for why that actually doesn't cover the full gamut of possible input evolutions.

AFAICT there are more remaining tasks:

1. multiple-cardinality support
2. explain the rationale behind the (auto-generated) naming scheme of the new config entities
3. provide an overview of which files are >=90% copy-pasted-and-renamed from field_union 🙏 I bet that >2K of the new lines are coming from field_union?

good point to stop and decide if we should keep going with this approach

I still am optimistic! So, I'd say: start with my main concerns (explained in detail on the MR), because if you can overcome/explain away those, then I think we're on track here 🤓

Will reply in detail shortly
Been thinking about this overnight

I think the third party settings and the component source specific aspects (eg the need to apply defaults in client conversion) make this approach non tenable

The third party settings show that while the schemas are close, they don't align

The composite field work we can copy into a branch in the issue for adding generic object shape support, so it isn't wasted work

The static prop source defaults work can be combined with Wim's versioned config entity and we can add a version column to the field item that will apply to all components not just specific sources. That will give us the consolidation of the inputs column we're chasing here

I'll change course to that today

Kinda relieved to read my concerns about third-party settings are (seemingly) confirmed by you.

The static prop source defaults work can be combined with Wim's versioned config entity and we can add a version column to the field item that will apply to all components not just specific sources. That will give us the consolidation of the inputs column we're chasing here

I read that as "versions for all Component config entities, not just some, and stored as a new component_version field property for content entities and a same-name key-value pair for config-defined component instances" — which is exactly where I thought this was going last week 😄👍

Can't wait to see what you cook up for me by your EOD/my start! 🤓

larowlan → closed merge request !1070

One final issue with the composite_field_type approach is that storing prop field definitions in a separate entity that is only written when the component is saved means we lost the ability to have draft JS code components drawn from autosave.

To enable @larowlan to focus on the actual tricky bits, and to avoid him having to spend time fixing the remaining test failures I left in #23 (8 days ago), I got that out of the way. I also addressed the @todo for adding validation.

All PHPUnit tests are passing, except for one. That one actually reveals a severe flaw in what I did 😇😬

The last failure:

Drupal\Tests\experience_builder\Kernel\Config\JavascriptComponentStorageTest::testComponentEntityCreation
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.js.bxnge13p with the following errors: 0 [versioned_properties.46d5d8c16712f873.settings.prop_field_definitions] 'noodles' is a required key.

… this is because the way Component config entities are currently validated per the current config schema, it is required for all past versions of a Component to have valid source-specific settings.

However … in the case of code components, it's possible that new props/slots are added! 😅 (More to come on that front in 🌱 Plan to allow editing props and slots for exposed code components Active .) Which then fails due to

…
    prop_field_definitions:
      constraints:
        # There must be a key for every `prop` in the corresponding \Drupal\experience_builder\Entity\JavaScriptComponent::$props.
        SequenceKeysMustMatch:
          configPrefix: experience_builder.js_component.
          configName: '%parent.%parent.%parent.%parent.source_local_id'
          propertyPathToSequence: props

And that is the big flaw: that actually only the active version MUST be valid, older versions MAY be valid. It's impossible to perfectly validate them, given the old implementation is no longer around. Will try to push a solution for that, too.

Will try to push a solution for that, too.

Done:

(I'm aware of the clean-up potential. I'll let @larowlan refactor from this — AFAICT — working state to his liking 😊)

Thanks!

@larowlan I don't understand why this commit brought back the ComponentIdParts constraint, the ::setSource() method etc. — could you explain? 🙏

Reduced PHPUnit CI job from 5 failures, 8 errors to 2 failures, 1 error.

But … that's misleadingly low: FallbackInputTest passes, but triggers some worrying PHP warnings, that I believe is related to #52. Which also leads to an informed guess about the answer to the Q I asked in #52: because of how A) VersionedConfigEntityBase::createVersion() always uses active as the identifier for the active version, B) type: experience_builder.component.versioned.active. The combination of those two means that only the active version has its source-specific settings validated (for IMHO good reasons, to solve #49), but that then of course fails when the component source is changed to fallback! 😅

That means the comment I added is wrong:

# Special case: the `active` version: it uses a non-Fallback source plugin.
# All other cases: key-value pairs under `versioned_properties` MUST contain both:
# 1. the Component Source-specific settings (which MUST be *valid* against the current implementation of the component)
# 2. the fallback metadata
# (because both may vary from one version to the next)

… because as of that commit, the active version might point to the Fallback source 🙈 IOW, that commit made this dead config schema:

# Special case: the `fallback` version: it uses the Fallback source plugin.
# @see \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\Fallback
experience_builder.component.versioned.fallback:
  constraints:
    FullyValidatable: ~
  type: mapping
  mapping:
    settings:
      type: experience_builder.component_source_settings.fallback
    # Note: NO `fallback_metadata`!

So my solution for #49 was incorrect or at best incomplete. I'm hopeful I got it right this time 🤞

D'oh, I missed @larowlan's comment that would've explained #52 🙈

Doing a bit more work on this before passing the baton to @larowlan.

Linking related core issues. PHPUnit tests are green.

Back to @larowlan for:

• getting end-to-end tests to green, too
• reviewing my changes
• addressing questions/thoughts I posted — including a suspected race condition, a suspected bug in the collapsing logic, whether the hashing should be actually made deterministic in a beta-blocking follow-up, and a bunch of smaller things

… so we can hopefully land this MR soonish 🤞

I think this is ready, there's a couple of open threads on the MR for creating followups, one for core, one for additional validation.

'fallback' is a version, not a plugin is genius! 👏

The one significant change in the validation logic I really didn't understand led me down a pretty deep rabbit hole, but I managed to crawl out 🐰

At the very end, I also scrutinized ComponentInputsEvolutionTest, because that's what proves:

• multiple versions of Component config entities can exist
• instances of those multiple versions can exist and continue to work
• the appropriate versions are created

Clarifying that test coverage happened in the last batch of commits. (And the expanded test coverage revealed a few small bugs 👍)

Follow-ups:

• Extracted critical beta-blocking follow-up issue for deterministic hashing: 📌 [PP-1] Ensure deterministic version hashes Active . This MR is already 79 files, +2578, -716 — that follow-up won't change the data storage, but will ensure integrity. (This is also what I tagged this issue for in #23.)
• Expand ComponentInputsEvolutionTest with "the other side of the coin", already happening in [#3501708.
• I contemplated renaming the field property from version to component_version, which would make it match component_id nicely. But perhaps the better way forward is to rename component_id to just component. Extracted a bikeshed issue: 📌 Either rename `component_id` field property to `component`, or `version` to `component_version` Active .
• (Low priority) Take advantage of this new infra to reduce data-on-the-wire: 📌 [PP-1] Reduce the verbosity of the client-side model for static prop sources Postponed
• Hopefully bring "versioned config entities" to core: 📌 Consider adding a VersionedConfigEntityBase Active .

I thought the issue summary actually was fairly up-to-date, so I removed that tag. But the issue title does not nearly adequately capture the scope. Trying to improve that prior to merging 🤓

Opened https://git.drupalcode.org/project/experience_builder/-/merge_requests/1095 for some more simplification and added robustness in ComponentTreeStructureValidator

(Low priority) Take advantage of this new infra to reduce data-on-the-wire: #3528043: [PP-1] Reduce the verbosity of the client-side model for static prop sources

I agree this can be lower priority but I think we need to at least audit all places in the frontend that are looking at the component list (drawn from the component API in Drupal) to get field data (things like json schema etc) and confirm they are compatible with versions.

I have a sneaking suspicion that we might need to special case editing legacy versions - opened 📌 Add e2e tests that prove we can edit an old version of a component Active to address that, so one more followup

Thanks for that follow-up MR — very nice to see ComponentTreeStructureConstraintValidator become simpler 👍

I opened 🐛 Deterministic Component version hash should depend not only on source-specific settings, but also slots + explicit input schema Active yesterday, that was an oversight here (and kinda in 📌 [PP-1] Ensure deterministic version hashes Active ).

@larowlan identified another oversight: 🐛 \Drupal\experience_builder\Plugin\DataType\ComponentInputs::getPropSources needs to take \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::rawInputValueToPropSourceArray into account Active .

I think I spotted one more gotcha: the \Drupal\experience_builder\Audit\ComponentTreeDependencyRepository logic + DB table looks at the dependencies of the Component config entity and stores those calculated dependencies in the xb_component_tree_dependencies table.
As of this issue, it should be calculating and storing the dependencies of the referenced version of the Component config entity used in the component instance — which could be an older version! Fortunately, @larowlan got that right here:
\Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::calculateFieldItemValueDependencies() does:

public function calculateFieldItemValueDependencies(bool $with_plugin_dependencies, ?FieldableEntityInterface $host_entity = NULL): array {
…
    $component = $this->getComponent();
  

(unchanged here!)

which combined with

  public function getComponent(): ?ComponentInterface {
    return $this->get('component')->getTarget()?->getValue();
  }

and

    $properties['component'] = DataReferenceDefinition::create(ConfigEntityVersionAdapter::PLUGIN_ID)

means that the correct version is indeed used 👍

The oversight: we didn't expand test coverage to explicitly test this.

The oversight: we didn't expand test coverage to explicitly test this.

Being fixed at #3528499-20: Revisit storage of dependencies in separate table now we have separate deltas per component → .

Missing test coverage landed: #3528499-22: Revisit storage of dependencies in separate table now we have separate deltas per component → .