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
- ✅ Change from
type: ignoretotype: mappingwith an explicitly specified classComponentSpecificInputsthat subclasses core's\Drupal\Core\Config\Schema\Mappingto 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.) - ✅ Introduce
ComponentTreeConfigEntityBase:: componentTreeInstancesInputsMustBeArrays()and use it in::set('component_tree', …)+::preCreate()to convert the JSON blob assigned to a component instance'sinputs(explicit inputs) to an array of key-value pairs (with the keys being the explicit inputs for which values are being specified). - ✅ Provide an explicit API for this: introduce
\Drupal\canvas\Attribute\ComponentSource::__construct(inputs_config_schema_generator)which requires aComponentInstanceInputsConfigSchemaGeneratorInterface.
(Making the pre-existingComponentSourceBase::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,updaterand nowinputs_config_schema_generator.) - ✅ 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'sComponentSpecificInputs extends Mappingclass takes into account the values for theinputsin the default translation: if it's populated by structured data, settranslatable: false. Test coverage was added for this. - ✅ 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
blockconfig entities - For SDC+code component component instances, specify for every input
form_element_class: CanvasStaticPropSourceFieldWidget extends FormElementBaseto 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
- For block component instances, reuse all the existing config translation infrastructure+UX that would be used for
- ✅ Ensure that the
ComponentSpecificInputs extends Mappingconfig schema type class gets its information fromComponentTreeItem'sinputsproperty. 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 apublic 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 ininputsfor a component instance are actually translatable.
Test coverage
- ✅ Kernel test coverage for configuration translation foundations in
\Drupal\Tests\canvas\Kernel\Config\ContentTemplateTest::testTranslationLifeCycleInDepth() - ✅ 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() - ✅ Integration test coverage to prove it is possible to generate valid config translations (
LanguageConfigOverrides) using the UI:TranslationTest::testContentTemplateConfigTranslationUi(). - ✅ Update path test coverage, to prove that the config-defined component trees are updated correctly, both when >=1 config-defined component instance's
inputsstill is a JSON blob (typical scenario) and when it already is an array (atypical, but possible when e.g. using recipes, possible due totype: ignorenot applying any transformation at saving time). :ConfigComponentInputsMustBeArraysUpdateTest.
Out of scope
- validation of config translations. Core's
LanguageConfigOverridedoes not seem to have any validation at all. Created #3583854: [upstream] Validate LanguageConfigOverrides targeting Canvas config entities for this. - alter hook to opt in more prop shapes to become translatable: #3584178: [later phase] Add alter hook for marking additional SDC/code component prop shapes AND/OR specific props as translatable
- TMGMT support → #3582558: [PP-2] WIP: Experiment to expose config-defined component instance inputs to TMGMT — precursor to Canvas-native translation UX
User interface changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 3582478-33-single-class-for-field-property-and-config-schema-impossible.patch | 9.28 KB | wim leers |
Issue fork canvas-3582478
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
wim leersSee
in the sibling MR.
Comment #4
wim leersComment #5
wim leersGoal: 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 🤓
Comment #6
wim leersIS updated now that the overall approach seems to be viable. Still some work left to get to green though.
Comment #7
wim leersDiscussed 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.
Comment #8
wim leersDown to the last failure; AFAICT that's solely due to missing update path + test coverage.
Comment #9
wim leersUpdate IS to to reflect change in direction to make auto-save functionality work with zero changes.
Comment #10
wim leersPoint 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.
Comment #11
wim leersComment #12
wim leersAnd now also solved point 3: https://git.drupalcode.org/project/canvas/-/merge_requests/868/diffs?com...
Next up: ADR.
Comment #13
wim leersMade 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.
Comment #14
wim leersGreen!
And manually confirmed my hypothesis in the IS: .
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:
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 🤞
Comment #17
wim leersPer #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.
Comment #18
wim leersPart of the diff reduction was thanks to #3583494: CanvasTestSetup and ContentTemplateOnDependencyRemovalTest should not rely on magic and specify component versions for its test component tree (
+33, −15).Opened #3584143: Incorrect property paths due to bugs in `BlockComponent::validateComponentInput()` + `ConstraintPropertyPathTranslatorTrait` for another out-of-scope piece of technical debt that is blocking this.
Comment #19
tedbowre #14
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
inputswas a json encoded string when theconfig_translationexpected it to be an arrayComment #20
wim leers#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 :)
Comment #21
wim leers🆕 #3584178: [later phase] Add alter hook for marking additional SDC/code component prop shapes AND/OR specific props as translatable
Merged: #3584143: Incorrect property paths due to bugs in `BlockComponent::validateComponentInput()` + `ConstraintPropertyPathTranslatorTrait` — rebasing !898.
Comment #23
wim leersADR 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.
Comment #24
wim leersRebased
!898on top of #3584143: Incorrect property paths due to bugs in `BlockComponent::validateComponentInput()` + `ConstraintPropertyPathTranslatorTrait`. 5 fewer files changed, and from+851, −36to+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...
Comment #25
wim leers@lauriii and @phenaproxima provided valuable feedback on the ADR MR. Addressed that.
Shifts:
type: string, format: uri-shaped props became trivial to implement 🥳EDIT: see #27, where I second guess we even need this 😇
Comment #26
wim leersMy current hunch on how to best incrementally ship valuable symmetric translation features without cornering us:
form_element_classof 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.@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? 🤞
Comment #27
wim leersNow 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:
labelsetting 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.
WDYT @lauriii? This would eliminate #25.4 and #26.3 and simplify #26.1+2.
Comment #28
lauriiiResponded on MR
Comment #29
wim leersIssue summary updated per #25–#28, to reflect that:
Back to @tedbow per #23.
Comment #30
wim leersI think there's 3 things left here:
ComponentSourceInterface::getExplicitInputDefinitionsAsConfigSchemaMapping()(point 3 in the proposed resolution) to something less arcane per @phenaproxima's feedback. ← in progressComment #31
wim leersADR MR updated:
CanvasStaticPropSourceFieldWidgetform_element_classComment #32
wim leersFixed #30.1 in the 8 commits I just pushed, all dated April 17 — basically: "introduce
ComponentInstanceInputsConfigSchemaGeneratorInterface, similar to theupdateranddiscoveryhandlers component source plugins specify".I used AI to accelerate it. Docs have been updated.
ADR MR has been updated too.
Monday:
Comment #33
wim leers#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])andcanvas.component_tree_node:inputsturned 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 😅Comment #34
wim leersProposed next steps
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%.
Comment #36
wim leersADR MR in, next up: #34.2.
Comment #37
wim leersFor #34.2: #3586342: Symmetrically translatable component trees, STEP 1: introduce `ComponentInstanceInputsConfigSchemaGeneratorInterface`.
Comment #38
wim leersBlocker in!
EDIT: after merging in what landed upstream, this is now down to
29 files, +1329, −28, a 35% reduction 👍Comment #39
wim leersExplicit update path + tests added:
ConfigComponentInputsMustBeArraysUpdateTest. Tests 2 scenarios:inputsstill is a JSON blob (typical scenario)type: ignorenot applying any transformation at saving time).Comment #40
wim leersSimplify 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`.