Remove the layout tab from translations because Layout Builder does not support translations yet

Tagging

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -362,6 +362,8 @@ public function save() {
   +    $result = $result->andIf(AccessResult::allowedIf(!($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation())))->addCacheableDependency($entity);
   

   Can we add an inline comment to explain that we do not support translations yet and maybe a @todo pointing to #2946333: Allow synced Layout override Translations: translating labels and inline blocks?

2. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
   @@ -0,0 +1,217 @@
   +    $entity_url = $this->entity->toUrl('canonical')->toString();
   ...
   +    $entity_url = $this->entity->toUrl('canonical')->toString();
   

   No need for canonical here.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
   @@ -0,0 +1,217 @@
   +    $this->drupalLogin($this->translator);
   

   I guess we should restore the previous user after the translation has been created.

[duplicate comment]

Here's an update path test.
It passes by itself, and fails without the update path.
Once the update path is written, it should fail with that but without the fix, and pass all combined.
FAIL patch is the interdiff.

At least they were different fails...
Looking into it

1. +++ b/core/modules/layout_builder/tests/fixtures/update/layout-builder-translation.php
   @@ -0,0 +1,126 @@
   +$node_field_data['revision_translation_affected'] = NULL;
   ...
   +$node_field_revision['revision_translation_affected'] = NULL;
   

   Why NULL? These will be 1 if you create a new translation while updating the default revision. It likely doesn't matter but better safe than sorry :)

2. +++ b/core/modules/layout_builder/tests/src/Functional/Update/MakeLayoutUntranslatableUpdatePathTest.php
   @@ -0,0 +1,110 @@
   +    $this->drupalGet("admin/structure/types/manage/$type/display");
   

   Why do we need this?

3. +++ b/core/modules/layout_builder/tests/src/Functional/Update/MakeLayoutUntranslatableUpdatePathTest.php
   @@ -0,0 +1,110 @@
   +    $this->drupalGet('es' . $node->toUrl()->toString());
   

   Ideally we would do
   $node->getTranslation('es')->toUrl(), but we'd need to reload the node first...

Clarifying.

ExpectationException: No link containing href /es/node/9/layout found

Looks like a "layout" tab is available when viewing entity translations. This appears to be a regression compared to #10.

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

I suspect this is the change that introduced that. I think it's missing a negation. Here's a patch to fix that.

EDIT: oh wait, I think that this is an intentional change, per #15.3 and this addition in #15:

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -100,6 +101,12 @@ protected function init(FormStateInterface $form_state) {
   public function buildForm(array $form, FormStateInterface $form_state, SectionStorageInterface $section_storage = NULL) {
+    $entity = $this->getEntity();
+    if ($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation()) {
+      return [
+        '#markup' => $this->t("Oh you would like to translate your layout? That's cool. I get it. Well but it's a little more complicated than that. So, well we tried. But you know there is translation and then localization and well it is really hard to know what you want. Also like what if you want both but maybe only sometimes. And what if you want to switch between one and the other on the same content. Or like what if you want to have some languages be translated and then some languages localized and on THE VERY SAME CONTENT. Like come on can't you see how this is really really complicated. And I haven't mentioned defaults and I wasn't going to mention them. But you know you are probably going to want to translate those and well maybe even localize them. Come on give us a break. But anyways we tried like I said. And you know we are going to do it and it is going to be awesome. So sorry you will just have to wait a little longer. Sorry."),
+      ];
+    }

— why this sudden change of direction tough? I don't see this mentioned in the issue summary or the comments here to do this?

+++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
@@ -103,8 +103,14 @@ protected function init(FormStateInterface $form_state) {
+      // layout_builder_post_update_make_layout_untranslatable() could set it

"could NOT set it", right?

Let's also document how that can happen? Right now it sounds rather mysterious. Ominous even.

Review of #15:

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   +  $a = 'b';
   

   mmh

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   +      if ($entity_type_has_translated_layouts === FALSE) {
   

   Nit: !$entity_type_has_translated_layouts?

3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   + * Determines if there are no translated layout for a bundle.
   

   "translated layoutS"?

4. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   +        $join_condition = "d.$revision_key = f.revision_id";
   ...
   +        $join_condition = "d.{$entity_type->getKey('id')} = f.entity_id";
   

   We're missing a condition on the langcode column being the same (langcode is an entity key, so in the data table the actual column name might be different).

5. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   +        return FALSE;
   

   Shouldn't this be TRUE? If there is no data table the can't be any translation.

6. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -162,7 +167,88 @@ function layout_builder_post_update_section_third_party_settings_schema() {
   +      $select->condition('d.default_langcode', 0);
   

   default_langcode is an entity key.

7. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -100,6 +101,12 @@ protected function init(FormStateInterface $form_state) {
   +        '#markup' => $this->t("Oh you would like to translate your layout? That's cool. I get it. Well but it's a little more complicated than that. So, well we tried. But you know there is translation and then localization and well it is really hard to know what you want. Also like what if you want both but maybe only sometimes. And what if you want to switch between one and the other on the same content. Or like what if you want to have some languages be translated and then some languages localized and on THE VERY SAME CONTENT. Like come on can't you see how this is really really complicated. And I haven't mentioned defaults and I wasn't going to mention them. But you know you are probably going to want to translate those and well maybe even localize them. Come on give us a break. But anyways we tried like I said. And you know we are going to do it and it is going to be awesome. So sorry you will just have to wait a little longer. Sorry."),
   

   We should add a dev-speak translation on localize.drupal.org so we don't lose this ;)

(#14 is still TODO)

I looked into the failing test and the problem is that system_update_8402() only added the base field definition but it didn't update the entity type definition as well to add the entity key. We need a separate issue to fix that..

While I was here, I cleaned up the update helper function and a few coding standards issues.

Not sending this patch to the testbot because it will have the same fails.

Talked to @amateescu in Slack, he's opening an issue tonight to fix the problem he discovered in #26, when he's back at his laptop. He knows exactly what needs to change obviously, so having him do it would result in the clearest and hence easiest to commit issue+patch 🙂

Here's the issue that fixes the missing revision_translation_affected entity key: #3042993: Translatable and revisonable installed entity type definitions are missing the 'revision_translation_affected' entity key, and a combined patch with #26.

I think the problem you described in #28 is tracked over here #2933202: EntityQuery returns incorrect results for notExists condition where one translation matches but others do not, I'll try to reproduce this scenario locally and see if we can tweak the entity query to work around that bug :)

1. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -90,6 +91,18 @@ protected function init(FormStateInterface $form_state) {
   +        // @todo Create actual message to that they have a translated layout values.
   +        '#markup' => $this->t("Whoops we couldn't set field to non-translatable. Our bad."),
   

   Ok, we should really come up with something final now :)

   What about "Layout builder does not support layout localization, read the documentation for more details."?

2. +++ b/core/modules/layout_builder/tests/fixtures/update/layout-builder-translation.php
   @@ -0,0 +1,168 @@
   +  $node_field_data['revision_translation_affected'] = NULL;
   ...
   +  $node_field_revision['revision_translation_affected'] = NULL;
   

   This need to be 1, see #14.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
   @@ -0,0 +1,217 @@
   +    $entity_url = $this->entity->toUrl('canonical')->toString();
   

   Nit: "canonical" is not needed here.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationTest.php
   @@ -0,0 +1,217 @@
   +    // Create a translation.
   +    $this->drupalLogin($this->translator);
   

   Can we restore the previous user after creating the translation to perform a more consistent test?

5. +++ b/core/modules/layout_builder/tests/src/Functional/Update/MakeLayoutUntranslatableUpdatePathTest.php
   @@ -0,0 +1,142 @@
   +    $this->assertEquals(
   +      $translated_layout_expected,
   +      FieldConfig::loadByName('node', $type, OverridesSectionStorage::FIELD_NAME)->isTranslatable(),
   +      $translated_layout_expected ? "Field on $type not set to translatable." : "Field on $type set to translatable."
   +    );
   

   Can we add an assertion also for the storage definition?

6. +++ b/core/modules/layout_builder/tests/src/Functional/Update/MakeLayoutUntranslatableUpdatePathTest.php
   @@ -0,0 +1,142 @@
   +    $this->drupalGet("admin/structure/types/manage/$type/display");
   

   Is this needed?

7. +++ b/core/modules/layout_builder/tests/src/Functional/Update/MakeLayoutUntranslatableUpdatePathTest.php
   @@ -0,0 +1,142 @@
   +    if ($translated_layout_expected) {
   +      $assert_session->linkByHrefExists($layout_href);
   

   Can we add an assertion to check that the warning message is displayed?

@tedbow, #35:

1: Because different layouts per language is localization and not translation, but nbd: users might not be able to tell the difference.
5: Rrright
7: The string would be translated only if the test enabled the locale module, so asserting the English string should be fine.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -233,24 +233,20 @@ function _layout_builder_no_translated_layouts($entity_type_id, $bundle) {
          $count = (int) $select->countQuery()->execute()->fetchField();
          return empty($count);
   

   I was wondering whether it wouldn't be better to switch to a LIMIT 0,1 ranged query to improve performance on very large data sets.

2. +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -99,8 +99,7 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
   +        '#markup' => $this->t("Layout builder does not support layout translations, read the <a href='https://www.drupal.org/node/3043164 >documentation</a> for more details."),
   

   Missing closing quote in the link tag, also we normally use double quotes for HTML attributes.

I haven't reviewed the whole patch yet, but just some drive-by nits of the post_update:

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -167,3 +173,86 @@ function layout_builder_post_update_fix_tempstore_keys() {
   +    if (isset($field_infos['layout_builder__layout']['bundles'])) {
   +      foreach ($field_infos['layout_builder__layout']['bundles'] as $bundle) {
   +        if (_layout_builder_no_entities_with_layouts_and_translations($entity_type_id, $bundle)) {
   +          $field_config = FieldConfig::loadByName($entity_type_id, $bundle, OverridesSectionStorage::FIELD_NAME);
   

   Any reason not to use the OverridesSectionStorage::FIELD_NAME constant in the first two lines as well?

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -167,3 +173,86 @@ function layout_builder_post_update_fix_tempstore_keys() {
   + * Determines if there are no translated layouts for a bundle.
   ...
   + * @return bool
   + *   TRUE if there no translated layout, otherwise FALSE.
   + */
   +function _layout_builder_no_entities_with_layouts_and_translations($entity_type_id, $bundle) {
   

   If "translated layout" is an accurate description, per the docs, then can we also change the function name to _layout_builder_no_entities_with_translated_layouts()? I think the current name of "layouts_and_translations" is inaccurate, because I think a site can have entities which have both layout overrides (in the default language) and translations (without a translation of the layout), and for such a site, this function returns FALSE. Is that correct?

+++ b/core/modules/layout_builder/layout_builder.module
@@ -174,10 +175,13 @@ function layout_builder_entity_build_defaults_alter(array &$build, EntityInterfa
+    // runs after content_moderation's alter so that it can add a warning to the

This is actually referring to Content Translation, right?

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -350,3 +353,76 @@ function layout_builder_quickedit_render_field(EntityInterface $entity, $field_n
   +    // layout translations are not supported..
   

   Nit: double trailing dot

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -350,3 +353,76 @@ function layout_builder_quickedit_render_field(EntityInterface $entity, $field_n
   +          $layout_field_checkbox['#description'] = t('<strong>Warning:<strong> Layout Builder does not support layout translations, read the <a href="https://www.drupal.org/node/3043164" >documentation</a> for more details.');
   

   This is not great from a visual standpoint, what about appending it to the operation column instead and adding the messages messages--warning classes? See the screenshot below for a preview:
   

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -350,3 +353,76 @@ function layout_builder_quickedit_render_field(EntityInterface $entity, $field_n
   +        'description' => t('Layout Builder does not support layout translations, read the <a href="https://www.drupal.org/node/3043164" >documentation</a>. Layout translation support is currently enabled for the following. @bundle_issues', ['@bundle_issues' => $bundle_issues]),
   

   The last sentence seems truncated, I guess it meant to be something like:

   "the following items."

+++ b/core/modules/layout_builder/layout_builder.module
@@ -350,3 +353,76 @@ function layout_builder_quickedit_render_field(EntityInterface $entity, $field_n
+          $field_config = FieldConfig::loadByName($entity_type_id, $bundle, OverridesSectionStorage::FIELD_NAME);

There is no corresponding use statement for FieldConfig, I was getting the following error on the status report:

Error: Class 'FieldConfig' not found in layout_builder_requirements() (line 404 of core/modules/layout_builder/layout_builder.module).

Also why not moving this to layout_builder.install?

NW for the last error.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -167,3 +173,93 @@ function layout_builder_post_update_fix_tempstore_keys() {
   +      // translation, nor do we care if it is for the same revision as the
   +      // revision that has a translation. Therefore, join only on the entity ID
   

   Has it already been discussed in this issue why we don't care about matching on revision? For example, if I had layout overrides before having translations. Then I stopped having layout overrides and only after that started having translations, then wouldn't it be safe to return TRUE? But if there are other scenarios where layout overrides and translations across different revisions are problematic, then yes, it would be safer to not join on revision ID. I'm just wondering if we know of such a scenario, or if we're just being extra cautious.

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -167,3 +173,93 @@ function layout_builder_post_update_fix_tempstore_keys() {
   +  // If we were not able to execute the query assume there are translated
   +  // layouts.
   +  return FALSE;
   

   s/translated layouts/translations and layouts/

   What about the case where the entity type or bundle isn't translatable? How are we handling that case in post_update? Or is there a reason that we don't have to?

Re #35:

This is because in 8.6.x if you created a layout override for the default language after you created a translation then the translation would have no layout override translation(and couldn't make one). But this does not mean that entity view of the translation would use the default translations layout override. This actually means that because the layout override storage would have the translated entity and the field is translated then OverridesSectionStorage::isOverridden() would return false and the translation would use the bundle's default layout.

Good catch! So +1 to #41.1. I confirmed that this is the standard behavior for adding translatable fields to content that already has translations. For example, create a node with 2 translations, then add a field to that bundle, the go to /admin/config/regional/content-language and make the newly added field translatable. If you now edit the node in its default language and put some value into the new field, it only gets applied to the default language. The existing translation, when both viewed and edited, continues to have that new field be empty. Which is different than when you create a new translation, but I can see why this difference is desirable. Given all that, OverridesSectionStorage::isOverridden() is acting correctly (or at least, consistently with translatable fields in general), so you're right about our post_update needing to query for "layouts and translations" rather than "translated layouts".

Soooo… @tim.plunkett, @tedbow, and @effulgentsia walked me through the current patch and… I got … very confused at the direction this patch has taken. I was sitting here believing this would be a 10-line patch to just remove the Layout tab from the non-default translation. :D

I know Layout Builder is beta, and therefore we care about the data integrity here… but this patch seems like it's going WAY over the top on hand-holding and putting permanent code in stable core for what I understand is being an issue that only affects a small portion of sites who used Layout Builder before 8.7.0-beta (when it was experimental) and are also multilingual.

It seems like we should be focusing on making sure this condition doesn't happen to new people (so do what the issue title says, and remove access to the tab), and making sure there's something easy to Google for those early adopters with multilingual sites who are suddenly missing a tab. Normally, it seems like we would just put an entry in the release notes that links to a site builder CR for a bug like this, versus codifying something in our UI for the rest of all of time for a temporary condition.

@tedbow had one concern, which was that if a site ever used Layout Builder before 8.7.0-beta, and makes it multilingual later, they will hit the same problem. For this, we could file a follow-up to add a (small) update path to make sure that if a site doesn't have any translations (apparently that's tricky to figure out), then mark the field untranslatable.

I completely agree with the proposal above in #51.

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -362,6 +362,8 @@ public function save() {
   +    $result = $result->andIf(AccessResult::allowedIf(!($entity instanceof TranslatableInterface && !$entity->isDefaultTranslation())))->addCacheableDependency($entity);
   

   This deserves a code comment

2. +++ /dev/null
   @@ -1,114 +0,0 @@
   -class LayoutBuilderMultilingualTest extends BrowserTestBase {
   

   Is there any value in keeping this test but changing the assertions to reflect the new reality? If not, this is fine

3. +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
   @@ -79,14 +81,21 @@ public function testAccess($expected, $operation, $is_enabled, array $section_da
   +      $display
   +        ->setOverridable()
   +        ->save();
        }
   -    $display
   -      ->setOverridable()
   -      ->save();
   

   I don't quite remember why we made this change.
   But doesn't the ->save() still need to live outside the if()?

So #51 is proposing to limit this patch to only:

• Setting the layout $field and $field_storage to not translatable when overrides are newly enabled for bundles and entity types.
• Removing the Layout tab from translations, even if it's for an entity type/bundle where the layout field is translatable (due to having been during 8.6).

It's proposing to not do any of:

• Adding an update path to change people's existing layout fields to be untranslatable.
• Adding a status report error/warning for those people.
• Adding a warning on the "Layout" row of the content translation configuration page for those people.
• Retaining the Layout tab from translations for those people, and having it tell you it doesn't work.

I agree with punting on the update path. It's getting too close to beta, and we're still having problems with it. I'd rather provide it when we're confident about it, whether that's for RC, in a patch release, in 8.8, or in a contrib module. Although https://www.drupal.org/core/experimental#beta says that beta module provide update paths, we're dealing with a case where this patch isn't breaking anything by lack of an update path. It's leaving an existing site functioning in the same way that it already was. And someone who wants the better behavior, they can go to the content translation configuration page and uncheck the box.

FWIW, I'm also ok with punting on the UI warnings, especially if a product manager doesn't think they're necessary.

Totally +1 on #51 as well, I had the same feeling that this was going too far: the scenarios we were accounting for feel way distant from the desired translation support, at least according from the feedback we were able to gather so far. For that reason I doubt there are many sites that are intentionally relying on the current behavior. Those can alter the tab access control and translation creation logic to restore it, we can even add some code snippets to the CR to explain how to do that.

For sites actually being hurt by this behavior, the following workaround is definitely viable:

And someone who wants the better behavior, they can go to the content translation configuration page and uncheck the box.

And if in a follow-up we can provide an update to make fields untranslatable where there's no risk of changing the site behavior unexpectedly, all the better.

@tim.plunkett, #53:

I'm not sure it's worth keeping that test around: the current logic is all covered by LayoutBuilderTranslationTest, at least from a functional perspective. I'd rather add a small kernel test to cover layout_builder_entity_translation_create(). Other than that and the remaining bullets in #53, the patch seems ready to go to me (reroll aside).

@plach pointed out that the second param of addTranslation is the key here: in the UI workflow the entity values of the default translation are passed in. Doing that here makes the test work as expected.

FAIL patch comments out the one line in layout_builder_entity_translation_create()

This looks great to me!

I have a couple of very minor non-blocking remarks that it would be nice to address if we happen to reroll this patch:

1. +++ b/core/modules/layout_builder/tests/src/Kernel/TranslatableFieldTest.php
   @@ -0,0 +1,86 @@
   +    // Create a translation.
   +    ConfigurableLanguage::createFromLangcode('es')->save();
   +
   +    LayoutBuilderEntityViewDisplay::create([
   +      'targetEntityType' => 'entity_test',
   +      'bundle' => 'entity_test',
   +      'mode' => 'default',
   +      'status' => TRUE,
   +    ])
   +      ->enableLayoutBuilder()
   +      ->setOverridable()
   +      ->save();
   +
   +    FieldStorageConfig::loadByName('entity_test', OverridesSectionStorage::FIELD_NAME)
   +      ->setTranslatable(TRUE)
   +      ->save();
   +    FieldConfig::loadByName('entity_test', 'entity_test', OverridesSectionStorage::FIELD_NAME)
   +      ->setTranslatable(TRUE)
   +      ->save();
   

   I'd move this logic to the ::setUp() method.

2. +++ b/core/modules/layout_builder/tests/src/Kernel/TranslatableFieldTest.php
   @@ -0,0 +1,86 @@
   +    // Layout translations are not supported.
   

   This is testing that "Per-language layouts are not supported". I think what we mean with layout translation is translating the layout component textual elements.

Also, we need a few updates to reflect the latest change in direction.

Fixed those, and applied the same change as 2 on the docs for our hook.

Also we need an addition for the release note.

In order for custom code to be able to restore translation functionality, splitting the access check out to a protected method.

Ok, aside from the pending updates this looks RTBC to me.

Added a release note snippet to the IS.

plach signed off in #64, pending the IS/CR/RN updates.
Confirming that this is now RTBC.
Thanks!

The IS and snippet look good to me, I performed a couple of small tweaks to the CR.

RTBC +1

OK I'm really concerned that the update path was abandoned here. We're knowingly deleting site data for sites that unknowingly started a translation before, with no workaround or other option for those sites to preserve their data.

The release note says:

Layout Builder overrides are now stored in non-translatable fields. This means that on entities with overridable layouts, the Layout tab is only available when viewing the entity in its default language (not from a translation) and layout changes made from there apply to all translations. For sites that installed Layout Builder and enabled layout overrides for some entity bundles while it was still Experimental (prior to 8.7.0-beta1), this release retains the site's existing translatability configuration of those fields. See the change record for more details.

Is that still true? It contradicts the patch and recent discussion in the issue. Site owners need to know the impact this will have and data loss/orphaned data is a pretty serious disruption.

For me, whatever followup issue to fix existing sites is a stable blocker. I've reached out to catch for a second opinion.

After reading the CR carefully and discussing with @tedbow and @effulgentsia, I'm really concerned with this part:

For sites that enable Content Translation at a later time
Sites that had layout overrides enabled before 8.7.0 and some time later enable the Content Translation module will also need to take the same action above to get the current module functionality for overrides

I asked @effulgentsia if this meant what I thought it did, and he confirmed:

Unfortunately, it also applies to if they use CT later. So for example, if they added LB in 8.6 and enabled layout overrides for articles. And they ignore this CR cause their site is monolingual. And then in a year they install CT. Then they'll have this same broken behavior at that time (for articles). Yesterday, Ted suggested an update path that at the very least handles this case (i.e., set the fields to not translatable if the site is currently monolingual). I think we could get that written pretty quickly, depending on how easy or hard it is to get the right test fixtures in place for it.

To me this is very much a data integrity issue. There is no way that sites that started using Layout Builder in 8.6 are going to find this one buried bullet point in the 8.7.0 release notes when they decide to enable Content Translation next year. There's no workaround, no action they can take now. We need either in-context messages about this to the user in the UI, or an upgrade path to fix it.

What about committing this patch as-is, and making #73 a stable-blocking follow-up? The patch here doesn't regress anything. Maybe it's just not enough to mark the module stable yet?

Ted said he would file a separate issue for the update path. For me, an update path to address at least #73 (update the field on monolingual sites) might be stable-blocking. @effulgentsia also mentioned that part of the problem has been upgrade path tests, but for a data integrity issue like this, I would prefer a thorougly manually tested upgrade path with tests descoped to a critical followup, rather than no upgrade path at all. But we can discuss in the other issue if it does get split out.

Crossposted with #74. I think one of two things is stable blocking: UI messages to the user about what happens when this field is marked translatable, or an upgrade path. I'd be okay with followup issues for both, tagged as stable blockers, but that we could decide to descope following discussion or if one or the other lands.

I created #3043646: For sites that have made layout overrides prior to 8.7.0 or sites that manually enable translation of the layout override field, add UI warnings and it is linked to this issue.

I updated the release note to be a bit clearer about not recommending the old behavior, and worked on an update of the CR with @effulgentsia.

@tedbow, @tim.plunkett, @effulgentsia and I discussed #73 and agreed that this can go in on its own, since it mostly solves the data integrity problem for sites that haven't already enabled layout overrides somewhere. However, the two followups #3043646: For sites that have made layout overrides prior to 8.7.0 or sites that manually enable translation of the layout override field, add UI warnings and #3043651: Update the Layout field to non-translatable when possible. remain as stable blockers for the time being, until we can decide how to handle existing overrides as well as sites that might enable translation on the field in the future.

#3042089: Update Layout Builder functional javascript tests now that local tasks are not required by the UI. removed one line in a file we are removing. Straight reroll.

Crediting reviewers.

Pushed to 8.8.x and 8.7.x and published the CR. See you in the two followups from #79.

+1 on #79, sounds like a very reasonable way forward, thanks!