Creating a new translation may delete translations with drafts

Tests for the 2 cases.

The last submitted patch, 8: 3329066-translation-drafts_failing-test-8.patch, failed testing. View results →

Only one of the tests are failing right now, is there something different about the second one?

@tim.plunkett, it almost looks like patch 8 is a tests-only patch? Way different than patch 2 and is mostly the added test that fails, does not include the code provided in patch 2.

I haven't tested the exact scenario as described however this patch might help:

#2951294-39: Sort out and fix language fallback inconsistencies →

@tim.plunkett The bug has different effects depending on if a new translation is published vs if the new translation is a draft so I tried to cover both cases with a test.

@joseph.olstad I took a look but I don't think these two are related. As far as I can tell this is specifically from the way the "add translation" form prepares the node object for saving.

Attaching a patch with the tests and an attempted fix.

smustgrave → credited JnLuC21 → .

Closed 🐛 Translation disappear when Content Moderation is enable. Closed: duplicate as a duplicate

Confirmed the issue following the steps in the issue summary

- Enable 2 additional languages: Spanish and French
- Add the default "editorial" workflow to the "Basic page" content type and enable translation
- Create a new published node in English with the body "English"
- Create a new published translation in Spanish with the body "Spanish"
- Add a new draft of the Spanish translation with the body "Spanish draft"
- Add a new published French translation (any body)
- Try and visit the published Spanish version of the node (should be /es/node/1)

I actually don't even see my spanish on /admin/content.

Applied the patch and it addressed the issue when I ran the steps again.

This is definitely critical as data appears lost and unrecoverable.

Nitpick:

diff --git a/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
new file mode 100644
index 0000000000..cf7084f8dc
--- /dev/null
+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
@@ -0,0 +1,165 @@\
[snip]
+    $this->assertFalse($this->container->get('state')->get('content_translation_test.translation_deleted', FALSE));
+  }
+
+}

I think we should drop the second argument (FALSE) in the $this->assertFalse

The issue summary update isn't sure that the proposed solution is correct, and nor am I.

Is the entity we're loading here definitely the right place to pull translations from? Tagging for issue summary update.

There was a delete translation bug caused by a bug in a specific version of the conflict module.

background information is here: 🐛 Original language entity content got overwritten when updating translated entity for multilingual site RTBC

If you have this bug make sure to upgrade the conflict module to the latest release in case you are using it.

Examine where it's comming from:
composer why drupal/conflict;

composer up drupal/conflict -W;

If this doesn't work, check your composer.json file or maybe override the version if you've got a dependency elsewhere not working.

Otherwise, keep reviewing , just thought I'd mention this because it was a critical bug in the conflict module fixed by 🐛 Original language entity content got overwritten when updating translated entity for multilingual site RTBC
.

Quickest/Easiest solution if you're affected by this is to either upgrade the conflict module or uninstall it.

@catch Thanks for the review. I think the only question about loading the entity is whether to load the translation from the default revision, or to load latest translation affected revision for each language. As far as I can tell, the way the patch is currently written saves data to the revision table in the same way as creating translations without the patch.

As for the core translation issue, there has been significant work put into what I would say is a related issue in the content_moderation module, consider testing/reviewing these patches:
#3007233-41: Draft translations should be based on the latest revision of the source language, not the published version →

issue summary has been updated.

I have read the comments and I see that Issue Summary has been updated, thanks! I see that #22 has not been addressed and I do not see a code review. It is not clear if @Spokje did a full review in #22.

Also catch asked a question in #23, which was answered in #26. I don't see agreement by anyone else on that point.

I then read the code, paying attention to comments, nothing else.

1. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
   @@ -0,0 +1,165 @@
   +    'language',
   

   Nit: can these be sorted?

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
   @@ -0,0 +1,165 @@
   +   * Test that creating a new translation does not delete a translation that has a draft.
   

   The summary line should be < 80 chars. See General considerations for API module parsing →

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
   @@ -0,0 +1,165 @@
   +   * Test that creating a new draft translation does not invoke translation delete hooks.
   

   Same here

4. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
   @@ -0,0 +1,165 @@
   +    $this->assertFalse($this->container->get('state')->get('content_translation_test.translation_deleted', FALSE));
   

   It would help to have a comment here explaining what is being tested, as what done in the other test.

I have updated credit but it still should be checked.

Thanks @quietone and @Spokje for the reviews. I've updated the patch to address the feedback.

Feedback appears to have been addressed from #32

@dabblela, thank you for updating the patch.

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationNewTranslationWithExistingRevisionsTest.php
@@ -0,0 +1,166 @@
+    // If the translation delete hook was incorrectly invoked, the state variable would be set.

The 80 character wrapping applies to all comments, so this needs to be wrapped as well.

I have time, so I have updated the patch to wrap that line. I also added a few line breaks to the test for consistency and readability.

I double checked the interdiff and the changes I made are formatting and line wrapping. I am restoring RTBC.

Really change the status this time.

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -68,6 +68,30 @@ public static function create(ContainerInterface $container) {
+
+    // Add translations from the default entity.
+    /** @var \Drupal\Core\Entity\ContentEntityInterface $default_revision */
+    $default_revision = $storage->load($entity->id());
+    // Check the entity isn't missing any translations.
+    $languages = $this->languageManager()->getLanguages();
+    foreach ($languages as $language) {
+      $langcode = $language->getId();
+      if ($entity->hasTranslation($langcode) || $target->getId() === $langcode) {
+        continue;
+      }
+      $latest_revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $langcode);
+      if ($latest_revision_id) {
+        if ($default_revision->hasTranslation($langcode)) {
+          $existing_translation = $default_revision->getTranslation($langcode);
+          $existing_translation->setNewRevision(FALSE);
+          $existing_translation->isDefaultRevision(FALSE);
+          $existing_translation->setRevisionTranslationAffected(FALSE);
+          $entity->addTranslation($langcode, $existing_translation->toArray());
+        }
+      }

Sorry I think this needs more of an explanatory comment, something like:

// Once translations from the default entity are added, there may be additional, draft translations that don't exist in the default revision at all yet. When this is the case, add those translations too so that we are only ever cumulatively adding translations and not deleting them.

Updated the comment on the translation controller. Thanks for the review @catch

Restoring RTBC with the comment update.

I wonder if the above patch will fix this very serious contrib bug that the maintainers have wrecklessly ignored?

🐛 Original language entity content got overwritten when updating translated entity for multilingual site RTBC

Made a slight tweak to the comment on commit, mostly line wrapping but also changing 'default entity' to 'default revision'.

diff --git a/core/modules/content_translation/src/Controller/ContentTranslationController.php b/core/modules/content_translation/src/Controller/ContentTranslationController.php
index 6dde70e09c..b906d49803 100644
--- a/core/modules/content_translation/src/Controller/ContentTranslationController.php
+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -71,9 +71,10 @@ public function prepareTranslation(ContentEntityInterface $entity, LanguageInter
     /** @var \Drupal\Core\Entity\ContentEntityStorageInterface $storage */
     $storage = $this->entityTypeManager()->getStorage($entity->getEntityTypeId());
 
-    // Once translations from the default entity are added, there may be additional draft
-    // translations that don't exist in the default revision. Add those translations
-    // too so that they aren't deleted when the new translation is saved.
+    // Once translations from the default revision are added, there may be
+    // additional draft translations that don't exist in the default revision.
+    // Add those translations too so that they aren't deleted when the new
+    // translation is saved.
     /** @var \Drupal\Core\Entity\ContentEntityInterface $default_revision */
     $default_revision = $storage->load($entity->id());
     // Check the entity isn't missing any translations.

@joseph.olstad it looks like the conflict module has had one commit in the past two years and only has an alpha release for Drupal 10, that suggests that it's unmaintained, not that the maintainers are 'recklessly ignoring' a bug. Not least because if this core patch does fix it, then they don't need to do anything anyway.

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

@catch the conflict module maintainers are still active and have responded. I haven't had a chance to evaluate the above fix to see if it solves the conflict module issue. We ended up uninstalling the conflict module due to the severity of the issue.

Automatically closed - issue fixed for 2 weeks with no activity.