Symmetric translation

Work has started

This is a beta2 target, there was some hope of doing in for beta1, but we need to follow the flow of #3548884: SourceValueItem field synchronized translations so let's take some more time and target beta2 as initially planned.

Hi Laurii,

What are you planning to use for determining which props should be translated? Asking because it might be good for us to coordinate on this so that we could provide a good experience for people who are using both Canvas and Display Builder.

Our logic (which is not really the current MR, we didn't push yet the current work which is very promising) is not based in props, it is based on our data source plugin type:

• Prop types (what you call "Prop shapes" in Canvas) are guessed from props schema
• Data source plugins are targeting prop types
• Data source plugins have a different data model than the prop shape they are targeting. Sometimes it happens there is a one-to-one mapping between the two models, but it is not the rule.
• Data source plugins data model is using config schema API so can have translatable strings here. This is where translation is managed.

I believe it would be a mistake to think about "translatable props":

• this is not part of JSON schema AFAIK
• this is not part of UI modeling and it is not up to the component authors to care about that
• this is a mechanism related to the CMS application state, so out of SDC

What do you think about that?

Instead of a ComponentTree like Canvas, we have a SourceTree, we nest data sources and each source plugin has its own config schema. So, by nesting the source plugins, we have a big config schema tree and we can extract the translatable strings.

This abstraction is the core of our architecture since the start of UI Patterns 2 development in June 2023 .

Is there a selection that happens at a certain point by someone to determine what gets translated?

Such selection is not necessary.

Since beta2, we have a fake, temporary, implementation of ContentEntityInterface, so of TranslatableInterface, in Drupal\display_builder\Entity\ContentEntityBase

We are still hesitating:

• do we keep this file and fill it with our own custom implementations of the expected interfaces (see also #3562989: Implements RevisionLogInterface for Instance entity) ?
• do we adopt the real \Drupal\Core\Entity\ContentEntityBase as a base class and adapt the custom logic in Instance and InstanceStorage only ?

Anyway, even if we are focusing on tarnslatabilitty of the "providers" (integration with Drupal API) for now, the addition of this intreface may interest you.

Moved to beta4 because we will merge #3549266: Move DisplayBuildableInterface to a new plugin type first.

#3549266: Move DisplayBuildableInterface to a new plugin type has been merged!

Instance entities are now using \Drupal\Core\Entity\ContentEntityBase and are currently adopting the Field API to store the data (see #3562989: Implements RevisionLogInterface for Instance entity).

Once done, we will be able to easily use the work from UI Patterns (#3548884: SourceValueItem field synchronized translations).

I will resquash and do a review.

However, the priority is the UI Patterns ticket for now: #3548884: SourceValueItem field synchronized translations

Squashed, rebased & linted.

We are back to 3555110-symmetric-config-translation-cw and this MR: https://git.drupalcode.org/project/display_builder/-/merge_requests/273

383 additional PHP LOC (tests excluded), that's lean for such a valuable feature 👍

My understanding:

Instance entity

A new method: InstanceInterface::ensureCurrentTranslation(): InstanceInterface, called from 2 places only:

• the new InstanceTranslationSubscriber listening KernelEvents::REQUEST.
• ProfileViewBuilder

As said in a note, I am surprised such mechanism is needed, i was expecting the Entity API to do all the work.

Buildable plugins

A new method: DisplayBuildableInterface::isTranslationSynchronized(): bool

For EntityViewOverride: a very simple implementation, because we use the same field type in Instance entity and in overrides.

For EntityView, PageLayout and ViewDisplay, the logic is more complex and implemented in the new ConfigBuildablePluginBase. This is where most of the logic happen and will be the main part of the review.

Island plugins

A new "Language" col in the logs panel

Do we also display the current language in the Controls panel?

(removing third_party_settings in all examples for clarity)

Page Layout

1. Just translating without messing with the sources tree

Test:

1. I have 2 languages: English (default) and French.
2. Create a new page layout in English for /test-translations page with:
   

   - source_id: component
     source:
       component:
         component_id: ui_suite_daisyui:button
         slots:
           label:
             sources:
             - source_id: textfield
               source:
                 value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1"
               node_id: d96b1621a2e47636
     node_id: a4ee7afd36d20134
   
   - source_id: textfield
     source:
       value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2"
     node_id: 21af59dc27fa5844
   

3. Add a translation: /admin/structure/page-layout/test/translate (by the way, the /admin/structure/page-layout/test/translate/fr/edit modal doesn't show source tree data as a translatable, i guess this is normal)
4. Back to Display Builder, I need to manually add the language code in the URL (⚠️ is it normal?): /fr/admin/structure/page-layout/test/builder
5. I publish the translation with "🇫🇷🥐🍷 1" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1" and "🇫🇷🥐🍷 2" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2", without moving anything around for now

Results:

• I see English language in /test-translation: ✅ OK, with 404, because the page doesn't exist
• I see French language in /test-translation: ❌ KO, 404 not found again, but without the expected display

It may be an issue not related to the current work, which may need its dedicated ticket, so let's try again with /user/* page condition instead of /test-translation.

Results:

• I see English language in /test-translation: ✅ OK
• I see French language in /test-translation: ❌ KO, I see the English translation

Technical analysis

Just a guess, but the error may come from us not following config translations expectations with the addition of a translations property which is too specific and (obviously) not present in the original language:

langcode: en
status: true
id: test
label: 'Test'
conditions: {}
profile: default
sources:
  - source_id: component
    source:
      component:
        component_id: 'ui_suite_daisyui:button'
        slots:
          label:
            sources:
              -
                source_id: textfield
                source:
                  value: '🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1'
                node_id: d96b1621a2e47636
    node_id: a4ee7afd36d20134
  - source_id: textfield
    source:
      value: '🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2'
    node_id: 21af59dc27fa5844

label: 'Test FR'
translations:
  - key: 'd96b1621a2e47636:source.value'
    value: '🇫🇷🥐🍷 1'
  - key: '21af59dc27fa5844:source.value'
    value: '🇫🇷🥐🍷 2'

I guess we don't need to mess with the default config translation storage: just a skeleton tree with only the translated values. So:

label: 'Test FR'
sources:
  - source:
      component:
        slots:
          label:
            sources:
              - source:
                  value: '🇫🇷🥐🍷 1'
  - source:
      value: '🇫🇷🥐🍷 2'

Then, Drupal Core APIs can do the merge deep they usually do at config load to get the full translated display. The ConfigBuildablePluginBase is translating between the UI Patterns source field storage and the "normal" config translation storage, in both direction.

Is it possible ? Will it simplify our logic ?

2. Moving stuff around in the translated language

Later

3. Changing a non translatable value from original language

Later

Entity View, Entity View Overrides, View Display

Later.

Discussed with Christian.

We will create a new branch to start a new proposal following the philosophy which was successful UI Patterns side: we don't mess with Drupal storage

That means config translation storage will be stored the normal way: symmetric and positional. And Display Builder will not intervene when the config is loaded by Drupal in read-only.

Like in UI Patterns with SourceItem and SourceItemList, we will run our logic when we load for display building tasks and when we publish from display builder. So, in the display buildable plugins, i guess :)

Only 186 PHP LOC added, so lean :)

Code review

Instance entity

A new method: InstanceInterface::ensureCurrentTranslation(): InstanceInterface, called from 2 places only:

• the new InstanceTranslationSubscriber listening KernelEvents::REQUEST.
• ProfileViewBuilder

⚠️ As said before, I am surprised such mechanism is needed, i was expecting the Entity API to do all the work. Can we move the logic to an override of the "normal" entity operations (load? save?) and remove the public method?

⚠️ Also, Instance::getBuildablePlugin() is now public, but called only from tests. It would be better to keep it protected.

Buildable plugins

A new abstract ConfigBuildablePluginBase class with the shared translation logic and those additional public methods:

• getConfigName(): string;
• getSourcesConfigKey(): string;

⚠️ Here again, those methods are only called from tests, so no need to expand the API scope. It would be better to keep them protected.

ConfigBuildablePluginBase is extended by EntityView, PageLayout and ViewDisplay.

For EntityViewOverride: a very simple implementation, because we use the same field type in Instance entity and in overrides.

⚠️ DisplayBuildableInterface::isTranslationSynchronized() has a single implementation always returning TRUE and never called outside some tests. It can be removed.

Island plugins

A new "Language" col in the logs panel.

Do we also display the current language in the Controls panel?

Page Layout

Tested with UI Patterns' 3548884-trans-make-sourcevalueitem-v2 branch (so before the modification of Mickael)

1. Just translating without messing with the sources tree

Test:

1. I have 2 languages: English (default) and French.
2. Create a new page layout in English for /test-translations page with:
   

   - source_id: component
     source:
       component:
         component_id: ui_suite_daisyui:button
         slots:
           label:
             sources:
             - source_id: textfield
               source:
                 value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1"
               node_id: d96b1621a2e47636
     node_id: a4ee7afd36d20134
   
   - source_id: textfield
     source:
       value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2"
     node_id: 21af59dc27fa5844
   

3. Add a translation: /admin/structure/page-layout/test/translate (by the way, the /admin/structure/page-layout/test/translate/fr/edit modal doesn't show source tree data as a translatable, i guess this is normal)
4. Back to Display BuilderI need to manually add the language code in the URL (⚠️ is it normal? do we need a translation switcher in the UI?): /fr/admin/structure/page-layout/test/builder
5. I publish the translation with "🇫🇷🥐🍷 1" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1" and "🇫🇷🥐🍷 2" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2", without moving anything around for now

Results:

• I see English language in /test-translations:: ✅ OK, with 404, because the page doesn't exist
• I see French language in /test-translations: ❌ KO, 404 not found again, but without the expected display

Let's try again with an existing page (/user/*) instead:

• I see English language in /user/1: ✅ OK
• I see French language in /user/1 ✅ OK

2. Moving stuff around in the translated language

❌ I go back to https://ui-patterns-2.ddev.site/fr/admin/structure/page-layout/test_tran... and I have:

Error: Class "Drupal\ui_patterns\ComponentTree\SourceTree" not found in Drupal\display_builder\ConfigBuildablePluginBase->extractTranslationsMap() (line 144 of modules/custom/display_builder/src/ConfigBuildablePluginBase.php).

So, I renamed ComponentTree to SourceTree.

I go back to https://ui-patterns-2.ddev.site/fr/admin/structure/page-layout/test_tran... and I reorder tteh root level, movinf the second source at the first position.

• The textfield has moved at the first position in both languages: ✅ OK
• In English, we are still in English: ✅ OK
• in French, we are still in French: ✅ OK

3. Changing a non translatable value from original language

I change the button component variant in the English translation. Results:

• ⚠️ weirdly, clicking on "Publish" button don't change the UI state, but do the publish server side
• The component variant has changed in both languages: ✅ OK
• In English, we are still in English: ✅ OK
• in French, we are still in Frencht: ✅ OK it still French

Entity View, Entity View Overrides, View Display

Later. Let's address the first round of feedback first.

Updated review.

Code review

Instance entity

A new method: InstanceInterface::ensureCurrentTranslation(): InstanceInterface, called from 2 places only:

• the new InstanceTranslationSubscriber listening KernelEvents::REQUEST.
• ProfileViewBuilder

⚠️ As said before, I am surprised such mechanism is needed, i was expecting the Entity API to do all the work. Can we move the logic to an override of the "normal" entity operations (load? save?) and remove the public method?

Buildable plugins

A new abstract ConfigBuildablePluginBase class with the shared translation logic, extended by EntityView, PageLayout and ViewDisplay. For EntityViewOverride: a very simple implementation, because we use the same field type in Instance entity and in overrides.

All this is OK.

Island plugins

A new "Language" col in the logs panel.

Do we also display the current language in the Control buttons island?

Page Layout

Tested with UI Patterns 2.0.19 this time.

1. Just translating without messing with the sources tree

Test:

1. I have 2 languages: English (default) and French.
2. Create a new page layout in English for /test-translations page with:
   

   - source_id: component
     source:
       component:
         component_id: ui_suite_daisyui:button
         slots:
           label:
             sources:
             - source_id: textfield
               source:
                 value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1"
               node_id: d96b1621a2e47636
     node_id: a4ee7afd36d20134
   
   - source_id: textfield
     source:
       value: "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2"
     node_id: 21af59dc27fa5844
   

3. Add a translation: /admin/structure/page-layout/test/translate (by the way, the /admin/structure/page-layout/test/translate/fr/edit modal doesn't show source tree data as a translatable, i guess this is normal)
4. Back to Display BuilderI need to manually add the language code in the URL (⚠️ is it normal? do we need a translation switcher in the UI?): /fr/admin/structure/page-layout/test/builder
5. I publish the translation with "🇫🇷🥐🍷 1" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 1" and "🇫🇷🥐🍷 2" instead of "🏴󠁧󠁢󠁥󠁮󠁧󠁿🍺🍲 2", without moving anything around for now

Results:

• I see English language in /test-translations:: ✅ OK, with 404, because the page doesn't exist
• I see French language in /test-translations: ❌ KO, 404 not found again, but without the expected display

Let's try again with an existing page (/user/*) instead:

• I see English language in /user/1: ✅ OK
• I see French language in /user/1 ✅ OK

2. Moving stuff around in the translated language

I go back to https://ui-patterns-2.ddev.site/fr/admin/structure/page-layout/test_tran... and I reorder the root level, moving the second source at the first position.

• The textfield has moved at the first position in both languages: ✅
• In English, we are still in English: ✅
• in French, we are still in French: ✅

3. Changing a non translatable value from original language

⚠️ The published state is not shown in LogsPanel. We may need to adapt the calculation of it. We may need #3595491: Move hash to permanent storage.

I change the button component variant in the English translation. Results:

• ⚠️ clicking on "Publish" button don't change the UI state, but do the publish server side
• The component variant has changed in both languages: ✅ OK
• In English, we are still in English: ✅ OK
• in French, we are still in Frencht: ✅ OK it still French

Entity View, Entity View Overrides, View Display

Later. Let's address the first round of feedback first.

Previous feedbacks

⚠️ As said before, I am surprised such mechanism is needed, i was expecting the Entity API to do all the work. Can we move the logic to an override of the "normal" entity operations (load? save?) and remove the public method?

According to Christian, it is needed because it need to be executed in every route of the API, including the HTMX ones.

I see French language in /test-translations: ❌ KO, 404 not found again, but without the expected display

Managing the 404 pages is not in the scope of the translation system.

⚠️ The published state is not shown in LogsPanel. We may need to adapt the calculation of it.

Fixed.

⚠️ clicking on "Publish" button don't change the UI state, but do the publish server side

Fixed (it was the same issue)

Back to Display BuilderI need to manually add the language code in the URL ⚠️ is it normal? do we need a translation switcher in the UI?

It will be a follow-up.

Today feedback

We have automated both:

• the creation of the Instance content entity translation, just by opening the Display Builder UI
• the creation of the targeted config entity translation, just by "publishing" the instance revision

Without the need of manually adding the translation with Config Translation UI.

It is weird but absolutely needed for Entity View Displays config entity because they don't have their own Config Translation UI pages in Core: #2546212: Entity view/form mode formatter/widget settings have no translation UI
Because it was added for this buildable, it is available for all buidables.

Also, DisplayBuildablePluginBase::getTranslationLangcode() must not be a public method.

We have some playwright fails but, good news, it is on the test we have added: multilingual.spec.ts

https://issue.pages.drupalcode.org/-/display_builder-3555110/-/jobs/1161...

The playwright fails have changed a bit: https://project.pages.drupalcode.org/-/display_builder/-/jobs/11658706/a...

I will review again, add documentation and maybe add a little Language Switcher island plugin.

Updated review.

Tested with UI Patterns 2.0.20.

Config storage

They share most of the logic in ConfigBuildablePluginBase, and they have a specificity: Both the creation of the Instance content entity translation (by just opening the Display Builder UI), and the creation of the targeted config entity translation (by just "publishing" the instance revision) are automated, without the need of manually adding the translation with Config Translation UI.

It is weird but absolutely needed for Entity View Displays config entity because they don't have their own Config Translation UI pages in Core: #2546212: Entity view/form mode formatter/widget settings have no translation UI Because it was added for this buildable, it is available for all buidables.

Page Layout

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Entity View

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

View Display

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Content storage

Entity View Overrides

1. Just translating without messing with the sources tree: ❌ clicking on "Publish" button don't change the UI state, but do the publish server side. it is similar than the issue we got we Page Layout which has been fixed recently.
2. Moving stuff around in the translated language: ❌ clicking on "Publish" button don't change the UI state, but do the publish server side.
3. Changing a non translatable value from original language: ✅

Conclusion

We are close to RTBC in m humble opinion. There is currently a phpunit fails but not related to the current work.

TODO before commiting:

• textarea source is not translatable
• EntityViewOverrides:
  • Prevent automatic Instance entity translation creation (because there is no automated content translation creation, we still need to create the translation in /node/{nid]/translations and that's a good thing here): returning a 404 from the display builder UI controller would be enough for now
  • Publishing issues

Can be done in follow-ups:

• Challenging InstanceInterface::ensureCurrentTranslation(): InstanceInterface and the "forcing" of the URL based language detection (see former comment)
• Language Switcher island plugin
• EntityViewOverrides: see translated content field instead of original language in Display Builder UI
• Documentation

I have hope adding Language Switcher island plugin and a few related methods in DisplayBuildableInterface can help us:

• debugging and testing the remaining issues with Entity View Overrides
• clarifying and centralizing our strategy of automating the creation of the Instance content entity translation (just by opening the Display Builder UI) and the creation of the targeted config entity translation (just by "publishing" the instance revision)
• clarifying and centralizing the "forcing" of the URL based language detection

I will give a try.

DONE:

• ✅ textarea source is not translatable
• ✅ Add Language Switcher island plugin
• ✅ EntityViewOverrides: Prevent automatic Instance entity translation creation (because there is no automated content translation creation, we still need to create the translation in /node/{nid]/translations and that's a good thing here): returning a 404 from the display builder UI controller would be enough for now
• ✅ Revert: Add language column in LogPanel. We were showing the default langcode of the revision, not of the current translation

TODO before commiting:

• ❌ EntityViewOverrides: Publishing issues

Fixed! The fix was very hard to guess but so easy to do: https://git.drupalcode.org/project/display_builder/-/merge_requests/311/...

Page Layout

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Entity View

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

View Display

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Entity View Overrides

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Ready for review, IMHO.

Follow-ups proposals:

• [Feature] EntityViewOverrides: see translated content field instead of original language in Display Builder UI
• [Feature] Add language column in LogPanel. We have already tried but not straightforward: all revisions are saved for all languages, default language seems to be the same for all revisions, only some operations (update, ThirdPartyUpdate) are altering translations...
• [Task] Challenge InstanceInterface::ensureCurrentTranslation(): InstanceInterface and the "forcing" of UR- based language detection isnetad of the configurable one from Core

I will rebase before sending to review again

Rebased. And documentation added.

Sure. I did the rebase, it went well, but we reached the PHPMD class complexity limit for ViewDisplay & DisplayBuildablePluginBase. I will have a look.

We reached the PHPMD class complexity limit for ViewDisplay & DisplayBuildablePluginBase.

Will be addressed here: #3621008: Move DisplayBuildable form logic to a dedicated clas

Rebased

Review of the new MR in progress.

Here is the diff for reference: https://git.drupalcode.org/issue/display_builder-3555110/-/compare/35551... with thosechanges:

• ::loadDefaultSources() removed in some buildable plugins, to address: "Publishing from a non-default language writes its config overrides into default config"
• Instance::getHash() not calling Instance::get('hash') anymore to address: "Saving a translation marks the other languages as unpublished"

First feedback:

• (Already in previous MR) Why drupal:language has a dependency in display_builder.info.yml ? Is the module still working without such dependency? Not all websites using Display Builder will want to activate the language module.
• (Introduced in new MR) Instance::getHash() is not calling Instance::get('hash') anymore but calculating "on the fly", so:
  • what is the point to store data to this field if it is never retrieved? Does that mean we have to remove it?
  • Is it not too costly performance wise?

I will do a little functional check soon.

Page Layout:

1. Just translating without messing with the sources tree:
2. Moving stuff around in the translated language:
3. Changing a non translatable value from original language:

Entity View:

1. Just translating without messing with the sources tree:
2. Moving stuff around in the translated language:
3. Changing a non translatable value from original language:

View Display:

1. Just translating without messing with the sources tree:
2. Moving stuff around in the translated language:
3. Changing a non translatable value from original language:

Entity View Overrides

1. Just translating without messing with the sources tree:
2. Moving stuff around in the translated language:
3. Changing a non translatable value from original language:

Page Layout:

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Entity View:

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

View Display:

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

Entity View Overrides

1. Just translating without messing with the sources tree: ✅
2. Moving stuff around in the translated language: ✅
3. Changing a non translatable value from original language: ✅

The only issue I have found is related to UI, so easy to fix. The language switcher button is looking a bit out of place in the toolbar, with a border in darker color not used anywhere else:

Is there a specific reason for that? If not, the change could be as easy as:

/src/Plugin/display_builder/Island/LanguageSwitcher.php
@@ -72,7 +72,6 @@ class LanguageSwitcher extends IslandPluginBase {
-    $trigger['#props']['variant'] = 'neutral';

We have a few little UI inconsistencies like that (just UI, not UX, so small stuff like: color, font, spacing, align...). I will create an issue to address them, but it would be nice to address this specific one here.

Once fixed, you can consider this ticket as RTBC

Follow-ups proposals:

• [Feature] EntityViewOverrides: see translated content field instead of original language in Display Builder UI. It is also the opportunity to ask if we init a content translation from the original language or from the translated config.
• [Feature] Add language column in LogPanel. We have already tried but not straightforward: all revisions are saved for all languages, default language seems to be the same for all revisions, only some operations (update, ThirdPartyUpdate) are altering translations...
• [Task] Challenge InstanceInterface::ensureCurrentTranslation(): InstanceInterface and the "forcing" of UR- based language detection instead of the configurable one from Core