File records, files themselves lost in translation

This is a thoughtful report and it is Critical unless someone can show it is not.

Could reproduce the data loss, but with an additional detail for point 1: the file isn't deleted from file_managed when creating the first translation, the entry is still there, but the status is set to 0 (however, at some point, cron will clean this up of course). When creating the second translation, it's gone from the managed_file table, as well as from the file system. What's even more annoying is that there's still a reference in the dedicated reference table of the field to a non existing record.

So you definitely need to test this with 3 translations, subtle things start kicking in at the first translation, the second translation is even worse.

Trying to write a test to expose the failure.

(edit, changed the first sentence regarding the detail, should recheck the usage table at some point)

Test that exposes the data loss.

Pasting the asserts that will fail in the code for reference.

1. +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
   @@ -0,0 +1,155 @@
   +    // Ensure the file status of the first file permanent.
   +    $file = File::load($first_fid);
   +    $this->assertTrue($file->isPermanent());
   

   This fails after the first translation

2. +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
   @@ -0,0 +1,155 @@
   +    // Ensure the first file still exists. Calling isPermanent() would crash
   +    // the test suite.
   +    $file = File::load($first_fid);
   +    $this->assertTrue(!empty($file), 'First file still exists.');
   +    // This inspects the HTML after the post of the translation, the file
   +    // should be displayed on the original node.
   +    $this->assertRaw('file--mime-text-plain');
   +
   

   This fails even more after the second translation

Some basic debugging: open FileUsageBase and comment out lines 36 & 37 and all will be fine (naturally). It's called from DatabaseFileUsageBackend(). We need to prevent decrementing it on the first translation here.

Related, but with different scenarios and on the image field: #2617174: Image field's file translation doesn't work as expected

Additional problem: after adding one translation and then deleting it again, the file you uploaded from the translation is not removed from the file_managed and file_usage table, but it is from the file system ..

This makes the test pass, however, I don't think the fix is right enough as I think it won't remove the file from the system in case you would edit the original source node, maybe other tests catch this one already, or I'll try to add additional tests.

Wow. Nice find. Adding tags. Adding issue summary template. Please fill in proposed resolution as well.

This should fix the fails in UserPictureTest - updated the proposed solution in the IS.
Added some references in the patch to a good movie :)

Note, as I've said already, I think the current patch now prevents from deleting the file from the system when you're deleting it from the source node (and probably from the translated node too), so we'll need an additional test and fix too.

Better assert

Ok, getting highly confused now, $translation_sync seems to be empty - back to drawing board it seems.

#2650468: Translation of image and file fields is setting file status to temporary and causes loss of data was marked a duplicate of this one.

I might be missing something here, but this makes the test pass. Lets see what else happens.

Basically, since we're called for every translation, we shouldn't fall back to the original translation of the entity. If the translation didn't exist, then there are no original ID's.

Wondering if there are reverse problems, with not removing usages when translations are removed or if this really works. Go testbot.

Amazing, good to have a fresh mind look at this one, thanks :)

Wondering if there are reverse problems, with not removing usages when translations are removed

I've been wondering this myself already too, I think it might be good to write additional tests for that, just to be sure.

Just added a fourth language test.

Sorry, mismatched the instruction for the new test to be add. Restarted:

• make sure to call the delete method for field items of removed translations in ContentEntityStorageBase::invokeFieldMethod()

Test:

• edit second translation, replacing file
• check 1./3. files untouched, new 2. file as permanent, old 2. file as temporary
• delete third translation
• check 1./new 2. files untouched, 3. file as temporary

Added test only patch.

Looks great. Since we're calling a lot of things on original here, I guess I just have one more question: I'm not sure exactly what happens in case we delete the 'original/source' node, does another one become the source ? Haven't looked exactly how/if content_translation handles that, so maybe just one additional test which deletes the original one then at the end - or maybe I'm even asking a stupid question :)

AFAIK, it's not possible to delete the source translation, that will delete the whole entity. Haven't tried, but I've seen issues about allowing that :)

That said, deleting the source translation and making sure that all files are then temporary makes sense.

I've also asked @plach to review this.

Added test deleting all translations.

