Component transforms need to be per sourceType, not per component prop

Created on 26 March 2025, about 1 month ago

Overview

Discovered in ๐Ÿ“Œ Split model values into resolved and raw Active

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

  1. 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
    
  2. Inspect the my-hero component shapes
    drush cget experience_builder.component.sdc.experience_builder.my-hero
    

    and 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
    
  3. Edit node 1 (created by XBTestSetup) in XB and note that the href value doesn't get updated
  4. Add a new Hero to that page and note that the URL is editable
  5. 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

  6. 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

๐Ÿ› Bug report
Status

Active

Version

0.0

Component

Redux-integrated field widgets

Created by

๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @larowlan
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    ๐Ÿ“Œ Split model values into resolved and raw Active needs to go in first because without that we always use the sourceType from the config entity

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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.

    But 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\StaticPropSource and 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 StaticPropSource for an SDC prop.

    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

    Ahhhh โ€ฆ ๐Ÿ™ˆ So this is about

        "transforms": {
          "text": {
            "mainProperty": [
              
            ]
          }
    

    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 ๐Ÿ“Œ Move clientside assumptions about prop form data shape into a series of prop specific transforms Active ๐Ÿ˜ฌ๐Ÿ™ˆ

    +1 for this being stable-blocking. But shouldn't it be per widget instead of "per sourceType"?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    +1 for this being stable-blocking. But shouldn't it be per widget instead of "per sourceType"?

    Yes, 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.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    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 ๐Ÿ˜ฌ๐Ÿ™ˆ

    Not really, because before ๐Ÿ“Œ Split model values into resolved and raw Active 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

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #12: Thanks, #3463996 was indeed the issue I was looking for but could not find! โ†’ ๐Ÿ˜„

    #9:

    We do for the sourceType which should map 1:1 to the widget via the prop shape.

    But per \Drupal\experience_builder\PropSource\StaticPropSource::getSourceType(), that's not sufficient, because sourceType does not track storage + instance settings.

    Example:

          // @see \Drupal\datetime\Plugin\Field\FieldType\DateTimeItem
          static::DATE_TIME => new StorablePropShape(shape: $shape, fieldTypeProp: new FieldTypePropExpression('datetime', 'value'), fieldStorageSettings: ['datetime_type' => DateTimeItem::DATETIME_TYPE_DATETIME], fieldWidget: 'datetime_default'),
          // @see \Drupal\datetime\Plugin\Field\FieldType\DateTimeItem
          static::DATE => new StorablePropShape(shape: $shape, fieldTypeProp: new FieldTypePropExpression('datetime', 'value'), fieldStorageSettings: ['datetime_type' => DateTimeItem::DATETIME_TYPE_DATE], fieldWidget: 'datetime_default'),
    

    โ€” \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! ๐Ÿ™ˆ

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Interested in feedback here @longwave @wim leers - couple of icky bits I think we can do better on the PHP side

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Digging deeper into how this works today:

    • \Drupal\experience_builder\PropSource\StaticPropSource::getWidget() has an optional $field_widget_plugin_id parameter, but we don't use it today except in tests.
    • The logic that determines the field widget is littered with @todos and most of it was introduced in August 2024 in ๐Ÿ“Œ Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed Fixed (in ComponentPropsForm back then):
            // 1. If the given static prop source matches the *current* field type
            // configuration, use the configured widget.
            // 2. Worst case: fall back to the default widget for this field type.
            // @todo Implement 2. in https://www.drupal.org/project/experience_builder/issues/3463996
            $field_widget_plugin_id = NULL;
            if ($source->getSourceType() === 'static:field_item:' . $prop_field_definitions[$sdc_prop_name]['field_type']) {
              $field_widget_plugin_id = $prop_field_definitions[$sdc_prop_name]['field_widget'];
            }
            assert(isset($component_schema['properties'][$sdc_prop_name]['title']));
            $label = $component_schema['properties'][$sdc_prop_name]['title'];
            $is_required = isset($component_schema['required']) && in_array($sdc_prop_name, $component_schema['required'], TRUE);
            $form[$sdc_prop_name] = $source->formTemporaryRemoveThisExclamationExclamationExclamation($field_widget_plugin_id, $sdc_prop_name, $label, $is_required, $entity, $form, $form_state);
      
    • AFAICT we have two possible choices:

    1. The client-side transforms really are for widget (hence // Build transforms from widget metadata.). The bug you reported here is possible we pass the xb.transforms metadata out-of-band: we pass it as one bit of metadata for all available XB components (the /xb/api/config/component response). 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 or drupalSettings (or some other mechanism) in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildConfigurationForm(), then we'd bypass that out-of-syncness.

    2. Alternatively, we basically solve ๐Ÿ“Œ [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed , but without supporting adapters. (Adapters make it hard. But nothing can use adapters today โ€” it's only used internally in tests to prove it is possible.)

      Because:

      •     cta1href:
              type: string
              format: uri
              title: CTA 1 link
              examples:
                - https://example.com
        

        is clear about what it needs: a URI

      • The initial storable prop shape for it is:
          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
         

        which is clear about where that value is stored: the uri prop of the link field type

      • Similarly, the updated/new storable prop shape for it is:
          cta1href:
              field_type: uri
              field_storage_settings: {  }
              field_instance_settings: {  }
              field_widget: uri
              default_value:
                value: 'https://example.com'
              expression: โ„น๏ธŽuriโŸvalue
        

        which is also clear about where that value is stored: the value prop of the uri field type

      • Which means that I think we can basically do the following upon editing:
        diff --git a/src/Plugin/ExperienceBuilder/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php b/src/Plugin/ExperienceBuilder/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
        index a52658f62..3aaae6dfb 100644
        --- a/src/Plugin/ExperienceBuilder/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
        +++ b/src/Plugin/ExperienceBuilder/ComponentSource/GeneratedFieldExplicitInputUxComponentSourceBase.php
        @@ -401,6 +401,13 @@ abstract class GeneratedFieldExplicitInputUxComponentSourceBase extends Componen
                 $disabled = !$source instanceof UrlPreviewPropSource;
                 $source = $this->getDefaultStaticPropSource($sdc_prop_name)->withValue($prop_source_array['value'] ?? NULL);
               }
        +      else {
        +        // Automatically update from the old recommended static prop source (as
        +        // defined in `$component->getSettings()['prop_field_definitions']` at
        +        // the time of creating this component instance) to the new one (as
        +        // currently in `$component->getSettings()['prop_field_definitions']`).
        +        $source = $this->getDefaultStaticPropSource($sdc_prop_name)->withValue($prop_source_array['value'] ?? NULL);
        +      }
               // 1. If the given static prop source matches the *current* field type
               // configuration, use the configured widget.
               // 2. Worst case: fall back to the default widget for this field type.
        
  • Pipeline finished with Failed
    28 days ago
    Total: 1567s
    #464112
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    We 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.php provided 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 #attached type instead.

    Roughly something like the attached patch.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    #attached is an awesome idea - thanks will do that tomorrow

  • Pipeline finished with Canceled
    27 days ago
    Total: 70s
    #464992
  • Pipeline finished with Failed
    27 days ago
    Total: 971s
    #465003
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    https://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 ๐Ÿ“Œ Get Options as buttons in Page Data form working Active

  • Pipeline finished with Success
    27 days ago
    Total: 1484s
    #465015
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I believe this reflects the pivot @larowlan implemented since #18 ๐Ÿ˜Š

    We should update section 3.4 in docs/redux-integrated-field-widgets.md to allow new contributors to understand how this work.

  • Pipeline finished with Success
    24 days ago
    Total: 1784s
    #467586
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Updated docs

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Still would like @bnjmnm's +1 per #21.

  • Pipeline finished with Success
    21 days ago
    Total: 1425s
    #469728
  • Pipeline finished with Failed
    7 days ago
    Total: 2187s
    #480906
  • Pipeline finished with Failed
    2 days ago
    Total: 648s
    #484346
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Thanks @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 ๐Ÿ› Page duplication in navigator results in blank title in editor panel Active and ๐Ÿ“Œ Discover cases where is no 1:1 map between field, prop and widget values Active ) to confirm fairly trivial changes (simplifying code in one file, adding a comment in another), I'm gonna go ahead and merge.

  • Pipeline finished with Skipped
    2 days ago
    #484586
  • Pipeline finished with Skipped
    2 days ago
    #484588
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
Production build 0.71.5 2024