$entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted

This looks like a duplicate of #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations. If it's not, please re-open but otherwise see you over there.

Moving to entity system and bumping to critical.

Having this same problem on a Drupal 8 site [ 8.1.10 ] Having this problem with image fields and file fields.

Here are my steps to reproduce:

1. Create a content type w/ an Image or File Field
2. Enable 2 languages on site
3. Configure language/translations settings here: admin/config/regional/content-language
4. Create a node in Lang 1, upload an image and save.
5. Check "file_managed" table, status should be correctly set to 1.
6. Change node language to Lang 2 and save.
7. Check DB table “file_managed”, this record will incorrectly get a status of 0 (marked as temporary file)
8. To fast track this and not have to wait for Cron or the file to expire, Edit file managed table for the file entry in DB and set both “created” and “changed” date to a timestamp that is expired (just set it to yesterdays timestamp)
9. Run Cron
10. Files will be deleted, since they are marked as temporary files w/ a status of 0.

Isn't this essentially the same problem as #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations? That'd not be surprising, because editor module essentially copied this logic from file module.

Recently @catch, @webchick, @xjm, @cottser, @cilefen and I discussed #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary , #2810355: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted and #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations. We all agreed that the current file usage behaviour that is resulting in files being deleted when they are being used by a site is unacceptable and a critical issue. We've created #2821423: Dealing with unexpected file deletion due to incorrect file usage to plan for how to fix and mitigate the related issues.

I'm investigating this issue currently. Going to check if existing tests will fail if we don't remove all usages, but only 1.

Did not know that unchecked "Display" checkbox makes the patch skip testing.

The test fail seems to be correct.

FileFieldItemList need to delete all file usages within the entity if the entity is going to be completely deleted.
This happens here: https://github.com/drupal/drupal/blob/740ccca37f6f3255e8afcf3246ce8cd7de...
The only wrong thing here: it tried to guess if entity is going to be deleted completely by checking $entity->isDefaultTranslation(). But there is no other way.

In case of changing the original language, this check is false positive because ContentEntityStorageBase loads a translation from the original entity. This translation is marked as default one.
It happens here: https://github.com/drupal/drupal/blob/740ccca37f6f3255e8afcf3246ce8cd7de...

I still don't know how to fix this because both pieces of code try to make the right things.
One possibility to solve this would be to unmark default translation on the deleting translation, but there is no way to do that.

BTW, according to my tests, the bug happens only if file field is translatable (no matter which its properties are translatable).

One possibility to solve this would be to unmark default translation on the deleting translation, but there is no way to do that.

Let's try that.

@Leksat have you seen #2821423: Dealing with unexpected file deletion due to incorrect file usage - i think we need to discuss the whole problem space - the counting of file usages seems very very brittle.

@alexpott I agree that it would be nice to have a general solution for all the file usages problems, but
1) we have a production D8 project where we lose files and we need at least a temporary fix for that (disabling "Delete orphaned files" option completely does not seem the right choice)
2) my work can be useful in case if it would be decided to leave file usages and fix all bugs

So, sorry for disturbing people following this issue, but I need to test at least one more patch :)

Okay, #20 can be used as a temporary fix for this issue.

@Leksat are you able to write a test to prove the temporary fix works? Also I didn't mean to discourage you from working on this at all - just pointing to the overarching plan. If you can make progress here that is absolutely fantastic. These file deletions bugs are the most serious we have.

@alexpott, okay :)

Wrote a small test for this issue.

But I still think that this is only temporary fix because the solution - a public $isDefaultTranslation property marked as @internal - is way too hackish. Yet I can't find anything better for now.

Also noticed this old issue from D7 #1716884: Changing the language of a piece of content deletes attached media that might be related?

Also occurs if you add an image (for instance with text on it in one language) in a node and an image with the same file name (with for instance the same text in another language on it) in the translation of that node. Once you have saved the node translation, the usage of the first file is set to 0, resulting in deletion by system_cron after a while.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -96,6 +96,15 @@
+  public $isDefaultTranslation = NULL;

Is there a reason we could not have an internal setIsDefaultTranslation method?

I closed #2837023: Image fields that follow core gets broken when set to translatable as a duplicate. Report from that issue:

Steps to reproduce;

1. Enable Content Translation module
2. Add 1 language
3. Make default content type Article that comes with core translatable
4. Check that the default image field is now set to translatable ("Users may translate this field"): File is not translatable, but the Alt and Title elements are translatable (default configurations)
5. Add a node (without touching the image field)
6. Add a translation of the node and upload a image
7. Check back on the other language version; The image is not there!