IMHO we should also add a dedicated test outside of the file module to ensure that those methods are executed.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -453,6 +454,20 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
+    // We need to call the delete method for field items of removed
+    // translations.
+    if ($method == 'postSave' && !empty($entity->original)) {
+      $original_langcodes = array_keys($entity->original->getTranslationLanguages(FALSE));
+      foreach (array_diff($original_langcodes, $langcodes) as $removed_langcode) {
+        $translation = $entity->original->getTranslation($removed_langcode);

It is not entirely obvious that removing an entire entity will let $entity->getTranslationLanguages() return an empty list of translations, so really a dedicated test for that itself would be nice.

Yeah, a dedicated test makes sense, will see where we could do that. Maybe EntityTranslationTest.

It is not entirely obvious that removing an entire entity will let $entity->getTranslationLanguages() return an empty list of translations, so really a dedicated test for that itself would be nice.

It's not entirely obvious because there is no such thing. You can't save an entity without any translations, there's always the source translation that can't be removed. The only way to do that is to delete the whole entity and then the delete() methods are called directly.

@swentel: indeed, its currently not possible to delete the original language variant without deleting the whole entity with all the translations. There is an issue to allow to reassign the default translation to some other translation within the entity. See #2485499: Allow source translations to be removed and #2443991: Allow default_langcode field value to be changed.

I'd expect that won't allow you to actually delete a source translation either, you could just switch to another to make that the source and then delete the old source language?

So I think nothing changes for our case here and if it really does, then we now have enough test coverage here so it would fail and would have to be updated.

Uploaded patch:

• added delete() in Drupal\entity_test\Plugin\Field\FieldType\FieldTestItem
• dedicated test in Drupal\system\Tests\Entity\EntityTranslationTest

Feedback on the comments.

1. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Create entities for both translatable and untranslatable test fields.
   +    $values = array(
   +      'name' => $this->randomString(),
   

   We create a single entity, with both fields, not for.

2. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Remove the second and third langcodes form the translatable test field.
   +    $entity->removeTranslation('l1');
   

   We remove it from the entity, not the field.

3. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Ensure that for the translatable test field the second and third langcodes
   +    // are in the deleted languages list, except for the first one.
   +    // The untranslatable test field should be untouched.
   

   the "except ..." part doesn't really make sense to me, just leave that out.

   I'd also move the comment about untranslatable to the line where you assert that.

4. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Remove all translations for both test fields.
   +    $entity->delete();
   

   I'd write this instead:

   // Delete the entity, which removes all remaining translations.

5. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Check if the default and the first langcode for the translatable test
   +    // field is deleted.
   +    $actual = \Drupal::state()->get('entity_test.delete.translatable_test_field');
   

   // All languages have been deleted now.

6. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,90 @@ public function testFieldEntityReference() {
   +    // Check if the default langcode for the untranslatable test field is deleted.
   +    $actual = \Drupal::state()->get('entity_test.delete.untranslatable_test_field');
   +    $expected_untranslatable = ['en'];
   

   // The untranslatable field is shared and only deleted once, for the default lancode.

7. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/FieldTestItem.php
   @@ -115,4 +115,15 @@ protected function mustResave() {
   +    $language_to_delete = $this->getLangcode();
   +    $deleted_languages = \Drupal::state()->get('entity_test.delete.' . $this->getFieldDefinition()->getName()) ?: [];
   +    $deleted_languages[] = $language_to_delete;
   +    \Drupal::state()->set('entity_test.delete.' . $this->getFieldDefinition()->getName(), $deleted_languages);
   

   I think to_delete is a bit confusing. It's already being deleted. I'd just leave that variable out and put \Drupal::state() directly on the line where you add it to the array.

Done.

1. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -869,30 +869,29 @@
    
   -    // Remove the second and third langcodes form the translatable test field.
   +    // Remove the second and third langcodes form the entity.
   

   nitpick: form => from. missed that before.

2. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -869,30 +869,29 @@
    
        // Ensure that for the translatable test field the second and third langcodes
   -    // are in the deleted languages list, except for the first one.
   -    // The untranslatable test field should be untouched.
   +    // are in the deleted languages list.
        $actual = \Drupal::state()->get('entity_test.delete.translatable_test_field');
   

   first line unfortunately still > 80 characters.

3. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -869,30 +869,29 @@
    
   -    // Check if the default langcode for the untranslatable test field is deleted.
   +    // The untranslatable field is shared and only deleted once, for the default lancode.
   

   same here, > 80 characters.

Refactored.

Sorted $actual and $expected arrays for test comparison.

Overall this makes a lot of sense to me, thanks!

I have only one serious remark. It may be valid or not, but would be good to check.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -453,6 +454,20 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
   +      $original_langcodes = array_keys($entity->original->getTranslationLanguages(FALSE));
   

   I'm wondering whether the FALSE argument here may be problematic when the default language is changed as part of the save operation. Also because $langcodes includes the default language.

2. +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
   @@ -0,0 +1,209 @@
   + * Uploads files to translated nodes
   

   Missing trailing dot.

3. +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
   @@ -0,0 +1,209 @@
   +    // Add a second and third language.
   +    $edit = array();
   +    $edit['predefined_langcode'] = 'fr';
   +    $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));
   +
   +    $edit = array();
   +    $edit['predefined_langcode'] = 'nl';
   +    $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add language'));
   +
   +    // Enable translation for "Basic page" nodes.
   +    $edit = array(
   +      'entity_types[node]' => 1,
   +      'settings[node][page][translatable]' => 1,
   +      "settings[node][page][fields][$this->fieldName]" => 1,
   +    );
   +    $this->drupalPostForm('admin/config/regional/content-language', $edit, t('Save configuration'));
   +    \Drupal::entityManager()->clearCachedDefinitions();
   

   We've written this code so many times, it should really become a trait sooner or later :)

4. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
   @@ -810,4 +812,96 @@ public function testFieldEntityReference() {
   +    // default lancode.
   

   typo

1. My understanding now is that that's currently not possible. That said, there's no reason to pass FALSE there, I think we just copied that from somewhere. So it should work even if you did that.

Fixed #44.2 and #44.4. May the trait task be a new issue ?

#44.1 done.

I think the trait is fine for another issue imo.

Only one small nitpick before RTBC.

+++ b/core/modules/system/src/Tests/Entity/EntityTranslationTest.php
@@ -810,4 +812,96 @@ public function testFieldEntityReference() {
+  /**
+   * Tests if entity translation statuses are correct after removing two translation.
+   */

Shouldn't go over 80 chars

Rerolled and done.

I would personally have shortened the sentence to "Tests entity translation statuses after removing two translation." to fit on one line, but that sounds like a nitpick with a critical issue :) Fix looks good and the additional test coverage is great.

Or maybe "Tests entity translation statuses after removing two translations" (plural) :)

1. My understanding now is that that's currently not possible.

You can't change the default_langcode flag, but you can change the value associated to langcode for the default language, i.e. the default entity language can be modified.

I think the trait is fine for another issue imo.

Yup, it was only a consideration :)

Sorry to hold this up on comments, but this one confused me. Patch looks great otherwise.

1. +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
   @@ -0,0 +1,209 @@
   +    // Ensure the first file still exists. Calling isPermanent() would crash
   +    // the test suite.
   +    $file = File::load($first_fid);
   +    $this->assertTrue(!empty($file) && $file->isPermanent(), 'First file still exists and is permanent.');
   

   This says isPermanent would crash the test suite, but then it calls it. Does it mean 'if the file did not exist'? Also isn't $file && $file->isPermanent() enough of a check?

The reason I added that was because I wanted to have more asserts after this one in the initial test-only patch to prove that more things started to go wrong. Oherwise, the test suite crashed on the fact that $file didn't exist at all and calling that method would crash it.

Of course, now with the fixes in place, we can actually just call $file->isPermanent() directly and even remove that $file check.
New patch, cleaning up those asserts a bit, still RTBC I think.

Yes, that makes sense.

Looks good, yay!

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Yay, woot!

Took this out as a follow-up: #2787187: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity

I'm working on a project that someone else did，An error was encountered on commerce checkout completion, what I did is just add an file field(multi value) to order type.

Error mesaage:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function hasTranslation() on null in Drupal\file\Plugin\Field\FieldType\FileFieldItemList->postSave() (line 54 of core/modules/file/src/Plugin/Field/FieldType/FileFieldItemList.php).
Drupal\file\Plugin\Field\FieldType\FileFieldItemList->postSave(1)
call_user_func_array(Array, Array) (Line: 938)
Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldMethod('postSave', Object, 1) (Line: 970)
Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldPostSave(Object, 1) (Line: 896)
Drupal\Core\Entity\ContentEntityStorageBase->invokeHook('update', Object) (Line: 56)
Drupal\commerce\CommerceContentEntityStorage->invokeHook('update', Object) (Line: 64)
Drupal\commerce_order\OrderStorage->invokeHook('update', Object) (Line: 598)
Drupal\Core\Entity\EntityStorageBase->doPostSave(Object, 1) (Line: 781)
Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object, 1) (Line: 523)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 804)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 339)
Drupal\Core\Entity\EntityBase->save() (Line: 429)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowBase->submitForm(Array, Object) (Line: 614)
Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 114)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 52)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
Drupal\Core\Form\FormBuilder->processForm('commerce_checkout_flow_multistep_default', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 224)
Drupal\Core\Form\FormBuilder->getForm(Object, 'review') (Line: 143)
Drupal\commerce_checkout\Controller\CheckoutController->formPage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 169)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 38)
Drupal\webprofiler\StackMiddleware\WebprofilerMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 718)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I haven't got what cause this problem, but the code change of this issue's commit has some flaw here:
if ($original->hasTranslation($langcode))
It didn't check whether the $original is null.

I have create a new issue for it