Update the Layout field to non-translatable when possible.

Per discussion, tagging for now.

Doing 3 and 4 is probably best. Those are simple queries. However, we'd need adequate test coverage for the different cases. So long as we're not blocked on issues with getting update path fixtures for these working, I think it's a great approach.

If the test fixtures for that are too problematic, and we want to go the route of 2 (or an and condition of 1 and 2), then we're in luck, because of #3043650: Exception when uninstalling CT when there are existing translations. If it weren't for that, there'd be the risk of orphaned translations, but since you can't uninstall CT if you have existing translations, we're (sort of) covered. Kind of sketchy relying on the fact that CT has a critical bug though. And I suppose there's the edge case possibility of someone having translations that they create via some other module (not via CT), though that's what anding the if with 1 would mitigate.

This is looking great, the base test class is a nifty solution! I manually tested the fixtures by running tests and stopping them before the ::runUpdates() call: after manually running the update everything in the UI and in the database looks as expected.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -186,3 +191,76 @@ function layout_builder_post_update_update_permissions() {
   +    $results = $query->execute();
   ...
   +  $results = $query->execute();
   

   If we array_intersect() the two full result sets we are actually implementing solution 5, I think. IMO we could do that, as this would further reduce the cases that are left out of luck. However we may need to pay attention to memory consumption, if the two result sets are very big (e.g. one million rows), here's the output of a test script I quickly put together:

   26661400 bytes
   33558608 bytes
   33558608 bytes
   33558608 bytes
   191301856 bytes
   

2. +++ b/core/modules/layout_builder/tests/fixtures/update/layout-builder-translation.php
   @@ -0,0 +1,150 @@
   +
   +/**
   + * Loop over test cases defined in the test class.
   + *
   + * @see \Drupal\Tests\layout_builder\Functional\Update\Translatability\MakeLayoutUntranslatableUpdatePathTestBase
   + */
   

   Nit: let's use regular inline comments (//) here.

3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -9,6 +9,11 @@
   +use Drupal\Core\Entity\Sql\SqlContentEntityStorage;
   +use Drupal\Core\Entity\Sql\DefaultTableMapping;
   

   Unused

4. +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/LayoutFieldTranslateUpdateCase1.php
   @@ -0,0 +1,46 @@
   +      'title' => 'Page Test - Spanish title',
   
   +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/LayoutFieldTranslateUpdateCase3.php
   @@ -0,0 +1,46 @@
   +      'title' => 'Page Test - Spanish title',
   

   I think we can remove this line, it's not used in the two tests and makes things slightly confusing.

5. +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/LayoutFieldTranslateUpdateCase3.php
   @@ -0,0 +1,46 @@
   +class LayoutFieldTranslateUpdateCase3 extends MakeLayoutUntranslatableUpdatePathTestBase {
   

   I couldn't help but noticing we are missing case 2: are you working on another test? :)

6. +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/MakeLayoutUntranslatableUpdatePathTestBase.php
   @@ -0,0 +1,85 @@
   + * This method of testing is used instead of a @dataProvider method because test
   

   Can we use "data provider" instead, just in case some weird parsing bug leads to even weirder test failures?

7. +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/MakeLayoutUntranslatableUpdatePathTestBase.php
   @@ -0,0 +1,85 @@
   +abstract class MakeLayoutUntranslatableUpdatePathTestBase extends UpdatePathTestBase {
   +
   +
   

   Nit: extra newline :)

Patch looks good to me and works as advertised. This is RTBC as far as I'm concerned, unless we want to try and implement solution 5.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -186,3 +189,107 @@ function layout_builder_post_update_update_permissions() {
   +    $update_field_storage = TRUE;
   ...
   +          $update_field_storage = FALSE;
   

   Would make me feel better if this started as FALSE and was later explicitly set to TRUE

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -186,3 +189,107 @@ function layout_builder_post_update_update_permissions() {
   +      // Set the field storage to untranslatable if possible. This removes all
   +      // layout fields for the entity type from the Content Translation
   

   "all layout fields" isn't it just one?

3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -186,3 +189,107 @@ function layout_builder_post_update_update_permissions() {
   + *   The entity type.
   ...
   + *   The entity type.
   

   Nit: The entity type ID

4. +++ b/core/modules/layout_builder/tests/src/Functional/Update/Translatability/MakeLayoutUntranslatableUpdatePathTestBase.php
   @@ -0,0 +1,84 @@
   +abstract class MakeLayoutUntranslatableUpdatePathTestBase extends UpdatePathTestBase {
   

   This is such a nice clean way to do this. +1

+1 to this being RTBC after those changes

Works for me! Thanks so much

Adding credit

+++ b/core/modules/layout_builder/layout_builder.post_update.php
@@ -186,3 +189,104 @@ function layout_builder_post_update_update_permissions() {
+        if (!$field_config->isTranslatable()) {
+          $non_translatable_bundle_count++;
+          // The layout field is already configured to be non-translatable so it
+          // does not need to be updated.
+          continue;
+        }

Wouldn't it make sense to perform the same check on the field storage and skip looping over all the bundles if that's already FALSE?

@effulgentsia pointed out in Slack that unless some custom code changed the storage to FALSE it would always be TRUE, so I think committing a follow-up is not worth the effort.

Pushed to 8.8.x and 8.7.x.

The biggest benefit of this patch is that it updates things nicely for monolingual sites, so I updated the CR accordingly: https://www.drupal.org/node/3043164/revisions/view/11360219/11361603.

That it also handles some other easy cases, such as a bundle that enables layout overrides but doesn't have any, is a bonus.