Overview

The explicit inputs for a component instance in a config-defined component tree currently looks like this:

diff --git a/config/schema/canvas.schema.yml b/config/schema/canvas.schema.yml
index 4c9d7514b..df8c05b86 100644
--- a/config/schema/canvas.schema.yml
+++ b/config/schema/canvas.schema.yml
@@ -360,7 +360,7 @@ canvas.component_tree_node:
       constraints: { }
     # @see \Drupal\canvas\Plugin\DataType\ComponentInputs
     inputs:
-      type: ignore
+      type: component_instance_inputs
       label: 'Input values for each component in the component tree'
     label:
       # This is entirely optional; provides more context for content authors.

That has been fine, because the ValidComponentTreeItem and ComponentTreeMeetRequirements validation constraints automatically turn the given config blobs into a ComponentTreeItem (the field type) and then use its validation constraints.

This is how Canvas is able to store component trees in both content and config entities with perfect consistency: because the same validation infrastructure is used for both.

Config schema must become more precise to support translations

However, translating config-defined component trees means using Drupal core's LanguageConfigOverride, config_translation and related infrastructure (see #3582464-23: [PP-2] Symmetrically translatable config-defined component trees, STEP 3: adjust sequence keys to avoid translations breaking when component instances are moved). All that infrastructure gets its information from … config schema.

⇒ config schema must become more precise.

💡 This should also make the "JSON-blobs-in-config-YAML" disappear, i.e.:

component_tree:
  - 
    uuid: fd141b6e-c57a-4355-9885-a8b2e60627d6
    component_id: sdc.canvas_test_sdc.heading
    component_version: 8c01a2bdb897a810
    inputs: '{"text":{"sourceType":"entity-field","expression":"\u2139\ufe0e\u241centity:node:page\u241dtitle\u241e\u241fvalue"},"style":"primary","element":"h1"}'

would become

component_tree:
  -
    uuid: fd141b6e-c57a-4355-9885-a8b2e60627d6
    component_id: sdc.canvas_test_sdc.heading
    component_version: 8c01a2bdb897a810
    inputs:
      text: {"sourceType":"entity-field","expression":"\u2139\ufe0e\u241centity:node:page\u241dtitle\u241e\u241fvalue"}
      style: primary
      element: h1

EDIT:

