Allow synced Layout override Translations: translating labels and inline blocks

FieldBlock is actually fine here, it's that Layout Builder is passing it the wrong entity.

This is somewhat related to #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

Thanks d.o for installing in a subdir to catch my naïve implementation of getting the translated entity URL.
Also had to fix/expand the OverridesSectionStorageTest.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -172,8 +174,18 @@ public function buildMultiple(array $entities) {
   +    if ($this->isOverridable() && $entity->hasField('layout_builder__layout')) {
   +      if (!$entity->get('layout_builder__layout')->isEmpty()) {
   +        return $entity->get('layout_builder__layout')->getSections();
   +      }
   +
   +      // If this is a translation consult the default translation.
   +      if ($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation()) {
   +        $default_entity = $entity->getTranslation(LanguageInterface::LANGCODE_DEFAULT);
   +        if ($default_entity instanceof FieldableEntityInterface && $default_entity->hasField('layout_builder__layout') && !$default_entity->get('layout_builder__layout')->isEmpty()) {
   +          return $default_entity->get('layout_builder__layout')->getSections();
   +        }
   +      }
   

   Just following this logic - if I create a translation of an overriden entity, then remove all sections on that translation and save, the default translation's sections will show up. I thought that if a translatable field didn't have a translated value, accessing it would get the default value. This just seems a bit manual to me, but I could be wrong.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -125,8 +141,14 @@ public function extractIdFromRoute($value, $definition, $name, array $defaults)
   +      list($entity_type_id, $entity_id) = explode('.', $id);
   ...
   +      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface) {
   +        $translated_entity = $this->entityRepository->getTranslationFromContext($entity);
   +        if ($translated_entity instanceof FieldableEntityInterface && $translated_entity->hasField('layout_builder__layout')) {
   +          return $translated_entity->get('layout_builder__layout');
   +        }
   +      }
   

   In getStorageId, IDs of translatable entities should now contain their langcode. Why does this method not respect that and check for a langcode when exploding the ID?

Tagging as this is currently on our shortlist.

Tested this patch on site we're working on and works fine. This is what we saw happening.
The site has 2 languages, where Dutch is the default.
It might be important to note that there were no fields rendered on the page, just views blocks and custom content (text) blocks.

Before the patch

The layout of the dutch node was overridden and had some custom blocks on them.
When we went to the english version, there was just a blank page.
When we edited and added a block, the Dutch version also was affected.

After the patch

When going to the english version, we saw the blocks that were configured on the Dutch version.
When clicking on layout, all the sections were empty and when adding a block, saving and viewing it in the frontend, we could see that one block. The Dutch version was also still rendered as configured before.

So the patch works fine, and I'm totally fine with the behavior at the moment which is:

• Show the blocks from the default translation on the canonical page
• Show nothing when on the layout edit page

So +1 from me.

There's still one problem with translations and it seems to be a layout builder's problem in combination with entity references.

Setup:

• two languages
• translatable taxonomy term
• on the content type, create entity reference field with this taxonomy as target
• content type itself is translatable, except for the term - so they sync
• configure the default layout for this node, put the term field on it, use label for display (but I guess it's a problem for all formatters)
• now create a term and translate it to both languages
• create a node in language A, select that term
• translate the node to language B, the term should be selected still and save it
• The term on the display will be rendered in the original language. This doesn't happen when not using the layout builder for layout.

Will dig a bit to see if I can quickly find this problem.

So the problem is in layout_builder/EntityViewDisplay::buildMultiple().

It contains logic to determine the language the field should be rendered in, but with the current patch, the right entity is already known.
It's then up to the formatters to determine what needs to happen which can be tricky business, especially in case of entity references. However, EntityReferenceFormatterBase::getEntitiesToView() has logic to determine further what needs to happen with the referenced entity, so in the end, the actual fix, is deleting code in layout builder :)

I've attached the diff to see what needs to be removed. Translatable entity reference which are synced are fine now. Less complex fields, like say body, which are marked as translatable on the content type still work fine as well.

Could use confirmation from plach and/or berdir of course :)

- edit - ignore the patch, completely wrong hacking, my god

Crap, I did something completely wrong here, hacking in the wrong class, I guess it's too warm :( Ignore my comment in #10

However, the problem is still there, it's in the wrong language.

Other than that, while debugging, I noticed that (some) fields are being built twice. However, given my brainfart hacking in #10
I will re-confirm that before I open an issue for that (not sure if it's related with this patch or not)

Ok, so, the problem with entity reference and language is related with #2955392: EntityViewBuilder::viewField() does not respect entity current language when used with an entity reference field. The patch in that issue solves it here, going to figure out if I can make something out of it.

Probably important: I'm not even using the current patch at all at the moment, all other fields, marked as translatable on the content type are rendered fine, but it's in the case of synced field on the content type with a translatable reference item where it goes wrong at the moment.
(using default layout, not tested layout per entity, iirc, that's what this patch fixes)

Ok, attached diff which respects fields and translations now.
There's much code duplication as the singleFieldDisplay method is protected in the entityViewBuilder.
Just leaving here, I don't necessarily think this should be included, unless we add it temporarily and remove it when the core issue is fixed.

So in conclusion:

• The patch in #5 fixes the problem when you allow layout overrides per node and have translations
• FieldBlock has a problem with translated fields. This is a totally separate problem - and in way, my gut feeling tells me that is the intention of the original bug report to begin with and you were fixing the wrong problem :)

Next steps:

• Split both problems, so that this one can be committed, with a better explanation of the problem.
• As to whether layout builder should have a fix in fieldBlock, or wait for the other issue in #2955392: EntityViewBuilder::viewField() does not respect entity current language when used with an entity reference field to be fixed: I'm fine with both, I now know at least where to fix/patch it myself :)

(glad I figured this out finally)

Bumping to major, but removing from our tag since it's not actively being worked on.

@swentel Great work! I've updated the issue summary to be scoped to focus on:

The patch in #5 fixes the problem when you allow layout overrides per node and have translations

Feel free to open a follow-up issue for the FieldBlock issue.

In drupal core 8.6.1 I am unable to apply patch in #5

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2946333-layout_translation-5.patch

@ariane i just fixed a patch from #5 for 8.6.1 version in order to be applied with composer

Reroll of #5

IS needs point how that gonna be solved

Back to green!

Updated the issue summary. Might need a @todo on #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing

I testing this patch with inline blocks and I ran into a problem

To see the error you have to have the changes in #2968110: Layout Builder's ConfigureBlockFormBase forms do not display validation errors on submit
You will still get the error without it just won't be displayed.

1. Enabled layout and overrides for articles
2. enabled content translation
3. added spanish. kept english defualt
4. Enabled translation on articles kept all defaults
5. created a node.
6. save an override with a inline block. Label = "Block en"
7. created a translation of the node
8. goto 'es/node/1/layout' to change translations layout.
9. edit inline block label = "Block es"
10. save layout.
11. view node/1 = block label ="Block en"
12. view es/node/1 = block label = "Block es"
13. So far so good 😊
14. goto node/1/layout
15. configure inline block.
16. save the block.
17. Error
    

    The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.

    If you don't have #2968110: Layout Builder's ConfigureBlockFormBase forms do not display validation errors on submit the form just won't submit with no error.

This is because the Spanish version of the layout saves the same block_content entity. So it creates a newer revision.
When the english layout form tries save the previous entity it gets this error because there is a newer revision.

Possible solutions

If block_content entities are not translatable

When creating the translate layout field override duplicate the block so it is new entity.
This what we do if you are creating an override for the first time.

OR we could only duplicate the block the first time user tries to make a change.

But either way the spanish translation would never see newer changes made to the inline block in the english layout translation because we reference revisions.

If block_content entities are translatable

In this case instead of just creating a duplicate we could actually create a translation of the block.

Non-revisionable entities with Layouts

I haven't tested this but looking at \Drupal\layout_builder\InlineBlockEntityOperations::handlePreSave()
if entity with the layout is not revisionable if we don't duplicate that block we would always be saving the revision of the inline because we would not be creating a new revision.

So the changing the inline block in either layout would effect both layouts. So this favors just duplicating the inline blocks when making a translation.

Either way we need tests for the inline block functionality so tagging.

Here is a updated patch with a failing test based on comment #34.

Let's see what happens now.

Works with 8.6.2 too! Many thanks! Saved my day!

Setting back to NW, still working on this

Split that clone portion into #3013197: Cloning an implementation of SectionListInterface does not deep clone the Section or SectionComponent objects

This patch includes patch from #3013197: Cloning an implementation of SectionListInterface does not deep clone the Section or SectionComponent objects. Added tests for translating a node, and then overriding it from default.

Also added a test for deleting a translation which currently fails. So i'm expecting this to go back to NW soon :)

Added #3013340: Add langcode column to inline_block_usage which most likely blocks this issue.

Ok, here are one patch with patches from related issues to make it pass, and a review patch. Now looking for reviews!

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -148,9 +150,21 @@ public function handlePreSave(EntityInterface $entity) {
   +    if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   +      if (!$entity->isNew() && $entity->isNewTranslation() && !$entity->isDefaultTranslation()) {
   +        $is_new_translation = TRUE;
   +        if ($this->isEntityUsingFieldOverride($entity) && !empty($sections)) {
   +          $duplicate_blocks = TRUE;
   +        }
   +      }
   +    }
   

   Shouldn't all this logic be moved within the if statement where we check if $sections?

   Also we need a comment to why we need to duplicate blocks on translations.

2. Any thoughts #34 section If block_content entities are translatable?

   It seems weird that inline blocks would the default language when we know they are part of the layout translation. But also when we first duplicate them they obviously aren't the new language yet.

   What if when they are edited if they are translatable and they aren't language of the current translation when set them to be?

Here's a plain reroll of #44 now that the blocker is in. #47 was not addressed.

Hi, in version 8.6.4 the patch stops working.

@julenmelgar thanks for reporting but patches are not expected to apply/work except against the git branch for Version on the issue. In this case that is 8.7.x.

Was going to tag this as `Needs reroll` but it applies cleanly. But it no longer passes tests, so still NW

[#48] misses patch from #3013340: Add langcode column to inline_block_usage, but i'll let the issue remain in NW for now. will look in to this soon, but if someone is interested in continuing the patch feel free to do so

just a reroll

test fixes

Ok this is a first pass to produce a patch using the patch @plach produced in #2942907-35: Entity system does not provide an API for retrieving an entity variant that is safe for editing(say that 3 times fast 😜)

\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTranslationTest will still fail. I started to address that but the logic for inline blocks still has the assumption of how overrides store their sections which needs to refactored. I am going to create an issue for that.

I know that you're addressing different issues currently, but I still want to adress your question in #47

Any thoughts #34 section If block_content entities are translatable?
It seems weird that inline blocks would the default language when we know they are part of the layout translation. But also when we first duplicate them they obviously aren't the new language yet.

...

I actually lifted this up previously in the proposed serialisation approach and the only benefit I can see is that it would allow us to have the layout not being translatable, but being able to translate the inline blocks. However, there is no UI for that (since we hide the inline blocks from the other UIs.). Maybe there could be a UI if there is a need for this.

What if when they are edited if they are translatable and they aren't language of the current translation when set them to be?

We need to prevent this, and if we were to use translations, we'd need to make sure they don't set the language than the current language to a different language anyway.
There are obviously many more scenarios we need to take in to consideration if we were to go for this approach. What about non-translatable fields on the block?

Are there other benefits I'm overlooking?

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -349,4 +381,47 @@ public function isApplicable(RefinableCacheableDependencyInterface $cacheability
+    if ($langcode) {
+      $contexts[] = new Context(new ContextDefinition('language', 'Interface text'), $langcode);
+      $contexts[] = new Context(new ContextDefinition('language', 'Content'), $langcode);
+    }

@plach point out that I could use \Drupal\Core\Plugin\Context\ContextRepositoryInterface::getAvailableContexts() here. So that removes the need for the language manager and finding the langcode all together.

Ok. this patch focuses on only getting \Drupal\Tests\layout_builder\Functional\LayoutBuilderTranslationTest to pass. I have taken out logic that may have just been put into to deal with inline blocks logic. Those test will fail.

It seems the only non-tests changes are to \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage which makes sense. This may be easier to review while work is done on #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -110,7 +139,11 @@ protected function getEntity() {
-    return $entity->getEntityTypeId() . '.' . $entity->id();
+    $id = $entity->getEntityTypeId() . '.' . $entity->id();
+    if ($entity instanceof TranslatableInterface) {
+      $id .= '.' . $entity->language()->getId();
+    }

ok since we are getting the language from the context not the only reason we need it added to the storage id is so the tempstore will be vary by language.

There is not reason tempstore id has to exactly match the storage id but I don't think we can get the language inside \Drupal\layout_builder\LayoutTempstoreRepository::set() because we only have a SectionStorageInterface instance.

Opened #3026698: Allow section storage to provide a more granular ID for tempstore to address #65

Ok here is patch with the changes to the API here: #2942907-40: Entity system does not provide an API for retrieving an entity variant that is safe for editing

I removed the test methods from \Drupal\Tests\layout_builder\Kernel\OverridesSectionStorageTest: testGetSectionListFromIdTranslatableNoTranslation and testGetSectionListFromIdTranslatable

because \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::getSectionListFromId() has no logic for translation anymore it just calls \Drupal\Core\Entity\EntityRepositoryInterface::getActive()

We do have to test right now if it can handle when the langcode is in the storage id but that is it. So expanded the dataProvider for testGetSectionListFromId() to cover this. This may change with #3026698: Allow section storage to provide a more granular ID for tempstore if the storage id is always the same but the tempstore id is different.

The new API allows us to remove the thin helper method I had for \Drupal\Core\Entity\EntityRepositoryInterface::getActive() to load the entity and get the contexts, since this is now handled by \Drupal\Core\Entity\EntityRepositoryInterface::getActive() 🎉

Quick code review:

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -84,7 +94,9 @@ public static function create(ContainerInterface $container, array $configuratio
+      $container->get('context.repository')

This can go now :)

Fixed #69

This is mega patch with #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing & #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals

1. I had #3026698: Allow section storage to provide a more granular ID for tempstore applied but I don't think we need this for patch.
2. At least \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTranslationTest::testDeletingTranslatedEntityWithInlineBlock() should fail. Because it is expects that the inline block would be deleted when you delete a translation.
   For that to work we would need #3013340: Add langcode column to inline_block_usage. But I think there problems with that approach. Namely before you make a translation you can actually switch the language of the entity with the layout. This would complicate the tracking.

   One option would be to just not delete any inline blocks until the parent entity is deleted. This would you would have inline for translations that have not been delete but you would not be deleting in blocks for previous revisions that had a different language when you delete the translation that was afterwards added in that language. You can read more about the scenario on #3013340: Add langcode column to inline_block_usage

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -92,7 +104,15 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (count($section_list) === 0 && $entity instanceof TranslatableInterface && !$entity->isDefaultTranslation()) {
   +      // If a translated entity has no sections the untranslated entity's
   +      // sections should be used.
   +      $entity = $entity->getUntranslated();
   +      $section_list = $entity->get(static::FIELD_NAME);
   +    }
   +    return $section_list;
   

   My idea here is that if there no sections on the translation we need to get the sections from untranslated entity.

   This is true but if we actually return back the $section_list itself then this will be used to update the sections when configuring the translated override. This results in the translated override actually being stored in the default language key.

   So the solution here was just to copy the sections into the translated section list if empty. This will work for render and for configuring the entity.

Whoops here are the patches

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -110,7 +130,11 @@ protected function getEntity() {
   public function getStorageId() {
     $entity = $this->getEntity();
-    return $entity->getEntityTypeId() . '.' . $entity->id();
+    $id = $entity->getEntityTypeId() . '.' . $entity->id();
+    if ($entity instanceof TranslatableInterface) {
+      $id .= '.' . $entity->language()->getId();
+    }
+    return $id;
   }

This should be done in #3026698: Allow section storage to provide a more granular ID for tempstore

I tested the patch in #71 and it fixes the issue in Drupal 8.7.x-dev

1. rerolling for #2942907-49: Entity system does not provide an API for retrieving an entity variant that is safe for editing
2. #73. Added a @todo
3. Should have some fail as #71

Just to be clear, does this issue affect only versions 8.6.x+ or not. I was able to update the translated content in my Drupal installation 8.5.4, but not in 8.6.1

1. re #77 not sure of your question. You can still translate content. The problem here is for having a different layout override per language.

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
   @@ -0,0 +1,250 @@
   +    $this->drupalGet($translated_layout_url);
   ...
   +    $assert_session->linkExists('Save Layout');
   ...
   +    $assert_session->addressEquals($translated_entity_url);
   +    $assert_session->pageTextNotContains('The untranslated field value');
   +    $assert_session->pageTextContains('The translated field value');
   +    $assert_session->pageTextNotContains('Powered by Drupal');
   

   #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing needs a reroll so not providing a patch now but I found a problem here.

   If we add after this the check

   // Ensure the untranslated entity still has the added block.
   $this->drupalGet($entity_url);
   $assert_session->pageTextNotContains('The translated field value');
   $assert_session->pageTextContains('The untranslated field value');
   $assert_session->pageTextContains('Powered by Drupal');
   

   The last assert here will fail. That is because the issue detail in this new issue #3033686: Saving Layout override will revert other field values to their values when the Layout was started.

   Basically what happens in the test is:

   1. translated_layout_url is hit before we have a saved override for the untranslated entity.
   2. This version of the entity is saved in the tempstore for the translated layout
   3. The untranslated layout is saved.
   4. The translated layout is saved contain the empty sections from tempstore for the untranslated layout.

   This wipes a layout but points a general problem that we save all the previous field values in the tempstore so this would happen with any field value which why I created the other issue to address this first.

The blocker is back in!

Here's a reroll of "2946333-75-do-not-test.patch".
I had to remove the OverridesSectionStorageTest modifications, because that class is completely different now.

Here's a combined patch of #82, #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals #66 and #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing #115.

@bendeguz.csirmaz thanks rerolling. Sorry this issue is a complicated mess right now.

reroll of https://www.drupal.org/files/issues/2019-02-11/2946333-75-do-not-test.patch

we don't actually need #3026434: Ensure that Layout Builder Inline Blocks doesn't assume section storage internals right now.
So just including #2942907-149: Entity system does not provide an API for retrieving an entity variant that is safe for editing

I am not sure the current approach is going to work.

I talked this over with @tim.plunkett and it might be better to have a new section storage
TranslationOverridesSectionStorage extends OverridesSectionStorage
Then we can give it a weight so it has priority over OverridesSectionStorage

I'm not even sure you want to extend it. Possibly, but I'd suggest trying without that first

updated Proposed resolution with detail description

Sorry, I'm not up-to-date per se (but I read the updated issue), but why would we need a TranslationOverridesSectionStorage?

@tedbow Please adress my concerns in #60 about translating the inline blocks

I talked with both @johndevman and @plach today about this issue and specifically how inline block translation will work.

1. One thing that @johndevman pointed out that I had not considered was that translatable block bundles can have some fields that are not translatable. This could lead to editing some fields on an inline block and having that effect multiple translations.

   For this reason I think we should actually be duplicating the blocks when we are creating a translation of the entity with layout rather than creating translations of the blocks. The other reason this will be better is because once you translate an override it is has no connection to the untranslated override. You can add new blocks and reorder blocks. So there will not be a way logically through the UI to connect a block to its translation on a translated override. They could be in totally different parts of the layout or not exist at all.

2. Another consideration I didn't take into account whether the layout_builder__layout field itself is translatable. It may not be translatable even when the bundle is translatable.

   I talked with @plach about this and he suggested that if the layout_builder__layout field itself is NOT translatable then we should offer the ability to translate the individual inline blocks.

   While I agreed with this when we chatted I have since had second thoughts

   There are 2 big problems with this:
   J

   1. Just because the layout_builder__layout field is not translatable doesn't mean that the field won't later be turned on for translations

      If we have different behavior for when layout_builder__layout field is not translatable vs when it is translatable then we have to some sort of migration for when the field becomes translatable. Would we do that for all entities of the bundle or just as the new translated override layout gets created for an entity that didn't have 1? If don't do migration should we keep offering the ability translate the individual inline blocks if you haven't create an translated override yet?

      I think there are just 2 many possibilities and the migration could be very tricky and involve thousands of entities.

   2. currently inline blocks have not path for editing so what would the UI for providing translations look like. Would it be in context of the larger layout which is not alterable per language?
3. So my suggestion would be
   If layout_builder__layout field is not translatable don't allow any translating of inline blocks

   If layout_builder__layout field is translatable duplicate the inline blocks when creating a layout translation

Here is an updated

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -103,7 +113,22 @@ public static function create(ContainerInterface $container, array $configuratio
   +    if (count($section_list) === 0 && $entity instanceof TranslatableInterface && !$entity->isDefaultTranslation()) {
   +      // If a translated entity has no sections the untranslated entity's
   +      // sections should be used.
   +      // We have to actually set the sections on the translated entity and
   +      // return that sections list. If we return $untranslated_section_list
   +      // then the untranslated field value will be updated.
   +      /** @var \Drupal\layout_builder\Field\LayoutSectionItemList $untranslated_section_list */
   +      $untranslated_section_list = $entity->getUntranslated()->get(static::FIELD_NAME);
   +      foreach ($untranslated_section_list->getSections() as $untranslated_section) {
   +        $section_list->appendSection($untranslated_section);
   +      }
   +    }
   +    return $section_list;
   

   this logic I just don't think is going to work.

   I have changed the patch so that there will be separate section override section storage per entity translation.

2. I have not update any of the logic for the inline blocks yet.
3. I will provide self-review/explanation comment in the next couple hours

whoops here is the patch

Also #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing was committed!!!!!!! So no combo patch!!

Is this 1:1 built on top of #2942675: Layout builder should use the active variant of an entity to avoid orphaned revisions or just similar?

I think the proposed resolution is good.
Just some concerns about how self-contained this logic should or could be.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -326,3 +326,20 @@ function layout_builder_system_breadcrumb_alter(Breadcrumb &$breadcrumb, RouteMa
   +    $translatable_fields = $translation->getTranslatableFields();
   +    if (array_key_exists(OverridesSectionStorage::FIELD_NAME, $translatable_fields)) {
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -398,4 +397,58 @@ public function isOverridden() {
   +      $translatable_fields = $entity->getTranslatableFields(FALSE);
   +      return array_key_exists(static::FIELD_NAME, $translatable_fields) && $entity->isTranslatable();
   

   This seems like it should have a method somewhere. Could be a follow-up of course, might even be an "upstream" addition.

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -141,12 +142,20 @@ protected function prepareLayout(SectionStorageInterface $section_storage) {
   -    elseif ($section_storage instanceof OverridesSectionStorageInterface && !$section_storage->isOverridden()) {
   ...
   +      if ($section_storage->isTranslatable() && !$section_storage->isOverridden() && !$section_storage->isDefaultTranslation()) {
   ...
   +      elseif (!$section_storage->isOverridden()) {
   

   Looks like you could keep the !$section_storage->isOverridden() in the original conditional?

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -288,6 +289,11 @@ protected function buildSections(FieldableEntityInterface $entity) {
        $storage = $this->sectionStorageManager()->findByContext($contexts, $cacheability);
   +    if ($storage instanceof OverridesSectionStorageInterface) {
   +      if ($storage->isTranslatable() && !$storage->isDefaultTranslation() && !$storage->isOverridden()) {
   +        $storage = $storage->getDefaultTranslationSectionStorage();
   +      }
   +    }
   

   I have no idea how possible this might be, but could this be contained within findByContext() somehow, somewhere within that call stack?

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   --- a/core/modules/layout_builder/src/OverridesSectionStorageInterface.php
   +++ b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php
   
   +++ b/core/modules/layout_builder/src/OverridesSectionStorageInterface.php
   @@ -31,4 +31,28 @@ public function getDefaultSectionStorage();
   +  public function isTranslatable();
   ...
   +  public function isDefaultTranslation();
   ...
   +  public function getDefaultTranslationSectionStorage();
   

   While our current implementation does mean that overrides == content entities, I think these 3 might deserve their own interface.
   Layout Library, for example, is a config entity that implements OverridesSectionStorageInterface.
   Plus, adding a new interface is more BC friendly.

5. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -398,4 +397,58 @@ public function isOverridden() {
   +          'view_mode' => new Context(ContextDefinition::create('string'), 'full'),
   

   Whenever hardcoding full, please include the @todo we have elsewhere.
   It's something like // @todo Expand to work for all view modes in https://www.drupal.org/node/2907413.
   Also see the code in OverridesSectionStorage::deriveContextsFromRoute() that ensures the view mode is the 100% correct one

1. I am not sure I am actually doing this right. If this is checking for the field being able to translatable or if it is currently translatable. I would like confirm before trying to generalize this.
2. fixed and simplified
3. I looked at a couple ways to do this. It is possible in isApplicable() but setting the entity to the untranslated if the default is not override. But feels wrong to set something there is is*() method.

   I look at doing it in setContext() but this is also called in load() which has other side-effects

   I could put the logic directly in but doesn't seem much better.

4. Created new interface
5. fixed

also did this with tests

Non-translatable layout field
if the "Layout" field for a bundle is set to NOT be translatable then the Layout tab should not be shown for translations.

+++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
@@ -264,4 +268,20 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
+  private function isNewEntityTranslation(EntityInterface $entity) {
+    if ($entity instanceof TranslatableInterface && $entity->isTranslatable() && $entity->isNewTranslation()) {
+      return TRUE;
+    }
+    return FALSE;
+  }

Actually we don't need to be testing here if the entity itself is a new translation. It never will be using the layout builder as the entity has to already created.

We need to check if the layout translation is new. So for this translation were the section empty before.

I have fix for this. I thought I could use $entity->original but this does not work.

I had to do.
$unchanged_translation = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadUnchanged($entity->id())->getTranslation($entity->language()->getId());

So I am loading the unchanged and then getting the translation we are currently working with.

This gets the test methods in InlineBlockTranslationTest to pass except for testDeletingTranslatedEntityWithInlineBlock() which I don't expect to work yet.

Hi there

I was evaluating Layout Builder as a Paragraphs alternative and found this issue when looking around for inline block translations. I'm concerned about the feedback in #91, specifically:

For this reason I think we should actually be duplicating the blocks when we are creating a translation of the entity with layout rather than creating translations of the blocks. The other reason this will be better is because once you translate an override it is has no connection to the untranslated override. You can add new blocks and reorder blocks.

That approach is impractical for the vast majority of editors I've worked with. It would mean that after initial page creation, that adding a block to n languages would mean creating n blocks. Reordering a page would have to be done in each language separately. Furthermore, even if you use media entities, changing a reference to another media entity could mean editing n blocks again, even if the field itself is untranslated.

The proposed solution in #91 is a consistent solution to asynchronous translations but I believe the synced translations are the much more common scenario, and if I read the issue correctly, synced translations are not planned to be supported, right?

The solution which would make most sense to me would be to make layout overrides untranslatable (that can be set already but makes "Layout" disappear) and add a translation edit mask for inline blocks (in lieu of "customize" on the block).

Is there maybe a chance that this approach could be revisited or amended for this use-case?

@grahl you concerns are very similar to others I have heard from @Gábor Hojtsy, @plach, @lauriii and others.

I think agree need to switch to supporting synced translations.

Right now in 8.7.x we have minimal support for un-synced translations but this was unintended in consequence of #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features.

I was assuming we had to support this because to take away this functionality would be BC break. But since #3004536 was not backported to 8.6.x this functionality has actually never been in core release! so are free to not support this anymore.

I am working on a more detailed plan. But right now the idea is to have the actually layout not be translatable but have 2 things be translatable

1. The inline blocks(if the block types are translatable)
2. The block labels which are entered in the Layout builder.

@tedbow Thanks so much for the quick response, the approach sounds great!

Will keep an eye on this issue to see if I can assist with that feature in some way.

Got a help from @tim.plunkett about this actually might be implemented.
Ok here is a start to #102. The interdiff is bigger than the patch so I am not sure how useful that will be.

1. It changes to store labels in configuration of each plugin. We could also store on the component level I guess.
2. On the layout translation override tab it only gives the ability to change labels, not inline blocks yet
3. All other layout changes are not allowed for translations(because it actually stores in the untranslated)
4. If there not a default translation override yet it you can't access the translation layout tab.

It doesn't handle inline blocks yet.

We also will need to be able translate the default(per bundle) layouts. Probably much of the same logic can be reused(provided we have same behavior of only allowing translating of block labels and inline blocks)

Because this method does not use the actual layout field translate value for storing translated sections(sections themselves aren't different) it leaves the possibility use field per language values later for actually creating totally independent layouts per language(truly different field values)

Ended up being a small review for now, will make a more in-depth review as the patch is in a more clear state. Some questions and thoughts though:

1. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderTranslationAccessCheck.php
   @@ -0,0 +1,37 @@
   +    $access = AccessResult::allowedIf(!($section_storage instanceof TranslatableSectionStorageInterface && !$section_storage->isDefaultTranslation()));
   

   Using 'not' operators make this really hard to interpret by looking at it.

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -105,6 +108,7 @@ public function preRender($element) {
   +    $is_translation = $section_storage instanceof TranslatableSectionStorageInterface && !$section_storage->isDefaultTranslation();
   
   @@ -114,17 +118,27 @@ protected function layout(SectionStorageInterface $section_storage) {
   +      if (!$is_translation) {
   ...
   +    if (!$is_translation) {
   

   Could we move the "$is_translation" - part to a method instead. This would make it easier to override for contrib. We don't really need to say "isTranslation", either it could be more generic, which would make sense for a contrib. like "Section lock".

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -231,7 +253,8 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
   +    $is_translation = $section_storage instanceof TranslatableSectionStorageInterface && !$section_storage->isDefaultTranslation();
   

   It could be a "Read-only" state if we were to make this more generic.

4. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -240,46 +263,56 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
   +          if (!$is_translation) {
   ...
   +          if (!$is_translation || $needs_translation_link) {
   ...
   +      if (!$is_translation) {
   

   It would be cleaner code to have separate methods for a read-only and default mode.

5. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -294,50 +327,81 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
   +  protected function defaultComponentHasTranslatableSettings(SectionStorageInterface $section_storage, SectionComponent $component) {
   

   How would this work if the block plugin provides its own layout_builder_translation form and ignores labels?

6. +++ b/core/modules/layout_builder/src/EventSubscriber/ComponentPluginLabelTranslate.php
   @@ -0,0 +1,46 @@
   +    if (!$plugin instanceof ConfigurableInterface && !isset($contexts['@language.current_language_context:language_interface'])) {
   ...
   +    $language = $contexts['@language.current_language_context:language_interface']->getContextValue();
   

   Do we really use the language interface here? Not the content language? Might work differently in contexts though, I don't know for sure.

7. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +class BlockPluginTranslationForm extends PluginFormBase implements ContainerInjectionInterface {
   

   Some obvious things here with code standards, but I suppose you haven't gotten to that yet.

8. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +     $form['translated_label'] = [
   +       '#title' => $this->t('Translated label'),
   

   Maybe just Label? If you add the translate form block, it should be kinda implicit anyway.

I just had a first code review, I realize this is a wip so feel free to disregard all the nitpicks :)

1. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -101,6 +108,21 @@ layout_builder.update_block:
   +layout_builder.translate_block:
   +  path: '/layout_builder/translate/block/{section_storage_type}/{section_storage}/{delta}/{region}/{uuid}'
   +  defaults:
   +    _form: '\Drupal\layout_builder\Form\TranslateBlockForm'
   +    _title: 'Translate block'
      requirements:
        _permission: 'configure any layout'
        _layout_builder_access: 'view'
   

   No translation requirement here?

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -138,6 +152,14 @@ protected function prepareLayout(SectionStorageInterface $section_storage) {
   +        //   save since the last time this page was opened.
   

   Typo, also I'm not sure what this comment is trying to tell me, but maybe that's ok :)

3. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +use Drupal\Core\Render\Element;
   

   Unused

4. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +  protected $current_language;
   ...
   +   * BlockPluginTranslationForm constructor.
   +   *
   +   * @param $current_language
   

   Missing PHP docs

5. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +  public function __construct($current_language) {
   

   Missing type hint

6. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +    $this->current_language = $current_language;
   

   We normally use langcode to denote language codes, this should be current_langcode.

7. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationForm.php
   @@ -0,0 +1,84 @@
   +      $configuration['layout_builder_translations'][$this->current_language]['label']  = $form_state->getValue('translated_label');
   

   Extra whitespace before =

8. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -385,4 +387,44 @@ public function isOverridden() {
   +    // @todo If not translatable should we always consider this the default
   +    //   translation?
   

   Seems fair

9. +++ b/core/modules/layout_builder/src/TranslatableSectionStorageInterface.php
   @@ -0,0 +1,39 @@
   + * @internal
   + *   Layout Builder is currently experimental and should only be leveraged by
   + *   experimental modules and development releases of contributed modules.
   + *   See https://www.drupal.org/core/experimental for more information.
   

   Is this really the case with LB targeting stable?

10. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
    @@ -0,0 +1,142 @@
    +    // The translate layout is not available.
    

    I'm not sure what this comment is trying to tell me either :)

11. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
    @@ -0,0 +1,142 @@
    +    $page = $this->getSession()->getPage();
    ...
    +    $layout_url = $entity_url . '/layout';
    

    Not used

12. +++ b/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
    @@ -127,6 +127,7 @@ public function providerTestExtractIdFromRoute() {
    +    $this->entityRepository->getTranslationFromContext(Argument::cetera())->shouldNotBeCalled();
    

    Why?

Do we really use the language interface here? Not the content language? Might work differently in contexts though, I don't know for sure.

Good point, we should probably use content language here. However with the Context API it's a bit tricky, see EntityRepository::getContentLanguageFromContexts().

re test fails in #104

1. LayoutBuilderMultilingualTest failed because now you cannot add new blocks in the layout translation. Changed this use a node in different language(not a translation). This still will fail if the fix if #3019333: If you translate the literal "inline blocks" to another language in the layout builder, it stops working. is commented out.
2. Same issue with \Drupal\Tests\layout_builder\Functional\LayoutSectionTest::testMultilingualLayoutSectionFormatter change a node in another language.

New Tests

Added basic tests for translating labels. Wanted to do this before address the reviews so I don't mess up existing behavior.

re #106

@johndevman thanks for the review.

1. Ok made this simpler $this->isDefaultTranslation() || $this->isOverridden()
2. I change the variable name to $sections_editabl. This allow removing most of the if statements and just using '#access' => $sections_editable.
   Since this is local var this won't really help contrib except if that wanted to undo the access restriction which is dangerous but anytime you undo access it is dangerous.
3. changing to $sections_editable to consistent.
4. I made misread which sections of code you were commenting on(can't wait for gitlab!) but I think using the '#access' property makes this cleaner now. Also cleaned up contextual links logic.
5. Good point but if I block provides its own layout_builder_translation form then they should still not ignore labels. But they maybe other things to translate like in the case of inline blocks.

   So I will tackle this when I do the inline blocks logic. Because even if no label is displayed in the case of translatable inline blocks we still need to be able to translate the block.
   adding todo for now.

6. good catch!

   Actually we have the entity that we are going to display so I think . we can just use the langcode from the entity itself. Because isn't the context on the request scope? We could be rendering an entity in a different language than the main page is, correct? So we want the langcod of the actual entity being rendered by the laytout builder. which might be a entity rendered in a view.

7. Yes it was late. fixed
8. fixed

re #107

@plach thanks for review

1. Yes, sorry this was confusing. The current was only give access if it is NOT as translation. So I changed this requirement to be on all routes but with a value of either 'untranslated'(don't want to use 'default) because 'defaults' has other meaning in Layout Builder) or 'translated'. Update the access check class to handle this.
2. fixed typo. This todo has to be handled in this issue.

   Basically right now if you open up the translated layout and at the same time you(or someone else) opens up the untranslated layout and saves untranslate first with changes, I think saving translated will undo changes. Because of the tempstore. Need to confirm and prevent this.

3. fixed
4. fixed
5. fixed
6. fixed
7. fixed
8. thanks for confirming. removing todo
9. I think this is boilerplate text we put on all class that will have @internal removed. and when we mark it stable we remove all at once. That will be in the final "Mark Layout builder as stable" issue.

   Basically it is true until it is not 😜

10. fixed
11. updated tests use these.
12. yes don't need that explicitly. removing

1. +++ b/core/modules/layout_builder/src/Access/LayoutBuilderTranslationAccessCheck.php
   @@ -0,0 +1,47 @@
   +    if ($translation_type === 'untranslated') {
   +      $access = AccessResult::allowedIf(!$section_storage instanceof TranslatableSectionStorageInterface || $section_storage->isDefaultTranslation());
   +    }
   +    elseif ($translation_type === 'translated') {
   +      $access = AccessResult::allowedIf($section_storage instanceof TranslatableSectionStorageInterface && !$section_storage->isDefaultTranslation());
   +    }
   

   Need an else here with an exception for unexpected value.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/TranslationTest.php
   @@ -0,0 +1,140 @@
   +    drupal_static_reset();
   +    \Drupal::entityTypeManager()->clearCachedDefinitions();
   +    \Drupal::service('router.builder')->rebuild();
   +    \Drupal::service('entity.definition_update_manager')->applyUpdates();
   +
   +    $this->rebuildContainer();
   

   All of this seems be unneeded. Tests pass without it.
   Removing

fix #110 and code sniffer problems.

There is problem with field blocks and label translations. Pretty sure this because they are ContextAwarePluginInterface and other blocks aren't.

Need to fix and use them in tests.

Re #109

2. We could use a "isEditable", method or something similar on the section storage as well. But I like this change I think, it's less duplicated code now :)
5. Well, it depends, the labels might not be used. For instance, in a project I build, we've removed them.
6. I agree, that looks good.

fixed the field blocks issue. It won't crash layout builder now but still the translated labels won't show up because of #3039185: Allow field blocks to display the configuration label when set in Layout Builder.

So added test coverage which I have commented out with todos.

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -340,4 +375,33 @@ protected function buildAdministrativeSection(SectionStorageInterface $section_s
   +      foreach ($section_storage->getDefaultTranslationSections() as $default_translation_section) {
   

   We don't actually need to check the defualt translation sections anymore because we are not using separate sections per translation. Removed this and removed it from the interface.

2. Adding support for translating inline blocks.
   
   1. Added LayoutBuilderTranslatablePluginInterface with hasTranslatableConfiguration. This allows plugins to determine for themselves if they aren currently translatable. For inline blocks if the block type is translatable they are too.
   2. added InlineBlockTranslationForm extends BlockPluginTranslationForm This will simply create a translation if it doesn't exist or load the existing translation.
   3. Set InlineBlockTranslationForm as the layout_builder_translation for inline block plugin.
3. Changed the existing inline block for to deny access to the langcode field. Users should not be able to choose. Because it should based on the entity that has the layout.
4. I don't have time to do the test right now for inline block translation but I will do that next.

Looked through the interdiff, looks good to me!

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -153,6 +157,9 @@ public static function processBlockForm(array $element, FormStateInterface $form
+      $element['langcode'] = FALSE;

$element['langcode']['#access'] = FALSE

Doing a manual review now.

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -280,4 +287,12 @@ public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE
+    return $block_content->isTranslatable() ||  (!empty($this->configuration['label_display']) && !empty($this->configuration['label']));

If this condition is true because of the configuration, the inline block form must be hidden.

Tested on 8.7.x

Setup

Installed Language, Content Translation
Installed Layout Builder, Layout Discovery

Added Swedish language

Enabled Layout Builder and Overrides on Basic page

* Layout is not shown under Content language 👏
* All inline blocks was successfully deleted when I removed all nodes after the tests below, in preparation for next patch :)

#1

* Created a node (en)
* Added a 'Basic block' inline block with label and body
* Translated node
* Translate the inline block
* Sets the label and body differently
* The label is translated properly
* The body shows the Swedish translation on the English node. If I update the English inline block again it is shown on the Swedish node.

So, it looks like I didn't enable translation for the block type. When I do that, the translations works as expected.

#2

Now I set the 'Body' field to be untranslated on the Block type.

* Created a node (en)
* Added a 'Basic block' inline block with label and body
* Translated node
* Tries to change the inline block body of the translated node, nothing happens (as in it's not translated, when I update the translation)
* Tries to change the inline block body of the original node, nothing happens. (as in it's not translated, when I update the translation)

#2 Untranslated fields should not be shown on the translate inline block form?

@johndevman thanks for the manual review.

1. #116 fixed
2. #117
   

   So, it looks like I didn't enable translation for the block type. When I do that, the translations works as expected.

   Ok so the ability to translate the block should NOT have been there if the block was not translatable. You should have only be able to translate the label(if shown) as with other blocks.

   +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -280,4 +287,12 @@ public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE
   +  public function hasTranslatableConfiguration() {
   +    $block_content = $this->getEntity();
   +    return $block_content->isTranslatable() ||  (!empty($this->configuration['label_display']) && !empty($this->configuration['label']));
   +  }
   

   I forgot that the translation form was being shown even if the block is not translatable when the label needs to be translated. But in the form logic I didn't account for this.

   fixed so that the block entity form is only show if the block entity is translatable.

3. #2 Untranslated fields should not be shown on the translate inline block form?

   there is setting on the translation page "Hide non translatable fields on translation forms" that we should probably respect. In less we think inline blocks are such a special case that the non translatable fields should only be shown on the default translation layout.

   That would be inline with other settings in the other blocks. They affect both untranslated and translated layout but we only show them on untranslated.

   Anyways if we did want to respect that setting I am not sure how content_translation is enforcing this now. I looked for the setting untranslatable_fields_hide but I don't see affecting the forms fields.

4. I added a todo to check \Drupal\content_translation\Controller\ContentTranslationController::add() and edit() to see if we need to implement any of the special logic it does to create the translation form. We might need to copy the logic now and do a follow to refactor that controller so we can just ask content_translation for the form. Of course we may not only all of the behaviors.

   This could handle the untranslatable_fields_hide setting for us. not sure

   Maybe @plach would know more about this?

#118.3 I'm not entirely sure on that setting, but I think it's more about solving data losses on pending vs. default revisions when using Content Moderation. Do we intend to support Content Moderation on Inline blocks? Or should we ignore it? If we should use that to hide neutral fields on the translations form I don't know.

Also, just to clarify: Currently you cannot change a language neutral field on the translation form.

1. Do we intend to support Content Moderation on Inline blocks?

   I would say no. The entities with layouts can use content_moderation.

   I suppose you could still enable content_moderation on the block types. So I don't know if we just don't support any block types that have content_moderation enabled or only support them if you can always save to the published state. And if a user cannot then they can't add/edit inline blocks.

2. I talked to @plach about and gets very complicated. My next step is change InlineBlockTranslationForm to extend ContentEntityForm. This will make it easier to integrate with content_moderation. We will then have to implement PluginFormInterface. and maybe move some logic in BlockPluginTranslationForm to a trait so it can reused in InlineBlockTranslationForm still enve that we can't extend BlockPluginTranslationForm any more.
3. This patch has basic tests for inline blocks in translations.

talked to @tim.plunkett and @plach

The combo of layout_builder + content_moderation + content_translation is going to make it super difficult to store a layout that will be the same across all languages(when each could have a forward revision) and the translate labels in 1 field.

I am working storing the translated labels(and maybe future other plugin settings) in another field that will be translatable.

This is NOT different layouts per language. Just different settings per component per language. With core hardcoding the restriction for just 'labels' for now.

Here is the first try at #122

Sorry I don't have to explain more about the approach. I will in 5 or so hours

This adds translations of inline blocks in layout translations with test coverage.

Next working on translations of layout defaults, DefaultsSectionStorage.

The interdiff was incorrect and used the wrong extension

Code review for #123:

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -93,7 +94,9 @@ public function getInfo() {
   +      $language = !empty($element['#lanuauge']) ? $element['#lanuauge'] : NULL;
   

   Typo

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -93,7 +94,9 @@ public function getInfo() {
   +      $element['layout_builder'] = $this->layout($element['#section_storage'], $language);
   

   $language can't be null according to the interface

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -212,6 +213,41 @@ protected function addSectionField($entity_type_id, $bundle, $field_name) {
   +      $field = FieldConfig::create([
   +        'field_storage' => $field_storage,
   +        'bundle' => $bundle,
   +        'label' => t('Layout Labels'),
   +      ]);
   +      $field->save();
   

   Should we mark this as internal or are we planning to expose it to HTTP API clients?

4. +++ b/core/modules/layout_builder/src/EventSubscriber/ComponentPluginTranslate.php
   @@ -0,0 +1,55 @@
   +    $plugin = $event->getPlugin();
   

   We could return early here if the site is not multilingual.

5. +++ b/core/modules/layout_builder/src/EventSubscriber/ComponentPluginTranslate.php
   @@ -0,0 +1,55 @@
   +    $entity = $contexts['layout_builder.entity']->getContextValue();
   

   Does this return the correct entity translation?

6. +++ b/core/modules/layout_builder/src/Form/LayoutBuilderEntityViewDisplayForm.php
   @@ -69,6 +70,29 @@ public function form(array $form, FormStateInterface $form_state) {
   +      foreach (\Drupal::languageManager()->getLanguages() as $language) {
   

   Let's inject the language manager :)

7. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -86,16 +88,32 @@ protected function init(FormStateInterface $form_state) {
   -    // Add the widget for Layout Builder after the alter.
   -    $display->setComponent(OverridesSectionStorage::FIELD_NAME, [
   -      'type' => 'layout_builder_widget',
   -      'weight' => -10,
   -      'settings' => [],
   -    ]);
   +       // Add the widget for Layout Builder after the alter.
   +      $display->setComponent(OverridesSectionStorage::FIELD_NAME, [
   +        'type' => 'layout_builder_widget',
   +        'weight' => -10,
   +        'settings' => [],
   +      ]);
   

   Wrong indentation

8. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -86,16 +88,32 @@ protected function init(FormStateInterface $form_state) {
   +   * @inheritDoc
   

   Missing { }

9. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutTranslationItem.php
   @@ -0,0 +1,67 @@
   + * @FieldType(
   + *   id = "layout_translation",
   

   Why do we need a new type? Isn't a map item enough?

10. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
    @@ -37,7 +39,8 @@
    + *       "EntityHasField" =
    + *   \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::FIELD_NAME,
    

    Unintended change?

11. +++ b/core/modules/layout_builder/src/TranslatableSectionStorageInterface.php
    @@ -28,4 +28,29 @@ public function isTranslatable();
    +   * Sets the translated component configuration.
    +   *
    +   * @param string $uuid
    +   * @param array $configuration
    ...
    +   * Gets the translated component configuration.
    +   *
    +   * @param string $uuid
    ...
    +   * Get the translated configuration for the layout.
    +   *
    +   * @return array
    

    Missing @param description.

Code review for #125:

+++ b/core/modules/layout_builder/src/Form/InlineBlockTranslationForm.php
@@ -64,6 +74,19 @@ public static function create(ContainerInterface $container) {
+        $entity = $this->entityRepository->getActive('block_content', $entity->id());

@@ -71,6 +94,7 @@ protected function getEntity() {
+      $entity = $this->entityRepository->getActive('block_content', $entity->id());

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -226,6 +238,7 @@ protected function getEntity() {
+        $entity = $this->entityRepository->getActive('block_content', $entity->id());

Why are we doing this? Revision negotiation should not be performed when the ID of the revision we need to load is known in advance. Additionally we are instantiating the desired translation afterwards, so I don't see the point of these calls.

@plach thanks for the review. Just want to address to 2 points as I am working on the defaults translations.

#123

3. Yes it should be internal. I am not sure where are other field is marked as internal.
   But we will have to do the same thing we did with our existing field here #3028301: Do not expose to Layout Builder's sections either in defaults or overrides to REST and other external APIs

#125

Revision negotiation should not be performed when the ID of the revision we need to load is known in advance.

We store the revision because we want to render the exact revision of block with the exact revision of entity with the layout.

but if we don't call $this->entityRepository->getActive() and the revision id is not the latest, because a new revision was made when creating a translation of the inline block in the translation layout we will get an error

The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved.

This why I expanded the test \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTranslationTest::testInlineBlockContentTranslation()

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTranslationTest.php
   @@ -0,0 +1,238 @@
   +    // Update the default translation's version of the block.
   +    $this->drupalGet('node/1/layout');
   +    $this->configureInlineBlock('Block en body', 'Block updated en body');
   +    $this->assertSaveLayout();
   

   This would fail if we didn't have the getActive() call in \Drupal\layout_builder\Plugin\Block\InlineBlock::getEntity() because we have just update the translations layouts inline block.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTranslationTest.php
   @@ -0,0 +1,238 @@
   +    // Update the translation block after updating default translation block.
   +    $this->drupalGet('it/node/1/layout');
   +    $this->updateTranslatedBlock('Block newer updated it label', 'Block updated it body', 'Block even newer updated it label', 'Block newer updated it body');
   +    $this->assertSaveLayout();
   

   This would also fail if we didn't have getActive() call in \Drupal\layout_builder\Form\InlineBlockTranslationForm::getEntity()

Thanks @plach and @tedbow.

@webchick, @Gábor Hojtsy, @effulgentsia, and I discussed this issue today. We should take the approach here that we did for Layout Builder's REST support as well as JSON:API's translation support: Start by disabling translation entirely, so that we don't get into a situation where we might have data integrity problems or lost work due to incomplete translation support. That's the minimum scope that needs to be a stable blocker, because it affects site data and sitebuilding decisions that are difficult to change after the fact. Tagging "Needs followup" to create a new issue for that first step, and postponing on it.

Then, we'll add Layout Builder translation support back once we're ready to ship translation of labels in defaults, labels in overrides, and inline blocks as a single integrated feature with integrated UX. @webchick and @Gábor Hojtsy confirmed that from a product perspective this can be treated as a missing key feature, which does not need to block marking Layout Builder itself stable. (It would be a blocker for enabling Layout Builder in Standard.)

per #130 created #3041659: Remove the layout tab from translations because Layout Builder does not support translations yet

This is no longer postponed(?) and I also think this deserves a issue summary update, because the goals and approaches of the issue has changed a lot.

Copying over @berdir's comment from #3044386-7: [META] Make Layout Builder layouts translatable because it deals with overrides so is more relevant here.

As mentioned earlier, this is what we call symmetric and asymmetric translations in paragraphs and we currently only support symetric in the main module, but there's a contrib module and issues/patches for the other variant.

I don't know the layout builder structure in detail, but the advantage of paragraphs/ERR is that the field itself doesn't store any translatable things and is very simple/structured, just two integer references. So we can just make the field untranslatable, handle the logic of displaying/editing the right translation of the paragraphs in the widget and we're done (as long as you ignore workflows, see below). (Obviously even that part is far from simple, because current entity forms are not very good at tracking the current language, it can be changed and so on..)

However, it's more complicated for you, because even when you also just store references (non-reusable content blocks), then those are in a blob, and you can also have block settings that can be translatable labels, like the block title. Given that, I'm not sure that an untranslatable field is even an option for you. It sounds more like image field translations work, where each property can be translated or not, and the untranslatable ones are synced between all translations. Except in your case it's much more complicated, as it would be based on the block config schema to decide recursively what can be merged/changed in a translation and what can't? Such a feature could also be interesting for the block_field module, as well as things like commerce shipping methods, which has a visible shipping rate label in within plugin settings stored in a field (#2934142: Shipping method entity does not support translations).

Where it gets really complicated is workflows and specifically drafts, because each translation is basically its own revision branch *but* each revisions contains all translations that exist at that point in time. Imagine this scenario:

1. You create a published EN node with a non-reusable content block, node revision 1 (N1) and block revision 1 (B1).
2. Now you create a EN draft and also change the block giving you N2 and B2.
3. Now you want to translate it and add a DE draft and also translate the block as N3 and B3.
4. Then you publish the EN draft and maybe you also make a few more changes to that, keeping it published, that gives you N4 + N5 and B4 and B5 (maybe the block has fewer revisions, maybe not)
5. Finally, you also want to to publish the DE draft, which is N3 and B3. But you can't just take those revisions, because they do not contain the more recent EN changes. So what core does at this point is prepare N6 based on the current default revision N5 (so there could even be extra EN drafts now), then copies over all the DE values of translatable fields into that and saves that.

Now, if you don't do anything, if the layout field is translatable, then you go back to the EN content in B3 because the whole thing is copied over 1:1. And if it's not translatable, you don't actually get the DE translation of the block. Either way, you lose. Additionally, entities also need to know which translations changed in a revision, which it for example uses to filter the list of revisions shown based on the current language. And if you think that's not enough fun yet, add a few more translations to the mix.

What we did for paragraphs is we put several things into core that are explained in this change record: https://www.drupal.org/node/2975280. This allows us to...

a) show the widget of our untranslatable field on translations (we disallow all structure changes ourself then, like add/remove/reorder paragraphs). This is not directly an issue for you, but you still need to figure out how the UI should look/work while being on a translation.
b) then we basically lie to core about the field having language-affecting changes (technically it always changes on translations, but we'll deal with that later)
c) finally, we use the revision create hook to repeat the merge process that core just did for the host entity and repeat that (recursively) for all referenced paragraphs.

As far as I understand, layout builder *must* implement something similar, except again, I expect it to be more complicated, as you need to merge whole block configurations, which themself might contain references which need to be merged too.

That's where we are now with paragraphs and it works quite well, but there is at least one major limitation. It is currently not possible to prepare new paragraphs (or reorder/remove) in a draft EN translation and then add translations based on that draft, because when creating a draft for DE, it will be based on the default revision, not the EN draft. layout builder should have the same problem, just replace "add paragraph" with "change layout/place block". I should have tried to find solutions for that for months now, but I'm kinda stuck. See #3004099: Allow to translate paragraphs from pending revisions issue for that and #3007233: Draft translations should be based on the latest revision of the source language, not the published version for the core issue that I opened as a possible option for resolving that.

Here is a re-roll of #125. There was a lot changes since then so I still need to double check but wanted to get the work started again here and see what tests fail.

Still needs work. Translations of labels is working but not for inline blocks.

Removing "Needs follow-up" as now this is a child issue of #3044386: [META] Make Layout Builder layouts translatable which has other related issues

Cleaning up the last patch

1. +++ b/core/modules/layout_builder/src/Form/BlockPluginTranslationFormTrait.php
   @@ -0,0 +1,38 @@
   +trait BlockPluginTranslationFormTrait {
   

   This trait was no longer used.

2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
   @@ -130,6 +150,10 @@ public function defaultConfiguration() {
   +    if (!$this->isNew) {
   +      // Get the active block for editing purposes.
   +      $block = $this->entityRepository->getActive('block_content', $block->id());
   +    }
   

   need to check here for $block->isNew() because $this->isNew actually determines if form is adding a new block. If the block has been added and then you edit the block before saving the layout the block will still be new and there will be no active.

3. \Drupal\Tests\layout_builder\Kernel\OverridesSectionStorageTest::testAccess still has some failures I am figuring out.
   

   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -415,7 +426,7 @@ public function access($operation, AccountInterface $account = NULL, $return_as_
   -    return $result->andIf(AccessResult::allowedIf(!($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation())))->addCacheableDependency($entity);
   +    return $result->andIf(AccessResult::allowedIf($this->isDefaultTranslation() || ($entity instanceof TranslatableInterface && $this->isOverridden())))->addCacheableDependency($entity);
   

   It might be related to \Drupal\Core\Entity\EntityBase::getCacheTagsToInvalidate()

   Not factoring in language so since the test already calls access() the result will be cached.

4. Clean up some test fails because of case changes in links.

Had to look at \Drupal\Tests\content_translation\Kernel\ContentTranslationEntityBundleInfoTest::testFieldSynchronizationWithDisabledBundle() to determine how to get a kernel test to work with translations. The problem before was $entity->isTranslatable() was always returning FALSE which affected \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::isTranslatable().

+++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
@@ -54,7 +57,9 @@ protected function setUp() {
     $this->installEntitySchema('entity_test');
+    $this->installEntitySchema('entity_test_mul');
     $this->installEntitySchema('user');
+    $this->installEntitySchema('configurable_language');
 

configurable_language is a config entity, that's not something you need to install.