Deterministic Component version hash should depend not only on source-specific settings, but also slots + explicit input schema

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

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.

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.

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

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

#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!

Didn't get to reviewing this today :/

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

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

🥳

BTW, 📌 Constraint slot names allowed by XB in its component tree Active added a todo pointing here — this MR should be able to drop those lines.

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.

Thanks! Will review 😊

AFAICT this also blocks #3519878 now, see #3519878-22: [PP-1] 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 🐛 SdcPropKeysConstraintValidator::validate() should complain about extraneous keys too, not just missing keys Active , 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.

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