If you add the image once you create the node it will be displayed across all translations. However if you later edit the image field (both in the source language or in translations) the image will only be changed in that language version (and the old image will remain in the other language version).

However it seems like this behavior is only present when making the default content type Article that is shipped with core, translatable, and using the default image filed that is also shipped with core. If you use the default image field in another content type, or create a new translatable image field to be used in default content type Article, there seems to not be any problems.

To me the issue seems a little critical though, as many people potentially will use exactly those default configurations that are shipped with core.

Patch in #23 applies to 8.3.x.

Re-running the patches from #23.

Added setIsDefaultTranslation() method.

Updated ContentEntityCloneTest to exclude isDefaultTranslation property.

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -16,6 +16,18 @@
+  public function setIsDefaultTranslation($is_default_translation);

I think there is a question if we want to have an API that can do both sides of this. I.e make a translation not the default translation and make a translation the default translation. Maybe introducing a enforceNonDefaultTranslation() would be better. Because then we can document when it should be used which is never. Or maybe we should add a parameter to \Drupal\Core\Entity\ContentEntityBase::getTranslation() which indicates whether or not we should enforce non default translation-ness. I'm not sure. I'll ping the entity maintainers.

I went through the entire issue.

The following comments were especially important:

1. @Leksat's analysis in #15 is splendid! @Leksat++
2. And then @Leksat is being his awesome self again in #23, where he provides both test coverage to prove the bug, and a work-around. The test coverage is most important.
3. @steinmb is eagle-eyed in #25. I think he's absolutely right that this is already broken in Drupal 7. @Berdir pointed out in [some other issue] that the reason the problem is bigger is that Drupal 8 has translation support built in, which makes it more likely to run into this problem.

Having looked at this again, I now disagree with my own analysis in #10. It's very similar to #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations, but it's not the same. The difference:

1. for #2810355 (this issue), the root cause is \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::delete() removing file usages, and it relying on $entity->isDefaultTranslation() (i.e. \Drupal\Core\TypedData\TranslatableInterface::isDefaultTranslation()) to determine whether a translation is being deleted or not, and apparently $entity->isDefaultTranslation() behaves incorrectly in case the default translation is being changed
2. for #2708411, the root cause is editor.module's use of hook_entity_insert() + hook_entity_update() + hook_entity_delete() + editor_entity_revision_delete() that were introduced in #1932652: Add image uploading to WYSIWYGs through editor.module. It looks like it should also use hook_entity_translation_insert() + hook_entity_translation_delete(), but those were added after #1932652 had already been started, in #1810370: Entity Translation API improvements.

I think it's important we retitle this issue & update its summary for clarity. Did both of those. A change record seems necessary too, because there's likely other code in contrib/custom code that is relying on isDefaultTranslation() — added the Needs change record issue tag.

