Overview

As of #3523841: Versioned Component config entities (SDC, JS: prop_field_definitions, block: default_setting, all: slots for fallback) + component instances refer to versions ⇒ less data to store per XB field row, a new Component version is created whenever the ComponentSource-specific settings for that component change.

For SDCs and code components, that's prop_field_definitions:

    $version = ComponentEntity::generateVersionStringForData($settings, 'experience_builder.component_source_settings.sdc');

and

    $version = ComponentEntity::generateVersionStringForData($settings, 'experience_builder.component_source_settings.js');
    $component
      ->createVersion($version)
      ->deleteVersionIfExists(ComponentInterface::FALLBACK_VERSION)
      ->setSettings($settings);

But … shouldn't a new version also be created when the set of slots changes? Otherwise, a change in slots doesn't cause a new version to be created? (Realized this while reviewing #3519891: Constrain slot names allowed by XB in Components (and in its component tree).)

Otherwise, we can end up in a place where the Fallback source (introduced in #3519168: Handle components provided by ComponentSources EXPLICITLY disappearing — enables deleting JS components that are in use) won't be able to know the actual slots available for a particular version of the component: as long as the props of an SDC/code component remain the same, we'd end up overwriting the information in fallback_metadata.slot_definitions for that same version.

Proposed resolution

Update the logic to let the following affect the deterministic hash (actually deterministic since #3528159: Ensure deterministic version hashes for ComponentSource-specific settings, thanks to config schema-powered normalization):

  1. ComponentSource-specific settings
  2. ✅ normalized slots: the result of ComponentSourceWithSlotsInterface::getSlotDefinitions() but without the description and only keeping the first example
  3. normalized explicit input schema:
    • ✅ the result of \Drupal\experience_builder\PropShape\PropShape::normalize() for SDC + code components, plus with the default value (i.e. the first example) ⇒ ⚠️this is strictly speaking not necessary for these component sources thanks to prop_field_definitions being part of the settings
    • ✅ the combination of a normalized config schema for that block plugin , plus the block plugin's ::defaultConfiguration (default values end up in type: experience_builder.component_source_settings.block)

The deterministic nature of the version hash must be validated, by validating the active_version. But older versions should not be validated, for the reasons outlines in #12.

IOW: this is necessary for #3524751: [later phase] Component Source plugins: generalized support for schema changes of explicit inputs to be possible without BC breaks. Now that many pieces exist, it's clear that this part that was descoped at #3520484-44: [META] Production-ready ComponentSource plugins actually is necessary. (This used to be mentioned in the data storage IS until a few weeks ago — see for example @longwave's comment at #3520449-17: [META] Production-ready data storage).

This should be easily manually tested to verify the expected real-world impact from a component developer POV (the example used is the block component source, but this should work equally well for SDC etc.):

  1. Go to /admin/config/development/configuration/single/export and export block.system_menu_block.tools
  2. Modify e.g. level in \Drupal\system\Plugin\Block\SystemMenuBlock::defaultConfiguration(), then rebuild.
  3. Repeat step 1 and observe: a new version, with a new default value in default_settings.
  4. Now go and modify block.settings.system_menu_block:* and add a new optional key-value pair, for example:
    diff --git a/core/modules/system/config/schema/system.schema.yml b/core/modules/system/config/schema/system.schema.yml
    index 0c7cfac313c..d38a75778ff 100644
    --- a/core/modules/system/config/schema/system.schema.yml
    +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -448,6 +448,9 @@ block.settings.system_menu_block:*:
         expand_all_items:
           type: boolean
           label: 'Expand all items'
    +    yar:
    +      type: boolean
    +      requiredKey: false
     
     block.settings.local_tasks_block:
       type: block_settings
    
  5. Repeat step 1 and observe: a new version, with the same default values in default_settings, but … a new version, because config schema has changed.

User interface changes

/admin/appearance/component now shows the number of versions of each component, and shows the active version upon hover.

Command icon 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

wim leers created an issue. See original summary.

wim leers’s picture

Issue summary: View changes
wim leers’s picture

Category: Bug report » Task

Can hardly be a bug if #3528159 has just landed.

larowlan made their first commit to this issue’s fork.

larowlan’s picture

the combination of a normalized config schema for that block plugin, plus the block plugin's ::defaultConfiguration

From BlockManager


$settings = [
        // We are using strict config schema validation, so we need to provide
        // valid default settings for each block.
        'default_settings' => [
            // The generic block plugin settings: all block plugins have at least
            // this.
            // @see `type: block_settings`
            // @see `type: block.settings.*`
            // @todo Simplify when core simplifies `type: block_settings` in
            //   https://www.drupal.org/i/3426278
          'id' => $id,
          'label' => (string) $definition['admin_label'],
          // @todo Change this to FALSE once https://drupal.org/i/2544708 is
          //   fixed.
          'label_display' => '0',
          'provider' => $definition['provider'],
        ] + $block->defaultConfiguration(),// 👈️👈️👈️👈️👈️
      ];

We already include default configuration - does that cover off the schema already?

Can you provide an example scenario where we need item 3?

Will make a start on item 2.

larowlan’s picture

I think this raises an issue if we're generating the hash based on information that isn't stored in the config entity.

Specifically things that aren't available in \Drupal\experience_builder\Entity\Component::validateVersions in order to validate the configuration.

It would require making that rely on actual source plugin instances and consulting them to ask what else should be considered in calculating the expected hash.

Will have a think about how to approach that, I don't know that booting an instance of the source plugin during validation is the correct approach when we're dealing with typed data (specifically config mapping data) and not config entities.

larowlan’s picture

I think if we inject the source manager into the constraint validator we could boot an instance without the config entity, I think that's acceptable, will do that.

This puts us into the Production ready component source plugins meta as much as the data storage one as it might require new methods on the interface

larowlan’s picture

I might have a way to do it with raw config typed data

wim leers’s picture

Assigned: Unassigned » wim leers

#5: block plugins’ default configuration might stay the same, but the validation constraints might become tighter or looser, which would then affect the freedom of component instances. Which then could lead to existing instances failing to render, and/or the need for an update path.

#7: yep, this straddles both. But component source plugins just must work well enough for beta1, the API is not expected to be final by beta1. Data storage must not change after beta1, and version hashes changing would be … bad.

Will review, curious what you came up with!

wim leers’s picture

Assigned: wim leers » larowlan

Didn't get to reviewing this today :/

wim leers’s picture

Status: Active » Needs work
Issue tags: +Needs issue summary update

#6: gave this some more thought.

You’re right that it’s impossible to validate old versions (any besides the active one) if they depend on data not contained within the config entity.

So … I think the solution is actually very simple, plus it would prevent brittleness in the future: only validate active_version!

That way, even if a ComponentSource plugin changes what information is used to deterministically compute the hash, old versions (hashes) continue to work fine. Because in the proposal I made in the issue summary, any such logic change would have caused all old versions (hashes) to become invalid 🙈

And that is actually perfectly in line with the source-specific settings for old (non-active) versions: those already use type: ignore because the config schema for older versions might have been different. What we’re dealing with here is similar.

larowlan’s picture

Re #12 that makes sense.
I think we can include the 'current schema' in the hash calculation and that gives us item 3 from the issue summary

Will pick that up today

wim leers’s picture

🥳

BTW, #3519891: Constrain slot names allowed by XB in Components (and in its component tree) added a todo pointing here — this MR should be able to drop those lines.

larowlan’s picture

Status: Needs work » Needs review

Got something going that uses the schema, but I'm not 100% happy with it.
I think its probably ok, but there's a chicken-egg scenario with creating a version string vs having a component typed data adapter that prevents using being able to use \Drupal\Core\Config\Schema\TypeResolver::resolveDynamicTypeName

The workaround seems ok, but is not ideal.

wim leers’s picture

Assigned: larowlan » wim leers

Thanks! Will review 😊

wim leers’s picture

AFAICT this also blocks #3519878 now, see #3519878-22: ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities. IOW: I think/hope that once this is fixed, the remaining test failures there will disappear.

The test coverage here is definitely sufficient, so removing tag.

P.S.: This removed a few @todos pointing to #3525759: SdcPropKeysConstraintValidator::validate() should complain about extraneous keys too, not just missing keys, because those bits were getting in the way of getting this MR to pass. This is critical for data model stability, so this takes precedence.

wim leers’s picture

Assigned: wim leers » larowlan
Status: Needs review » Needs work

@larowlan mentioned he wasn't happy with one area. I investigated and implemented a counterproposal.

larowlan’s picture

Picking up where Wim left off.

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review

This is passing now

wim leers’s picture

Assigned: Unassigned » wim leers
wim leers’s picture

Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thanks, @larowlan, for pushing this to completion! All I had to do here was fix nits, and add docs for the tricky bits 😊

Issue summary updated.

wim leers’s picture

Issue summary: View changes
StatusFileSize
new218.64 KB

Updated /admin/appearance/component to show the number of versions of each component, and shows the active version upon hover.

  • wim leers committed 1911c983 on 0.x authored by larowlan
    Issue #3528362 by larowlan, wim leers: Deterministic Component version...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.