- Issue created by @larowlan
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Specifically doing this for only the
sdc
andjs
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
StaticPropSource
s contain all that information, and why that is stored in theinputs
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 thetree
field property, not for theinputs
field property π Updated issue summary based on this interpretation. But I'd like you to confirm it. π - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
From @larowlan's issue summary:
- Adding a new required prop - this is relevant if we go down the non JSON storage approach
- Removing a value from an enum
- Changing the mapped field type
- 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:
- 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.
- 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.
- 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. - 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?
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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.
- π¬π§United Kingdom longwave UK
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.
- π¬π§United Kingdom catch
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@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 fromdeps_config
, and introduce a separatecomponent_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 componentSo 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
DynamicPropSource
s (aka theFieldConfig
entity, the host entity an instance lives in, etc.), then why is it not forStaticPropSource
s?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? π€ - π¬π§United Kingdom longwave UK
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?
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
@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 β€οΈ
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@longwave: yep, see my response to @catch above :)
Getting this going π
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
I like it! Wish I thought of it! π
Consider it done. I'll be spending my AM getting this going! π€πΊ
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Got the foundations ready:
type: versioned_config_entity
, a subtype fortype: config_entity
- an accompanying
\Drupal\experience_builder\Entity\VersionedConfigEntityBase
- moved the
settings.local_source_id
to the top level atsource_local_id
(sibling to HEAD'ssource
) β which makes - moved
settings.*
(aka everything besides the intra-source plugin ID) toversioned_properties.settings.*
β which makes them versionable π - moved
fallback_metadata
(currently only slot definitions) toversioned_properties.fallback_metadata
- thanks to this, there's no more need for changing
source
β we can use the version itself to determine when to use theFallback
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 fromAutoSaveManager::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 convertstrue
to1
andfalse
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. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π€―
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).
- π¬π§United Kingdom catch
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..
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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
andinputs
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. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@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:
- Making the
Component
config entity know about/remember different versions - Updating the stored component instances to also store the version that was active at the time of their creation
- β¦ 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 respectiveComponentSource
plugins
Observations for those 3:
- It's not about updating existing component instances. This is what you're referring to AFAICT.
- 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.
- 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 affectedComponent
config entity. This is why my 3rd "solely about" point is so narrow.
- π¬π§United Kingdom catch
@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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
wim leers β changed the visibility of the branch 3484682-handle-update-and to hidden.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@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 πππ - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 π€ ):
- 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 β¦
- 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 usetype: versioned_config_entity
andVersionedConfigEntityBase
.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
Component
s to use the sameShapeMatchedFieldUnion
. But: how often would that even happen? - 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 givenComponent
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).
- the "to" example lists the
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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
- π¬π§United Kingdom 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). - πΊπΈUnited States effulgentsia
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 thevalue
key for each input, is that correct? For example, for an instance of aheading
component with 3 props, theinputs
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" }
- πΊπΈUnited States effulgentsia
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.
- πΊπΈUnited States effulgentsia
#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": [] } ] }
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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 ?
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
So do we think there is merit in doing a spike on #15 / #35 ?
Yes, please! ππ
- π¬π§United Kingdom catch
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.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Did an early peek at @larowlan's in-progress branch β observations:
- That proxy functionality+architecture is super interesting β looking forward to reviewing that in detail when there's an MR up :)
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 .- I have doubts about
type: experience_builder.composite_field:.*.third_party.experience_builder
. Why not keep that information ontype: experience_builder.generated_field_explicit_input_ux
? π€ The newCompositeField
config entity type must by definition not have a reference back to theComponent
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 ontype: experience_builder.generated_field_explicit_input_ux
! CompositeFieldType::isInUse()
will only work for content entity types, so it won't work for e.g. an XBContentTemplate
config entity β won't this be problematic if this component (and its associatedFieldComposite
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 apre_save
hook implementation that makes it also respect XB's (atypical!) use of fields in config entities? π€
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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. - π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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
- was able to collapse done the values stored in
- Merge request !1070Draft: Resolve #3523841 Store prop definitions in a 'composite field' config entity β (Closed) created by larowlan
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Reviewed this
34 files, +2899, -166
diff at a high level.- My go-to test for making changes to the
Component
config entity is unsurprisinglyComponentValidationTest
. But unfortunately that currently fails hard π ComponentTreeItemListTest
nicely illustrates the simplification for the stored data π- I have concerns about
::applyComponentTreeItemDefaults()
, and I'm especially concerned about how it's used byClientServerConversionTrait::clientModelToInput()
. Left a lengthy comment there. - π
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:
- multiple-cardinality support
- explain the rationale behind the (auto-generated) naming scheme of the new config entities
- 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 fromfield_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 π€
- My go-to test for making changes to the
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Will reply in detail shortly
Been thinking about this overnightI 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
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 newcomponent_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! π€
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 aComponent
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 π)
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@larowlan I don't understand why this commit brought back the
ComponentIdParts
constraint, the::setSource()
method etc. β could you explain? π - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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 usesactive
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 tofallback
! π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 theFallback
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 π€
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.