Proposed resolution

  1. ✅ Change from type: ignore to type: mapping with an explicitly specified class ComponentSpecificInputs that subclasses core's \Drupal\Core\Config\Schema\Mapping to auto-generate at runtime the correct mapping definition: one that corresponds to the explicit inputs the referenced Component config entity (version) supports.
    (Prior art for this: \Drupal\canvas\Config\Schema\JsonSchemaObject, for validating code components' definition against a JSON Schema subset.)
  2. ✅ Introduce ComponentTreeConfigEntityBase:: componentTreeInstancesInputsMustBeArrays() and use it in ::set('component_tree', …) + ::preCreate() to convert the JSON blob assigned to a component instance's inputs (explicit inputs) to an array of key-value pairs (with the keys being the explicit inputs for which values are being specified).
  3. ✅ Provide an explicit API for this: introduce \Drupal\canvas\Attribute\ComponentSource::__construct(inputs_config_schema_generator) which requires a ComponentInstanceInputsConfigSchemaGeneratorInterface.
    (Making the pre-existing ComponentSourceBase::getExplicitInputDefinitions() public is insufficient; we'd need to change its return values, which in turn would result in different component version hashes. That's avoidable disruption. This is still not ideal, but that is true for many of the methods on that interface. The point is to first grow it organically to meet all needs, and then distill a sensible API/interface from it. We have #3520484: [META] Production-ready ComponentSource plugins for that. But this was implemented in a way that follows the pre-existing pattern towards a public API: discovery, updater and now inputs_config_schema_generator.)
  4. ✅ Ensure that a config-defined component instance that populates an explicit input using structured data (EntityFieldPropSource, HostEntityUrlPropSource) correctly detects and prevents duplicating or overriding that structured data in a symmetrical translation. This is achieved using the same dynamic config schema: this MR's ComponentSpecificInputs extends Mapping class takes into account the values for the inputs in the default translation: if it's populated by structured data, set translatable: false. Test coverage was added for this.
  5. ✅ Ensure that a config-defined component tree is symmetrically translatable using core's Config Translation UI:
    • For block component instances, reuse all the existing config translation infrastructure+UX that would be used for block config entities
    • For SDC+code component component instances, specify for every input form_element_class: CanvasStaticPropSourceFieldWidget extends FormElementBase to generate a field widget-powered UX, and its submission handling will automatically use the same optimizing ("collapsing") for storage, resulting in correct overrides. → this means any prop shape or any specific prop can be marked as translatable, and it will automatically work.

    Details

  6. ✅ Ensure that the ComponentSpecificInputs extends Mapping config schema type class gets its information from ComponentTreeItem's inputs property. It could be retrieved from $component_tree_item->get('inputs')->getTranslatableInputKeys() but that would be more expensive: config schema construction would ideally not be instantiating field objects like that. Instead, make a public static ComponentInputs::resolveConfigSchemaMapping() available that is used by both classes.
    The result: both config translation (this issue) and content translation (#3583684: [PP-1] Symmetric translations for content entities should allow only translatable properties in inputs to be overridden) can use the same central metadata to determine which of the key-value pairs in inputs for a component instance are actually translatable.

Test coverage

  1. ✅ Kernel test coverage for configuration translation foundations in \Drupal\Tests\canvas\Kernel\Config\ContentTemplateTest::testTranslationLifeCycleInDepth()
  2. ✅ Integration test coverage to prove it yields the expected results when rendering a component tree and visiting as an English and a French site visitor: \Drupal\Tests\canvas\Functional\TranslationTest::testContentTemplateTranslationRendered()
  3. ✅ Integration test coverage to prove it is possible to generate valid config translations (LanguageConfigOverrides) using the UI: TranslationTest::testContentTemplateConfigTranslationUi().
  4. ✅ Update path test coverage, to prove that the config-defined component trees are updated correctly, both when >=1 config-defined component instance's inputs still is a JSON blob (typical scenario) and when it already is an array (atypical, but possible when e.g. using recipes, possible due to type: ignore not applying any transformation at saving time). : ConfigComponentInputsMustBeArraysUpdateTest.

Out of scope

User interface changes

None.

Issue fork canvas-3582478

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

Title: [WIP] Change config schema type for `inputs` to avoid undescribable and hence untranslatable JSON blobs » [WIP] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs

See


    // Verify the translation was stored, in the expected efficient manner: only
    // the translatable subset, instead of duplicating everything.
    // @see \Drupal\language\Config\LanguageConfigFactoryOverride::onConfigSave()
    // @see \Drupal\Core\Config\ConfigFactoryOverrideBase::filterOverride()
    $override = $language_manager->getLanguageConfigOverride('nl', $template->getConfigDependencyName());
    self::assertFalse($override->isNew());
    self::assertSame([
      'component_tree' => [
        '435d1d20-a697-4d36-9892-9d61c825c99c' => [
          'inputs' => [
            'text' => 'Aangedreven door Drupal Canvas',
            // @todo This MUST disappear, too! https://www.drupal.org/project/canvas/issues/3582478 will make it so, because then the config schema will no longer mark the entirety of `inputs` as translatable, but individual key-value pairs.
            'href' => 'https://drupal.org/project/canvas',
          ],
        ],
      ],
    ], $override->getRawData());

in the sibling MR.

wim leers’s picture

Title: [WIP] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs » [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs
Priority: Normal » Major
Issue summary: View changes
wim leers’s picture

Assigned: wim leers » tedbow

Goal: fixing #2.

Working PoC that achieves it: https://git.drupalcode.org/project/canvas/-/merge_requests/868/diffs?com...

I think this is enough for @tedbow to continue experimenting if he has the time on Monday. Otherwise, I'll happily continue on Tuesday 🤓

wim leers’s picture

Assigned: tedbow » wim leers
Issue summary: View changes
Issue tags: +Needs ADR

IS updated now that the overall approach seems to be viable. Still some work left to get to green though.

wim leers’s picture

Discussed with @tedbow; he's +1 given the dynamic schema bit here is not introducing significant complexity — he expected worse :)

To be continued tomorrow, but well on track now. 87 failures/158 errors (this AM) 7 failures/13 errors (now). 🚀

Extracted #3583494: CanvasTestSetup and ContentTemplateOnDependencyRemovalTest should not rely on magic and specify component versions for its test component tree already as blocking technical debt.

wim leers’s picture

Down to the last failure; AFAICT that's solely due to missing update path + test coverage.

wim leers’s picture

Issue summary: View changes

Update IS to to reflect change in direction to make auto-save functionality work with zero changes.

wim leers’s picture

Issue summary: View changes
Related issues:

Point 4 in the issue summary is now also working!

In doing so, finally created #3583854: [upstream] Validate LanguageConfigOverrides targeting Canvas config entities, because the risks for Canvas and oversights upstream in core have now become sufficiently clear.

wim leers’s picture

Issue summary: View changes
wim leers’s picture

wim leers’s picture

Made sure that this does NOT corner us into symmetrical translations only — we will want to add asymmetrical translation support for config entities in the future. Proven: #3583945-2: [later phase] Support asymmetric translation of config-defined component trees.

wim leers’s picture

Issue summary: View changes
Issue tags: -Needs update path tests

Green!

And manually confirmed my hypothesis in the IS: 💡 This should also make the "JSON-blobs-in-config-YAML" disappear.


I think that I was wrong in in #8: no update path is needed for this: this merely refines config schema, reads existing config entities just fine, and just changes how it's represented as YAML at export time:

  • no more JSON blobs
  • reordered according to config schema

That requires no update path, but #3582464: [PP-2] Symmetrically translatable config-defined component trees, STEP 3: adjust sequence keys to avoid translations breaking when component instances are moved will. So let's land this simple subset first, that does NOT require #3582464's significant changes which will require an update path 🤞

wim leers’s picture

Per #14, I had AI extract the subset of MR868 that does not bring over all of #3582464: [PP-2] Symmetrically translatable config-defined component trees, STEP 3: adjust sequence keys to avoid translations breaking when component instances are moved. See MR 898.

(The AI's work is the new oldest commit. And then I fixed the mistakes it made: the second commit. After that, all commits have been replayed.)

From +1554,−473to +851, −36, and NOT needing an update path. 🚀

🤖 The "lift and replay" MR 898 was AI-accelerated.

wim leers’s picture

Title: [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs » [PP-1] [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs
Related issues: +#3584143: Incorrect property paths due to bugs in `BlockComponent::validateComponentInput()` + `ConstraintPropertyPathTranslatorTrait`
tedbow’s picture

re #14

I think that I was wrong in in #8: no update path is needed for this: this merely refines config schema, reads existing config entities just fine, and just changes how it's represented as YAML at export time:

I can test again but in https://git.drupalcode.org/project/canvas/-/merge_requests/885 it actually only worked with ContentTemplate entities I made after switching to the branch. ContentTemplate I made on on 1.x did not work because inputs was a json encoded string when the config_translation expected it to be an array

wim leers’s picture

Issue summary: View changes

#19: That's fine — just this change alone won't make config-defined component trees properly translatable anyway. We need #3582464: [PP-2] Symmetrically translatable config-defined component trees, STEP 3: adjust sequence keys to avoid translations breaking when component instances are moved too. Which is why the test coverage (just updated the IS to point to those) specifically points to #3582464 in a number of places :)

wim leers’s picture

Title: [PP-1] [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs » [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs
Assigned: wim leers » tedbow
Status: Active » Needs review
Issue tags: -Needs ADR

ADR up: https://git.drupalcode.org/project/canvas/-/merge_requests/901. #10, because #9 is not yet claimed but should be: #3514033-26: Document high-level architecture decisions for extensions API 😅

ADR MR needs review. I'm not 100% happy with it, but there's so many things to consider that writing this overwhelming — especially in balancing that with the need for not going into full detail in an ADR 😬

The actual MR still needs to be rebased, one test failure fixed.

wim leers’s picture

Rebased !898 on top of #3584143: Incorrect property paths due to bugs in `BlockComponent::validateComponentInput()` + `ConstraintPropertyPathTranslatorTrait`. 5 fewer files changed, and from +851, −36 to +827,−22.

DO NOT YET review !898, only the ADR MR !901 🙏 To simplify reviewing, look at the rendered result: https://git.drupalcode.org/issue/canvas-3582478/-/blob/3582478-ADR-confi...

wim leers’s picture

@lauriii and @phenaproxima provided valuable feedback on the ADR MR. Addressed that.

Shifts:

  1. For SDCs and code components, this now reuses the same field widget infrastructure when translating, which is why @lauriii's request to also allow translating type: string, format: uri-shaped props became trivial to implement 🥳
  2. This in turn made #3584178: [later phase] Add alter hook for marking additional SDC/code component prop shapes AND/OR specific props as translatable trivial!
  3. Descoped TMGMT. TMGMT's philosophy ("extract strings") is kind of at odds with how config translation works, is definitely very hard to reconcile with how Canvas uses field types to populate SDCs+code components and is kind of in conflict with what @lauriii just requested: it specifically disallows translating URIs on link fields. Details on MR. @lauriii approved descoping TMGMT for now on the ADR MR.
  4. I don't yet see how we would generate a translation form for block component instances 😬🫠 If we'd use TMGMT, that'd be simpler.
    EDIT: see #27, where I second guess we even need this 😇
wim leers’s picture

My current hunch on how to best incrementally ship valuable symmetric translation features without cornering us:

  1. symmetric translations for config-defined component trees (templates, regions), using the Config Translation UI → first this issue, then #3582464: [PP-2] Symmetrically translatable config-defined component trees, STEP 3: adjust sequence keys to avoid translations breaking when component instances are moved. Do not advertise loudly to the public.
  2. figure out how to use TMGMT instead (which reuses form_element_class of the Config Translation UI when translating config — so step 1 is not wasted). See #25.3's "details on MR" link how that could possibly work. This would allow translating SDC+code+block for config entities, and SDC+code for content entities. Advertise loudly.
  3. Make block component instances in content entities translatable via TMGMT.

@lauriii is +1 to this sequence if it indeed doesn't corner us.

@tedbow, WDYT? You've been investigating #3583684: [PP-1] Symmetric translations for content entities should allow only translatable properties in inputs to be overridden so you'll be able to shine a light on this from the content entity angle? 🤞

wim leers’s picture

Assigned: tedbow » lauriii

Now that there is a plan to actually review, I'm trying to assess whether truly all these things are necessary.

Doing so, I realized that symmetrical translations for block component instances may be almost pointless because:

  1. pretty much only the (currently unused+hidden in Canvas!) label setting for every block plugin will be translatable: https://git.drupalcode.org/project/canvas/-/merge_requests/901/diffs#not...

    Includes screenshots to illustrate the common case.

  2. Block plugins are all about executing logic, and offering the content author some useful dials and knobs to control that logic. N results, the Zth menu level, a TRUE/FALSE decision … those are the typical settings for block plugins.

WDYT @lauriii? This would eliminate #25.4 and #26.3 and simplify #26.1+2.

lauriii’s picture

Assigned: lauriii » Unassigned

Responded on MR

wim leers’s picture

Assigned: Unassigned » tedbow
Issue summary: View changes

Issue summary updated per #25–#28, to reflect that:

  • config translation now fully works, is tested, and supports more than only plain+HTML strings.
  • TMGMT is out of scope here

Back to @tedbow per #23.

wim leers’s picture

Title: [exploration] Symmetrically translatable config-defined component trees, part 2: change config schema type for `inputs` to avoid indescribable and hence untranslatable JSON blobs » [exploration] Symmetrically translatable config-defined component trees, STEP 1: change config schema type for `inputs` to allow translating via core's Config Translation UI
Issue summary: View changes

I think there's 3 things left here:

  1. Refactor ComponentSourceInterface::getExplicitInputDefinitionsAsConfigSchemaMapping() (point 3 in the proposed resolution) to something less arcane per @phenaproxima's feedback. ← in progress
  2. One missing goal: added as point 6 to the proposed resolution.
  3. Make the one failing update path test pass again.
wim leers’s picture

ADR MR updated:

wim leers’s picture

Assigned: tedbow » wim leers
Issue summary: View changes
Issue tags: +AI-accelerated

Fixed #30.1 in the 8 commits I just pushed, all dated April 17 — basically: "introduce ComponentInstanceInputsConfigSchemaGeneratorInterface, similar to the updater and discovery handlers component source plugins specify".

I used AI to accelerate it. Docs have been updated.

ADR MR has been updated too.


Monday:

  1. Make it possible for a translation to populate an optional translatable input that is not populated in the default translation — @lauriii confirmed we want that
  2. #30.2
  3. #30.3
wim leers’s picture

#30.2/item 6 in the proposed resolution is now done!

Implemented that as far as possible, but a single class to power both ComponentTreeItem::propertyDefinitions([inputs]) and canvas.component_tree_node:inputs turned out to be impossible 😬 The attached patch gets as far as possible, and it mostly works, but it eventually fails on \Drupal\canvas\Plugin\DataType\ComponentInputs::getValue() returning a JSON string rather than a mapping/an associative array. That in turn is necessary only because of a missing core feature that's been around for >12 years 😅

wim leers’s picture

Assigned: wim leers » tedbow
Issue summary: View changes
  1. Test coverage #30.2/item 6 in the proposed resolution is now done: tests now prove that regardless of where a component instance is defined (in a content or config entity), for the same stored inputs, the same set of translatable input keys is returned. Also added explicit test coverage
  2. ADR MR updated. I think this is ready to go now.

Proposed next steps

  1. Land this issue's ADR MR. It got positive feedback from @lauriii and @phenaproxima >1 week ago. @phenaproxima's comments have been addressed. The overall ADR is much more thorough now (and lists fewer implementation details that previously made this ADR less timeless).
  2. Extract a new blocking issue/MR from this one, specifically focused on the "provide translatability metadata" aspect. So: ComponentInstanceInputsConfigSchemaGeneratorInterface + ComponentInputs::getTranslatableInputKeys() + \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\ComponentSourceTestBase::testGetTranslatableInputKeys.
    This should cut the size of this issue's MR (now sizable, at ~2K LoC!) MR by 30–50%.
  3. Make this issue's implementation MR RTBC-worthy: #30.3 + self-review + clean up this MR (the many shifts along the way (plus the use of LLMs) have left parts of this MR in a confusing state).
  4. Then land this issue's implementation MR, which thanks to the blocking MR can focus on "make config translation work end-to-end".

  • wim leers committed 553e4774 on 1.x
    feat(Data model): #3582478 ADR #10: Symmetrical translations of config-...
wim leers’s picture

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

ADR MR in, next up: #34.2.

wim leers’s picture

Title: [exploration] Symmetrically translatable config-defined component trees, STEP 1: change config schema type for `inputs` to allow translating via core's Config Translation UI » [PP-1] Symmetrically translatable config-defined component trees, STEP 2: change config schema type for `inputs` to allow translating via core's Config Translation UI
Status: Needs work » Postponed
Related issues: +#3586342: Symmetrically translatable component trees, STEP 1: introduce `ComponentInstanceInputsConfigSchemaGeneratorInterface`
wim leers’s picture

Title: [PP-1] Symmetrically translatable config-defined component trees, STEP 2: change config schema type for `inputs` to allow translating via core's Config Translation UI » Symmetrically translatable config-defined component trees, STEP 2: change config schema type for `inputs` to allow translating via core's Config Translation UI
Status: Postponed » Needs work

Blocker in!

EDIT: after merging in what landed upstream, this is now down to 29 files, +1329, −28, a 35% reduction 👍

wim leers’s picture

Issue summary: View changes

Explicit update path + tests added: ConfigComponentInputsMustBeArraysUpdateTest. Tests 2 scenarios:

  1. when >=1 config-defined component instance's inputs still is a JSON blob (typical scenario)
  2. when it already is an array (atypical, but possible when e.g. using recipes, possible due to type: ignore not applying any transformation at saving time).
wim leers’s picture

Issue summary: View changes

Simplify IS: 2 pieces of proposed test coverage in the IS actually were lifted out of here and landed as part of #3586342: Symmetrically translatable component trees, STEP 1: introduce `ComponentInstanceInputsConfigSchemaGeneratorInterface`.