Overview
Discovered in #3493943: SDC+JS Component Sources: split default values into `resolved` and `source`
We currently store model values like so
'href' => [
'sourceType' => 'static:field_item:link',
'sourceTypeSettings' => [
'instance' => [
'title' => 0,
],
],
'value' => ['uri' => 'https://drupal.org'],
'expression' => 'ℹ︎link␟uri',
]
But after data has been stored, prop shape matching can be modified by installing new modules/field types.
And the representation of the data in the component config entity can change.
Steps to reproduce
- Install a test site using XBTestSetp - eg
php ./core/scripts/test-site.php install --setup-file "/data/app/modules/experience_builder/tests/src/TestSite/XBTestSetup.php" --install-profile "nightwatch_testing" --base-url http://nginx:8080 --db-url mysql://local:local@nginx/local --json - Inspect the my-hero component shapes
drush cget experience_builder.component.sdc.experience_builder.my-heroand note that the href is stored as a link
cta1href: field_type: link field_storage_settings: { } field_instance_settings: title: 0 field_widget: link_default default_value: uri: 'https://example.com' options: { } expression: ℹ︎link␟uri - Edit node 1 (created by XBTestSetup) in XB and note that the href value doesn't get updated
- Add a new Hero to that page and note that the URL is editable
- Enable the xb_test_storage_prop_shape_alter module and re-inspect the config entity
cta1href: field_type: uri field_storage_settings: { } field_instance_settings: { } field_widget: uri default_value: value: 'https://example.com' expression: ℹ︎uri␟value- note that it has now changed to a URI consistent with the stored data
- Revisit node 1 in XB and note you can now edit the link href of original Hero but not that of the new one
This happens because we store the transforms in the component list and these are drawn from the config entity.
But the data shape stored for a given component depends on the sourceType at the time it was created. This is important because <the sourceType can change the data stored. For example in this case a link item has multiple columns so is stored as ['uri' => 'https://example.com'] whilst a uri item only has a single value and is stored as 'https://example.com'. This means we have to respect the stored sourceType because the value matches it.
In the frontend this manifests as having the wrong transforms. The link item has a specific Link transform whilst the uri only uses the MainPropertyName transform. Because we're looking at the current component as the source of transforms, we're getting the wrong one depending on when the data was stored.
Proposed resolution
Instead of keeping transforms in the component list under the field data for each prop, send a list of transforms per source type in drupalSettings or an API and use the sourceType (which we have available) in the clientside code to find the transforms to apply based on the stored sourceType rather than the current one
User interface changes
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3515629-17-suggested-direction.patch | 4.7 KB | wim leers |
Issue fork experience_builder-3515629
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
larowlanComment #3
larowlanComment #4
larowlan#3493943: SDC+JS Component Sources: split default values into `resolved` and `source` needs to go in first because without that we always use the sourceType from the config entity
Comment #5
larowlanComment #6
wim leersBut that only dictates the field type + widget to use for new component instances.
All existing component instances do NOT change. That's why all that information (field type + storage settings + instance settings + widget) is self-contained in the
\Drupal\experience_builder\PropSource\StaticPropSourceand stored for every prop of every instance of every SDC-like component.@lauriii once created an issue with a mock-up many months ago, early during the life of XB, to indicate what he thought the UX should be to allow the content creator to "update" to the new default
StaticPropSourcefor an SDC prop.Ahhhh … 🙈 So this is about
And not about
StaticPropSources and what data they contain — it's about those two getting out of sync! IOW: this is a bug we unwittingly introduced in #3499554: Move client-side assumptions about prop form data shape into a series of prop-specific transforms 😬🙈+1 for this being stable-blocking. But shouldn't it be per widget instead of "per
sourceType"?Comment #7
wim leersComment #8
wim leersComment #9
larowlanYes, but we don't store the widget type in the model or send it to the front end.
We do for the sourceType which should map 1:1 to the widget via the prop shape.
Comment #10
larowlanNot really, because before #3493943: SDC+JS Component Sources: split default values into `resolved` and `source` we ignored what was stored in the field and recreated the default static prop each time. So they couldn't get out of sync because we always ignored the stored one.
Now that is in, this is indeed a bug (it was discovered and worked around there).
Regardless, working on it
Comment #11
larowlanComment #12
larowlanI think this goes towards solving #3463996: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade too
Comment #13
wim leers#12: Thanks, #3463996 was indeed the issue I was looking for but could not find! 😄
#9:
But per
\Drupal\experience_builder\PropSource\StaticPropSource::getSourceType(), that's not sufficient, becausesourceTypedoes not track storage + instance settings.Example:
—
\Drupal\experience_builder\JsonSchemaInterpreter\JsonSchemaStringFormat::computeStorablePropShape()Both have the same
sourceType, but with different storage settings. They currently happen to use the same field widget, but that is not necessarily the case.#10: XB prior to #3493943 always using the default static prop source was indeed also wrong, for the reason you say: it always ignored the stored one. #3499554 was great! But it can only work as long as every SDC prop shape forever uses the same field widget (and presumably the same field type + storage settings + instance settings).
That's the point I tried to make in #6, but there's too much noise around it, sorry! 🙈
Comment #15
larowlanInterested in feedback here @longwave @wim leers - couple of icky bits I think we can do better on the PHP side
Comment #16
wim leersDigging deeper into how this works today:
\Drupal\experience_builder\PropSource\StaticPropSource::getWidget()has an optional$field_widget_plugin_idparameter, but we don't use it today except in tests.@todos and most of it was introduced in August 2024 in #3462709: Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed (inComponentPropsFormback then):AFAICT we have two possible choices:
// Build transforms from widget metadata.). The bug you reported here is possible we pass thexb.transformsmetadata out-of-band: we pass it as one bit of metadata for all available XB components (the/xb/api/config/componentresponse). But actually, that API response dictates the current storage choices for prop shapes, not old ones. That's where the mismatch happens.If we'd stop passing that information there, but instead pass that information via
data-attributes ordrupalSettings(or some other mechanism) in\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildConfigurationForm(), then we'd bypass that out-of-syncness.Because:
is clear about what it needs: a URI
which is clear about where that value is stored: the
uriprop of thelinkfield typewhich is also clear about where that value is stored: the
valueprop of theurifield typeComment #17
wim leersWe cross-posted! :D
And you basically implemented something like #16.1. You're concerned about some parts of the MR. I agree with those concerns.
But your changes to
src/Render/MainContent/XBTemplateRenderer.phpprovided me with some inspiration: I think it's good to keep that mechanism, but I think we can avoid relying on global variables to pass the data, by using a new#attachedtype instead.Roughly something like the attached patch.
Comment #18
larowlan#attached is an awesome idea - thanks will do that tomorrow
Comment #19
larowlanhttps://git.drupalcode.org/project/experience_builder/-/jobs/4865800 I think points to issues installing modules which we've seen before so I cherry-picked in the EXTRA_MODULES env variable approach from #3515294: Get `options_buttons` field widget in Page Data form working: support `input[type=radio]`
Comment #20
larowlanComment #21
wim leersNot awaiting a review from @bnjmnm on this one because #3493941: Maintain a per-component set of prop expressions/sources was built primarily by @larowlan and @longwave, and this MR is mostly reorganizing things for clarity, not changing any fundamentals. All end-to-end tests still passing is sufficient evidence.Actually, I'd rather have @bnjmnm +1 this given #3487284-18: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form — it's important multiple people thoroughly understand this 😊
Comment #22
wim leersI believe this reflects the pivot @larowlan implemented since #18 😊
We should update section 3.4 in
docs/redux-integrated-field-widgets.mdto allow new contributors to understand how this work.Comment #23
larowlanUpdated docs
Comment #24
wim leersStill would like @bnjmnm's +1 per #21.
Comment #26
wim leersThanks @bnjmnm for reviewing after I requested that in #24 🙏 😊
@bnjmnm's 2 points of feedback have been addressed. Rather than ask Ben to context switch into this (Ben's currently working on #3520721: Page duplication in navigator results in blank title in editor panel and #3487284: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form) to confirm fairly trivial changes (simplifying code in one file, adding a comment in another), I'm gonna go ahead and merge.
Comment #28
wim leersComment #29
wim leersSee #3484395-32: CKEditor 5 not loading on formatted text Field Widgets in the component instance form — a tiny change here turns out to have been a distracting bug!