Finally, a patch review.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -340,7 +349,17 @@ public function setRevisionTranslationAffected($affected) {
      public function isDefaultTranslation() {
   +    if ($this->isDefaultTranslation !== NULL) {
   +      return $this->isDefaultTranslation;
   +    }
        return $this->activeLangcode === LanguageInterface::LANGCODE_DEFAULT;
      }
   

   This looks to me like it's a hacky temporary work-around rather than a complete solution. Because rather than tracking the new default translation, this is just an override that will only work if ContentEntityStorageBase::invokeFieldMethod() happens to be called on behalf of postSave(). Which means isDefaultTranslation() will still return the incorrect value when the entity is being updated in the future (when no translation is being removed, but the active lang code also won't be LanguageInterface::LANGCODE_DEFAULT?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -485,7 +485,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
   +        // Fields may rely on the isDefaultTranslation() method to determine
   +        // what is going to be deleted - the whole entity or a particular
   +        // translation.
   +        if ($translation->isDefaultTranslation()) {
   +          $translation->setIsDefaultTranslation(FALSE);
   +        }
   

   Rather than stating that this translation is no longer the default, this should be indicating which translation now is the default.

   If that were the case, then we also wouldn't have to perform this if-test for every iteration of this loop.

3. @dawehner asked in #28 for a new @internal setIsDefaultTranslation() method. @Leksat did this in #33. @alexpott is now saying in #37 that it should be possible to do both sides. But I'm not sure Alex' request makes sense entirely. It makes sense on the surface. But … there must be a default translation. And so making a translation not the default translation (which he suggests should become enforceNonDefaultTranslation()) implies that at the same time, another one must be made the default translation. Which effectively implies that you cannot ever call enforceNonDefaultTranslation() without also calling setIsDefaultTranslation().

This needs to be reviewed by an Entity/Field Translation API maintainer/expert. Tagging Needs subsystem maintainer review for that.

Thanks to @Juanswe at #2666700-28: User profile images unexpectedly deleted, I was able to close #2666700 as a duplicate of this with 100% certainty. Quoting my comment at #2666700-30: User profile images unexpectedly deleted:

#28: thank you so much for posting that! Your information confirms exactly what I suspected:

its happening when i install a new language on my page and the logged in user change the default language on her page, the avatar dissapear, But only if he change the language. The others user have not problems.

Then the steps to reproduce are:

1. use the "user picture" field
2. user X uploads a photo to their "user picture" field
3. install a new language (note this can be a new interface language — you don't need to have content_translation installed!)
4. user X changes their preferred language
5. user X' user picture disappeared!

This is because \Drupal\user\AccountForm::form() does:

    // User entities contain both a langcode property (for identifying the
    // language of the entity data) and a preferred_langcode property (see
    // above). Rather than provide a UI forcing the user to choose both
    // separately, assume that the user profile data is in the user's preferred
    // language. This entity builder provides that synchronization. For
    // use-cases where this synchronization is not desired, a module can alter
    // or remove this item.
    $form['#entity_builders']['sync_user_langcode'] = '::syncUserLangcode';

Which means that \Drupal\user\AccountForm::syncUserLangcode() is called upon saving the account form, which then does:

  public function syncUserLangcode($entity_type_id, UserInterface $user, array &$form, FormStateInterface &$form_state) {
    $user->getUntranslated()->langcode = $user->preferred_langcode;
  }

… which means that the default translation is being changed!

This is exactly the same problem as #2810355: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted then.

Thanks to #28, I was able to prove this conclusively.

This means that even sites without content_translation are affected by this, merely having the language (Interface translation) module installed and having >1 interface language enabled is sufficient to reproduce this problem for image/file fields on User entities!

That significantly increases the chances that somebody is running into this.

This is now a blocker of #2708411 — see #2708411-54: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations.

Given #2708411-60: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations, I'm downgrading this one to major as well.

So looking at this further.

I think fixing the "isDefaultTranslation()" as in the patch is not the way to go.
Since in essence the entity the field is connected to is indeed the default translation even if its langcode changed.
Also note that at no point the actual "entity" is really changed in any other way. But the fields connected to the old language value have been stripped off and cloned to values in the new value (calling update and deleted respectively).

However it's possible to check the "status" of the new translation.
See \Drupal\Core\TypedData\TranslationStatusInterface which (in case of the updating of an existing translation would lack the needed flag and can be used in this context to differentiate between the changing of the language and the deleting of the parent entity).

Flow as I can see it.
- Entity is constructed. At this point the $activeLangcode property is set to LanguageInterface::LANGCODE_DEFAULT.
- The field update / delete callbacks are called.
- First the postSave will add an extra file usage instance to the table (which is correct, but overwritten in the next step).
- Then the delete callback will delete the entire file usage for this entity based on the isDefaultTranslation(). (as Mentioned)
The isDefaultTranslation() is a very simple function:
return $this->activeLangcode === LanguageInterface::LANGCODE_DEFAULT;
It would seem this will indeed return true for every entity that hasn't explicitly loaded another language. Since the only way that I could find that changes this property from it's original value seems to be:
\Drupal\Core\Entity\ContentEntityBase::getTranslation()

New version without a silly oversight in how the isDefaultTranslation is resolved.

However,
This still seems to trigger an error in the test that doesn't seem to be related to the patch directly.
Added a link to a more general revision issue.

Rerolled patch. test was removed it seems, so I didn't reroll that part.

Hi there,

I'm getting a similar disappearing avatar issue/behavior in a drupal 7 install ... is there a patch or recommended solution for D7? Thank you!

I suspected that a version of this patch might help our use case.

I'm helping develop/improve the Drupal 8 version of ETUF (entity_translation_unified_form (a VERY cool module, the D7 version is prestine, D8/9 version requires some polishing as I am about to describe:

ETUF module puts both English and French (our two active languages) field elements onto the same node edit form.

Image fields however are uploaded via ajax, the upload actually works fine with the ajax call, however, when saving the node, or somewhere along the line, the OTHER language image field doesn't get saved properly, the image (file managed) record remains a 'temporary' file and does not get stored as 'permanent'.

I'm looking to fix the root cause however I'm pretty close to the symptoms and have isolated this enough that I can probably create a workaround however I did try a couple of the above patches with no change in behavior, perhaps I need to take a closer look at the proposed api change here.

Turned out not to make a difference in our case, sorry for the noise. I found a workaround for my use case, although it looks like some sort of a core bug, I fixed the symptom.

Please disregard my previous comment above.

Confirmed file_usage is out of wack for sites with more than one language. I tried relying on file_usage data to do a cleanup, ended up deleting files that were still in use.

patch #48 still applies cleanly to 8.7.x, still applies to 8.8.x and 9.0.x

Something is wrong with the tests. The test needs updating or else there's some regression with the patch.

we have two languages on our site, and our file_usage table is incorrect, values that were flagged as unused are in fact still in use by the other language.

We would really appreciate this issue being fixed.

this is a data integrity issue

new patch comming shortly.

Rerolled 36, a test moved so I adjusted the patch, and there was a conflict that I resolved with another test, so I generate a new patch.

replaced deprecated calls with D9 api calls

ok one more try.
see interdiff and new patch.

It would be great if someone wants to take the glory and push this home, have to first see Wim Leers comments in #2810355-38: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted

needs to be reviewed by an Entity/Field Translation API maintainer/expert. Tagging "Needs subsystem maintainer review"

Patch 61 (reroll of 36) has tests to prove worthiness.

Thanks!

Adding same patch as 61 with the tests only patch.

This core isDefaultTranslation() function is incorrectly labelled and incorrectly utilised in core, this patch essentially forces it to return false but would have to look closer just as to 'when' and "how often" it will return false.

imho, the isDefaultTranslation would be more appropriate as a method of the entity interface. It should only do processing if the core content_translation module is enabled.

With that said, isDefaultTranslation is used elsewhere in core, those calls would be done to a renamed function that is more accurate as to the meaning/context/use of it (to save us from the confusion).

A translation is relative, relative to what? relative to the configuration in use.

each entity can have a different language for a 'source' translation, however isDefaultTranslation assumes that the whole site works in the way defined in the language default without considering that source language can be any language on a site and entities can be differently configured (source language) from one entity to the next regardless of the global site language configuration. Ex, install Drupal with a language of your choice, add a second language, (take your pick) , an added language could be the source language for an entity as can the default language (so what is the default translation in this case? what does that mean?, the usecase does not seem to match.

inversely, a site can be installed with any language and any other language can be the 'added' language.

What is confusing here is the label of this function and its usage. The latest patch has a test that illustrates the problem but the solution is concerns that Wim Leers mentions brings up debate on how to approach this.

I think we need @plach to have a look at this.

imho, the isDefaultTranslation would be more appropriate as a method of the entity interface. It should only do processing if the core content_translation module is enabled.

@joseph.olstad, entity translations are part of the content entity API and do not require content_translation to be enabled. The isDefaultTranslation() method returns TRUE for the first translation of the entity - this is the translation language in which the entity was created initially and represent the default translation of the entity. Through the entity API it is possible to translate the entity in different languages using \Drupal\Core\TypedData\TranslatableInterface::addTranslation(). For this new translations isDefaultTranslation() will return FALSE. The method is not placed on the EntityInterface, but on TranslatableInterface, because we do not want to force someone directly implementing the EntityInterface to implement any methods related to translations. For example config entities do not implement this interface, but only content entities and the ContentEntityInteface extends from the TranslatableInterface. I do not think that there are any reasons to change this architecture.

I hope that this clarifies it for you? If not let us please discuss this in a dedicated issue as this even though related falls out of the scope of the current issue. Thank you.

@hchonov ,

I was probably a bit sleepy when I wrote the previous post , some precisions:

a more appropriate name for this functionality (outside of the entity translation / (content translation)) referring to interface language or interface translation would be 'isDefaultInterfaceTranslation' or 'isDefaultInterfaceLanguage' . I recall programming in the 1980s on computers like the VIC 20 when there was only a few kilobytes of memory available, back then it was appropriate to write cryptic code with very short function names and variable names as every "byte" was preciously conserved however we no longer have such limitations and it is far more appropriate to write explicitly named function names and method names than the opposite.

of course we now have namespaces, but when debugging code revolving around concepts as complex and multi-facetted as translation it is easy to get confused so in this case I think a more explicit label for the function would be helpful for those writing the code and debugging it.

Although more importantly we need to permanently fix the bug that the above test illustrates.

I looked at the patch and while it does work and fixes the problem it is confusing looking at this code.

Translation is on several levels, interface strings, entities, current interface language, default language has more than one meaning (you could talk about site default interface language or get confused with the source language of an entity translation, a translation has more than one meaning, to debug the code you have to be aware of all of this and when the labels are not explicit it's easier to get confused).

Can you please take a close look at patch 64 and explain why it works? It is not that obvious.

As Wim Leer mentioned, this bug also affects installations not using content translation. Installations can be installed in languages other than English and only have one language but Drupal in this case still has to deal with string /interface label translations (not entity).

With that said, the fix has to work with installations that either do not use content translation and installations that use content translation.

Translation is on several levels, interface strings, entities, current interface language, default language has more than one meaning (you could talk about site default interface language or get confused with the source language of an entity translation, a translation has more than one meaning, to debug the code you have to be aware of all of this and when the labels are not explicit it's easier to get confused).

This is why it is always important to consider the context. When calling the function on an entity object it is pretty obvious that one is asking the entity object itself and not some other service. We cannot go and provide unique names for every possible method, nevertheless methods are also being reused. Please note that \Drupal\Core\TypedData\TranslatableInterface::isTranslatable() is in the TypedData and not in the Entity namespace, which means that the same interface might be reused for translatable data in Drupal which does not represent entities. Having one interface and a one method make the code reusable.
If you still think there is further discussion about this needed, then please open a new dedicated issue as this is clearly out-of-scope here.

Can you please take a close look at patch 64 and explain why it works? It is not that obvious.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -844,7 +844,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
+        // Fields may rely on the isDefaultTranslation() method to determine
+        // what is going to be deleted - the whole entity or a particular
+        // translation.
+        if ($translation->isDefaultTranslation()) {
+          $translation->setIsDefaultTranslation(FALSE);
+        }

Because of this change in the storage, which will flag the translation of the original entity object as non-default. The delete method is then called on every field of the original translation inclusive the File field delete method, which checks whether only a translation was removed or the whole entity was deleted. In \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::delete() it is assumed that if the entity is in the default translation, then the whole entity itself is being deleted and otherwise only a single translation.

The test also works because when \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::postSave() is called it would have increased the count to 2. Then the delete method will be called on the original translation after flagging it as a non-default translation and then this will decrease the file usage by 1. Before flagging the removed translation as non-default this used to remove all the usages.

-----

Looking at the patch and considering that in order to execute the delete method on the fields of a deleted translation we need to load the original entity and from there retrieve the deleted translation object on which fields to execute the delete method, I think that a better approach would be to make it possible for the storage to load a deleted translation from the current object instead. To cover the case where there is only one translation and its langcode is being changed, then when this happen we would have to create dynamically a new translation and flag the previous/original one as deleted. Then the field's postSave and delete field methods will behave correctly and we will invoke the postSave method on the new default translation, which then will increase the usage by 1 for this new translation, but afterwards call the delete method on the removed and already non-default translation and decrease the usage by 1. Basically this will result into the same what is already happening with the current patch, but in a more proper way.

I do not like the approach of introducing another setter method to temporarily disable isDefaultTranslation() from returning TRUE, instead it should be possible for ::getTranslation($langcode) to load a removed translation from the entity object. getTranslationLanguages() should further not return the langcode for the removed translation(s).

Yowza, this not good. I thought we'd fixed all these file deletion bugs already.

Since we should fix this bug in D8 too, I'm filing it against 8.8.x (which is the current bugfix support branch). Patches can be tested against other branches as needed when they're uploaded.

The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!

Removed Wrong comment, posted by mistake over here : -Still this patch is under review. I tried to unset this error constraint.

function MYMODULE_entity_type_alter(array &$entity_types) {

  foreach ($entity_types as $entity_type) {
    $constraints = $entity_type->getConstraints();
    unset($constraints['ContentTranslationSynchronizedFields']);  
    $entity_type->setConstraints($constraints);
  }

}

/**
 * Implements hook_module_implements_alter().
 */
function MYMODULE_module_implements_alter(&$implementations, $hook) {
  if (in_array($hook, ['entity_type_alter'])) {  
    $group = $implementations['MYMODULE'];
    unset($implementations['MYMODULE']);
    $implementations['MYMODULE'] = $group;

  }
}

@joseph.olstad, see #72. Thanks!

> The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks!

8.9.x? Tagging "Bug smash initiative" as well.

Posting the relevant error message here so it can be picked up by search engines.

This validation message is triggered when retroactively setting a language to existing media, and then adding a translation:

Error message: This entity (file: 51943) cannot be referenced.

Patch 64 still applies cleanly to 9.2.x, 9.1.x and 9.0.x, 8.9.x, 8.8.x, has tests.
If no one can find anything wrong with the patch then we should please move forward with it as running 'with' the fix is far better than running 'without' the fix.

Most people won't realize there's a problem until it's too late!!! Lets save everyone the trouble and get this in.

I think I agree with #71. I'm not convinced about the solution. I agree that adding the setter doesn't look great and it feels like there should be another way around this.

Here's a review of the actual patch.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -86,6 +86,15 @@
   +  public $isDefaultTranslation = NULL;
   

   I think this can be protected.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -409,10 +418,20 @@ public function setRevisionTranslationAffectedEnforced($enforced) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setIsDefaultTranslation($is_default_translation) {
   +    $this->isDefaultTranslation = $is_default_translation;
   +  }
   

   setters should return $this for a fluent interface.

3. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -30,6 +30,18 @@
   +  /**
   +   * Overrides isDefaultTranslation() result.
   +   *
   +   * @param bool|null $is_default_translation
   +   *   If boolean value is passed, the value will override the result of
   +   *   isDefaultTranslation() method. If NULL is passed, the default logic will
   +   *   be used for determining the result.
   +   *
   +   * @internal
   +   */
   +  public function setIsDefaultTranslation($is_default_translation);
   

   If we going to add this to the interface then we should add scalar typehints. Ie. public function setIsDefaultTranslation(bool $is_default_translation = NULL);

@alexpott thanks for the feedback, I don't fully understand why this patch works however I made your suggested changes (very good) and have provided an interdiff also.

Thanks!

checking patch

New patch , new interdiff

All green again with recommended changes from #79.
Has tests.
Let me know if there's more to be done here.

It's maybe not pretty but it gets the job done and has tests.

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -30,6 +30,18 @@
 
+  /**
+   * Overrides isDefaultTranslation() result.
+   *
+   * @param bool|null $is_default_translation
+   *   If boolean value is passed, the value will override the result of
+   *   isDefaultTranslation() method. If NULL is passed, the default logic will
+   *   be used for determining the result.
+   *
+   * @internal
+   */
+  public function setIsDefaultTranslation(bool $is_default_translation = NULL);
+

I'm not so sure we're allowed to add a method to this interface under the BC rules. This is not a 1:1 interface, although core only uses it on content entities through TranslatableInterface in the entity namespace and then TranslatableRevisionableInterface.

It is definitely going to break a bunch of modules like simple_oauth, which has its own custom implementation of UserInterface extending ContentEntityInterface. I don't think that was a good idea, but hard to change now ;)

Do we have a pattern for doing this in a BC-way?

Putting it on a separate interface doesn't help too much in practice, as ContentEntityInterface would still have it, but it would technically fall under the 1:1 rule then.

I'm not sure how BC would be broken here, we're defaulting is_default_translation to NULL and is only needed in the functions in the patch above (for now) but probably this will be helpful in the future as well.
IMHO, we have to be pragmatic about this.

While there's only 62 people subscribed to this issue there will inevitably be many more not realizing the problem and by then too late, it was actually pure hasard that I discovered this problem and sheer determination that I followed up here (during import/export scripting to/from Drupal8/9 to Drupal 8/9. ) by the time someone discovers this issue, they've probably lost their files (which was my case) aka were deleted inadvertantly as my script was relying on file usage but file usage came up 0, and then my script logic relied on this thinking it was safe to delete so I lost a bunch of files and didn't realize until later.

I retriggered the test for 9.1.x, probably just a hiccup in the runner.

1. +++ b/core/modules/file/tests/src/Functional/FileOnTranslatedEntityTest.php
   @@ -206,4 +206,37 @@ public function testSyncedFiles() {
   +    $file = \Drupal\file\Entity\File::load($this->getLastFileId());
   

   We should use a "use" statement instead of \Drupal\file\Entity\File

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -86,6 +86,15 @@
   +   * @var null|bool
   

   Nitpick: change to bool|null

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -86,6 +86,15 @@
   +  protected $isDefaultTranslation = NULL;
   

   Could we add some more documentation about this class variable. How and why is it different from the method with the same name. The how and why of this variable is not completely clear to me.

4. I agree with @berdir that adding this @internal method to the interface is a BC break. Please remove the method from the interface.
5. Could there come a reply on the patch review from @wim_leers in comment #38

Addressed comment #89.1 and #89.2

NW for remaining points of #89.

Fixed failed test of patch #90.

Worked on test case.

Still remaining #89.3, #89.4 and #89.5.

There was no reason given for closing this and it's still something worthwhile fixing.

Still remaining #89.3, #89.4 and #89.5.

FYI #3276540: Block (temporarily?) stephencamilo user for modifying many issues to be "won't fix"

Fixed failed the test of patch #92. Still needs work for #96.

Going through important patches that we have been using for several years. It would be nice to get this one in.

All the nits from #89 are resolved by 100 and then 102

See interdiff.

Justification for RTBC:

• #64 "only tests" proves the fix is needed.
• Nits resolved from #89
• Patch 102 passes tests on 9.4.x
• Patch 102 passes tests on 9.5.x
• Patch 102 passes tests on 10.0.x
• Patch 102 passes tests on 10.1.x

> I agree with @berdir that adding this @internal method to the interface is a BC break. Please remove the method from the interface.

-- from #89.4

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -30,6 +30,18 @@ interface TranslatableInterface {
+  public function setIsDefaultTranslation(bool $is_default_translation = NULL);

+1 to remove this.

Otherwise, RTBC +1

Addressing #105

Tagging "Needs Review Queue Initiative" looks for review.

I don't think #106 addresses the bc break brought up by @berdir.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -947,7 +947,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
       $original_langcodes = array_keys($entity->original->getTranslationLanguages());
       foreach (array_diff($original_langcodes, $langcodes) as $removed_langcode) {
+        /** @var \Drupal\Core\Entity\ContentEntityInterface $translation */
         $translation = $entity->original->getTranslation($removed_langcode);
+
+        // Fields may rely on the isDefaultTranslation() method to determine
+        // what is going to be deleted - the whole entity or a particular
+        // translation.
+        if ($translation->isDefaultTranslation()) {
+          $translation->setIsDefaultTranslation(FALSE);
+        }
+

If we don't check an interface (or ContentEntityBase), we can't rely on it here - instead of an immediate fatal error because you haven't implemented the method, you'd get one when this code runs, which would be a slower but nastier surprise.

I think this probably needs a new interface, which ContentEntityBase then implements, and then we can check instanceof for that interface.

EntityTranslatableInterface maybe?

Then we could open a follow-up merge that back into TranslatableInterface for 11.0.0

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -947,7 +947,16 @@ protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
+        /** @var \Drupal\Core\Entity\ContentEntityInterface $translation */
         $translation = $entity->original->getTranslation($removed_langcode);
+
+        // Fields may rely on the isDefaultTranslation() method to determine
+        // what is going to be deleted - the whole entity or a particular
+        // translation.
+        if ($translation->isDefaultTranslation()) {
+          $translation->setIsDefaultTranslation(FALSE);
+        }
+
         $fields = $translation->getTranslatableFields();
         foreach ($fields as $name => $items) {

If it's a temporary name then I'd go with ha specific name for the interface, like SettableDefaultTranslationInterface.

But then we also need to do a deprecation messsage if an entity doesn't implement that and the bug won't be fixed there for now.

And then in D11, we need to deprecate the interface and remove it in D12 :-/.

I do wonder if we can somehow find a different solution for this.

I think it would kind of make sense if invokeFieldMethod() would be on the entity, then it would be an internal property that we could handle without a new public method for one very specific use case. But moving that has exactly the same problem.

The other idea is that we're not actually deleting a translation, we're changing the langcode of an existing one. Feels weird to call delete at all in this scenario, but we're doing it to counter the equally "wrong" is-new-translation check in \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::postSave(), so we add a usage and then remove one again, resulting in no change (with the patch).

I don't have a completely formed idea, but I'll try to think about it a bit more.

What about just doing a method_exists() check on $entity for BC?

The problem that I see is that we can't detect this at runtime in a useful way. We could easily add an else to this logic and then trigger the deprecation message, but the chances of an actually working full implementation of ContentEnttityInterface that actually get saved are near-zero. The cases we're working with are edge cases that aren't really entities. Searching a bit, I couldn't find any examples even for that, I suspect simple_oauth has been refactored to no longer do that.

That means that if there really are such classes, they will never get in there, they are most likely also not actual entity types, so we have no easy mechanism to find them and trigger a deprecation message. We could maybe do something with phpstan or so, I checked if symfony/deubgclassloader has anything for that, but I think not.

So, method_exists(), deprecation message, and worst case is someone will get an easy to fix fatal error on D11 when they update.

Here's a patch for that, also created a change record. I added a @trigger_error, while I don't expect this will ever be triggered, it also serves as a reminder to make that change in D11.

Fixing that test fail.

Thanks for that @Berdir .

This is a pretty important fix, I hope that the core maintainers prioritize this. The current behavior was a nasty surprise when I relied on file usage and ended up losing a lot of media due to the bug.

Removing CR tag as I see that was added.

Can the needs subsystem maintainer tag be removed since @berdir, @catch, and @hchonov are all on this ticket?

Leaving in NR but can deprecation message be updated in #115 also please.

@smustgrave, what part of the deprecation message needs changing?

In #115 it is as follows:

@trigger_error('Not providing a setIsDefaultTranslation() method when implementing \Drupal\Core\TypedData\TranslatableInterface is deprecated in drupal:9.4.0 and is required from drupal:11.0.0. See https://www.drupal.org/node/3376146', E_USER_DEPRECATED);

The version, it needs to say 10.2 instead of 9.4. We can't retroactively deprecate it, this will not be backported as it introduces a new method.

Brought up in #needs-review-queue-initative and can remove the needs subsystem maintainer tag.

Moving to NW for the trigger_error update so that doesn't get lost.

Ok thanks for clarifying, here's a new patch and interdiff.

This patch applies on 9.5.x, 10.0.x, 10.1.x, and 11.x

Thanks!

Going to mark this and see if we can get into 10.2 soon!

Adding credits from the triage discussion mentioned in #11 (nb: Cottser has a different username now).

Thank you all very very much. We just ran into this in a Drupal 10.1.2 project and I was searching for hours until I found the solution here. Indeed critical and super nice to see this close to be fixed, thank you so much! Confirming that #121 fixed the issue for us!

I first thought it was #1452100: Private file download returns access denied, when file attached to revision other than current.

Would be really important to have this fixed in the next possible release.

Random fail in Drupal\Tests\Component\Utility\RandomTest.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -86,6 +86,18 @@ abstract class ContentEntityBase extends EntityBase implements \IteratorAggregat
   +  protected $isDefaultTranslation = NULL;
   

   I think this should be
   protected bool|null $enforceDefaultTranslation = NULL;

   As this is consistent with $enforceRevisionTranslationAffected.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -409,10 +421,26 @@ public function setRevisionTranslationAffectedEnforced($enforced) {
   +  public function setIsDefaultTranslation(bool $is_default_translation = NULL) {
   

   If we look at the methods above this I think this method naming is awkward...

   The code above this is:

     /**
      * {@inheritdoc}
      */
     public function isRevisionTranslationAffectedEnforced() {
       return !empty($this->enforceRevisionTranslationAffected[$this->activeLangcode]);
     }
   
     /**
      * {@inheritdoc}
      */
     public function setRevisionTranslationAffectedEnforced($enforced) {
       $this->enforceRevisionTranslationAffected[$this->activeLangcode] = $enforced;
       return $this;
     }
   

   I also think we should not provide the default for the argument.

   Should have a return type of :static and the @return needs documenting.

   So I this should be
   public function setDefaultTranslation(bool|null $is_default_translation): static {

@alexpott:

1. Are we supposed to declare the type of any new class property? If so, I think we should use ?bool instead of bool|null. I like the idea of naming the new property enforce....
2. For consistency, I think the function name should end with Enforced. Again, I think we should use ?bool.

I am also updating some of the comments.

Personally, I would use

  public function isDefaultTranslation() {
    return $this->enforceDefaultTranslation
      ?? $this->activeLangcode === LanguageInterface::LANGCODE_DEFAULT;
  }

but I will restrain myself since this is someone else's code.

Seems points in #130 have been addressed.

Committed and pushed aa3ecb2c2d2 to 11.x and 4976f3a460f to 10.2.x. Thanks!

I created #3394676: Add setDefaultTranslationEnforced to \Drupal\Core\TypedData\TranslatableInterface so we remember to add the method to the interface.