Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity

Seeing this as well, due to the data loss which silently occurs, this is at least major if not critical.

Yowza. Thank you for reporting this.

I'm writing a test.

Attached is a patch which fixes my immediate problem.

With that check, loosely adapted from the postSave() function in place my translation only gets a decrement in file_usage of 1 instead of all. Though I might have queried the entity api incorrectly there since the deletion of the original doesn't actually seem to force the other half of the if statement.

The latter shouldn't be a big issue if the counter would not increment incorrectly. I am consistently seeing a count of 3 when a translation is added to the original instead of 2 (or is that intentional?).

Yepp, this test reveals the bug.

And the patch (inspired from #7).

@azinck, you're right but I think you are referring to #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger. Isn't so?

Fixed the IS.

Given #11 and #14 I'm setting this to 'needs work.'

@Mile23, can you, please, be more specific? I don't think something else needs to be done here. Removing usages added by revisions is fixed in #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger, not here. As I recall is fixed by deleting the revisions and that is piped thorough \Drupal\file\Plugin\Field\FieldType\FileFieldItemList::deleteRevision.

@catch, @effulgentsia, @alexpott, @Cottser, and I agreed that this is definitely a critical bug because of the data loss issue.

Overall looks great to me, but:

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileFieldItemList.php
@@ -81,9 +81,12 @@ public function delete() {
+    $count = (int) ($untranslated->language()->getId() != $entity->language()->getId());

While this is pretty clever casting logic, it would be better to write this explicitly, as it is very hard to understand on a first glance.

@Fabianx, thank you for review.

While this is pretty clever casting logic, it would be better to write this explicitly, as it is very hard to understand on a first glance.

Made it more explicit.

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileFieldItemList.php
@@ -81,9 +81,12 @@ public function delete() {
+    $count = $untranslated->language()->getId() === $entity->language()->getId() ? 0 : 1;

Couldn't you use isDefaultTranslation actually?

Sure, forgot that method.

Improved the comment.

Sweet, much better now!

Committed/pushed to all three 8.x branches, thanks!

Yay, thanks!

Test failed in HEAD, reverted and sending for re-test on all three branches.

This is amazing. Those 2 tests are passing now in 8.2.x and 8.3.x HEAD because of the combined effect of two bugs that are compensating each other: this one and #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger. This bug releases too many file usages while the other wrongly add them back making, together, a zero balance. But because #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger is not yet committed, the effects of the 2 bugs are not compensating anymore.

I propose to temporarily get rid about that failed test till #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger. In that patch we need to restore the lines. See the interdiff for details.

The behavior has changed after #2490136: Enable revisions by default has been committed.

Back to RTBC. As discussed with @catch on IRC we can live with the temporary solution until #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger.

• 8.1.x - patch from #24: 2787187-24.patch
• 8.2.x and 8.3.x - patch from #37: 2787187-35-8.2-3.x.patch

Another approach suggested by @catch: making the node types from tests not adding new revisions by default.

Back to RTBC, looks good to me.

Always fun when there are bugs that cancel each other out.

Committed/pushed to all three 8.x branches, thanks!

This accidentally introduced a new critical: #2810355: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted, because $entity->isDefaultTranslation() is broken.

This also broke the case of when file A is used in translation X and file B is used in translation Y. When you delete the non-default translation, if that used a different file than the default translation, its usage will never reach zero.

Reopening to ensure a follow-up is created for that.

I don't think though that the non reaching zero problem is a critical issue.

Now that #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary is in, this no longer results in unexpected file deletion. #2821423: Dealing with unexpected file deletion due to incorrect file usage is open as a critical meta-issue to discuss the broader approach on this. So I'm going to downgrade this to major after discussion with @xjm and @cilefen, since we feel effort would be better spent on replacing the file_usage system at this point. We can still fix this bug of course as a major.

I encountered issues when relying on file usage data in a cleanup job. I discovered that important files that were still 'in use' were deleted.

There is a related fix/patch that has tests and is passing tests.
see #2810355-61: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted

Appreciate community and core maintainers assistance to get this patch to where it needs to be in order for it to be accepted.
Thanks,

#47 re-opened this for a follow-up, I think that follow-up is now #3364744: Add a reliable entity-usage system to core so marking fixed again.