Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger

I've tested, and this appears to be a core issue.

Are you able to delete the file using media/[file id]/delete?

most likely part of the file system, rather.

steps to reproduce w/o media module, from a new installation of drupal:

1) create an article post with an image file
2) edit the post, check to create a new revision, remove the original image and upload a new image, and submit
3) edit and delete the post
4) you'll see the new image removed from the files/field/image folder, yet the original remains
5) examining the file_managed table in mysql, you'll see an entry for the original.

you'll also see an entry in the file_usage table.

thinking about it, it's most likely in the node system, to do with revisions not properly decrementing the count in file_usage when deleting.

oddly, deleting a single revision does indeed properly release the file. it's only when you delete a node with revisions that this occurs.

Does this only happen with media module, or other files too?

Hi,

Agree with #6 and i faced with this trouble too in 7.x (having orphans files with revision enable and a image field which heavy used file element and process).

I have understand that when a node is delete, it did't delete revisions associated by invoking appropriate hooks (node_revision_delete and field_attach_delete_revision) for example by using node_revision_delete().
As a result, hook_field_revision_delete aren't called and file module did'nt decrement file usage.
Indeed, you need to delete revision one by one and then delete node itself !

Or implements a hook node delete and do what the node module should do (?) : load revisions and for each of them, execute node_revision_delete() (or do it manually whatever).
E.g :

function MYMODULE_node_delete($node) {
  $revisions = node_revision_list($node);
  foreach ($revisions as $revision) {
    //By executing this method, it will call revision hook
    //File field revision hook is called and decrement file usage properly.
    //If no other nodes and revisions use this file, it will be deleted.
    node_revision_delete($revision);
  }
}

Here is a quick unstable 7.x patch for example.
But the real problem is : modules invoke in node delete maybe need to load revisions data about the current node beeing deleted (for e.g field revision value). But mostly do it already rather than using revision hook ?
By deleting revisions first, it will just breaks these invoked modules behaviors...
And i'm sure that i miss a lot of things...!

forgot to say that in an extrem use case, if we have 10000 revisions to delete in one shot per node to delete in a huge transaction, sure some bad behavior will happen on the server! And this is maybe one reason why it haven't been done...
maybe using a cron to create/update a limited queue to delete orphans file (forced) is a better solution (from file_usage table, make a left join on node table and select nid where nid is null)...

Have a patch but it seems to need work and tests.

Hi.

We need this solution urgently for D7.

Is there a patch that we may test?

Thank you very much.

Regards,
Gilsberty

Is there a module out there that will clean-up orphaned files? Or a recipe for using VBO to view a list of orphaned files with the checkboxes to delete them?

I did some research and found these resources -- thought I have not yet tried them:

https://www.drupal.org/project/auditfiles
https://www.drupal.org/node/733258#comment-5582764

I submitted a patch for the same issue (https://www.drupal.org/node/2393641), but quickly marked it as a duplicate when I found this (the patch here makes more sense as it allows for node_revision_delete() hooks).

@capellic - I've used the following SQL to remove the orphans manually:

DELETE from file_usage WHERE fid IN (select fid from (select fu.fid as fid from file_usage fu LEFT JOIN node n ON n.nid = fu.id WHERE n.nid IS NULL) as tt);

Is there any reason why we couldn't also add this? After all, if the node is deleted, so should all revisions and all file use references to the file, correct?

I supposed using only the hooks allows for modules more control, but as I see it, if a node is deleted, there shouldn't be any references in the file_usage table which, of course, references the non-existence node id.

db_delete('file_usage')
       ->condition('id', $nids, 'IN')
       ->execute();

After some testing with the core patch that invokes revision deletion, I've verified that, at least in my case, the file_usage table record is still not being released. The code above tests out to work correctly. It removes the records from the file_usage table and allows the file to be deleted. This includes deleting files when deleting nodes, due to the fact that node deletion invokes file deletion.

Just confirming that #10 works (although line numbers are different now and it might not be the best solution as the author says) and is still needed in D7.38

Bumping to major since this is a data-integrity issue.

Discussed with @catch, @effulgentsia, @xjm, @amateescu, @plach, @berdir, @swentel, and @fago. We agreed this issue is major.

Holy cow, we've had this bug since 2011, and it also exists in Drupal 7?!? This is impressive.

I also confirmed this issue is still a problem while working on #2725415: Text Editor module fails to track usage of images uploaded in text_with_summary fields, allows uploaded images to be deleted.

Also setting to active since the existing patch on the issue has no bearing on D8. For D7, we can create a separate followup issue postponed on this one, per the updated backport policy: #2462101: [policy, no patch] Update backport policy to account for 8.x minor versions and 7.x divergence.

I would like to propose that this is the responsibility of the file module not the node module.

This patch will fix the bug for later deletions, but the existing file usage records are going to remain corrupted - we don't have any way to rebuild file usage from scratch, the API doesn't allow for this.

So while the files themselves don't get lost in this case (in fact it makes them almost impossible to lose), we are losing data in file usage that can never be repaired. So I think this should be promoted to critical.

#25 seems worth discussing to me. Promoting to critical for now for visibility since it is a data integrity problem, although it is kind of borderline since it's theoretically possible to manually check the uses, then confirm that the referenced thing does not exist. I'm leaving the major triage tag on purpose in case we re-demote it.

Edit: I should say that with revisions on by default in 8.2.x, this feels more critical to me there since it becomes a whole lot worse for new sites, but I might consider it major for 8.1.x.

#2732393: Decide if and how to try to save incorrectly orphaned images uploaded in text_with_summary fields from #2725415 also has a need to rebuild usage data (but for the opposite reason), so adding that as a related issue here. It's possible that both of these issues could have data updates resolved by the same functionality.

OTOH is this actually so hard to clean up? All you have to do is an inner join on the usage table and relevant entity tables. If no entities listed in the usage exist, then that's a list of files to mark for purge.

@catch @xjm See #2708411-29: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations for an example of how you can recalculate file usage data.

That works for editor module, but afaik this affects every file_usage implementation that could possibly be applied to any revisionable entity - due to the way that revisions are deleted. So I don't think it's an equivalent case for which generic code can be written - we'd need to know what editor, file/image and any other modules are doing.

Well I guess we could write an update for each type we know about in core to clear those out, then a change record for contrib. So not impossible to do something.

I marked #2393641: File usage records for files referenced in older node revisions are not removed on node deletion (the Drupal 7 issue) postponed on this one.

#10 and #11 look like an accurate description of the problem for both Drupal 7 and Drupal 8 (revisions don't get formally deleted when an entity is deleted) but I agree with some of the concerns mentioned there.

So another way to do it could just be to have File module, when it reacts to an entity being deleted, try to find all files referenced by all revisions of the entity (rather than just the current version of the entity) and try to delete (D7) or delete usage records for (D8) those.

I added a number of related issues. Files in revisions have lots of issues and maybe this should be considered as a whole to see how this area can be improved. The related issues mention potential access/security issues as well.

Deleting individual revisions when we delete an entity is probably the most robust way to deal with this and avoid similar issues in other places. That really needs #89181: Use queue API for node and comment, user, node multiple deletes though since revisions are unbounded.

Opened an issue to discuss that.

Looks like you forgot to reference the issue @catch?

Whoops, was trying to add to related issues mostly likely. Here it is: #2743345: Revisions should be individually deleted when deleting entities

I'm currently working on this issue. There is at least test - I'll attach it, before fix.

Here is patch with test for issue.

Thanks @mtodor! Setting NR to trigger testbot and demonstrate the test.

Here is patch with possible solution for problem. It's full patch with Test included.

To describe solution in short:
- new method is added to 'file.usage' service (deleteForEntity)
- that method is executed after all fields are deleted for entity, to cleanup file relations for that entity

In future some better generic solution should be introduced, for example: to chain deletion and propagate to entity revision, where revision would take care of proper deletion of it's fields and so on. In short to delegate responsibility to elements that should be aware of made action and handle it accordingly. I don't know is that part of #2743345: Revisions should be individually deleted when deleting entities.

Here is new solution of problem. This option actually replaces code that is not doing proper deletion of entity related files.

In process of changing I have found other issue: when field is deleted for that entity type, file usage still stays unchanged for not active revision of entity. Test for that problem is added too.

@alexpott, @catch, @effulgentsia, @Cottser, and I discussed this issue today and agreed to handle it as a critical issue. While it is not data loss per se, orphaned files are a data integrity problem and could also result in problems like unintended information disclosure (since site users have no reason to expect that files attached to something they deleted would still exist.) This is also another data integrity problem that becomes worse the longer it exists, so it does make sense to explore a hotfix as @alexpott and I discussed in person with @mtodor at the sprint.

Thanks all! Looks like the next step is to work more on @mtodor's patch from #46 and then review it.

Fix to use the right media tag. Sorry.

This is also critical for phase A of the workflow initiative.

The approach from the latest patches can not work because we have to take translations into account and they are not stored in the file_usage table.

Instead, I managed to write a tricky aggregated entity query which gives us the file IDs used by all revisions of an entity, with optional filtering on a specific translation. This works fine for all uses cases but one: when we delete the field, the query can not find the renamed field tables anymore.

Not sure how to fix that yet but I'm posting what I have so far to get some feedback on the approach :)

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileFieldItemList.php
@@ -80,9 +81,31 @@ public function postSave($update) {
+    foreach (File::loadMultiple($fids) as $file) {

We should have an empty $fids check else we'll load all the files.

We should have an empty $fids check else we'll load all the files.

Nope, passing an empty array to loadMultiple() makes it return an empty array, so we're good.

Discussed this issue with @catch today and we concluded that we should postpone it on the only viable solution that we have so far: deleting individual revisions.

In order for that to happen, we need at least two things: use the Queue API for deletion (#89181: Use queue API for node and comment, user, node multiple deletes) and have some kind of archiving capabilities in order make any entity 'unpublished' until the deletion process is finished (#2725463: WI: Add StatusItem field type to store archived state).

If this is postponed on that, then those would need to be critical too I think.

Postponing this on those issues worries me though; the first has been stuck for years.

I don't think there's a viable solution that can be done without piggybacking off those issues though - anything we come up with will be pretty fragile, which is not what we want. amateescu spent two days trying to fix this without deleting individual revisions, spoke to me about it in irc, and we ended up with this plan.

This is almost a duplicate of #89181: Use queue API for node and comment, user, node multiple deletes really, and the specific bug here has been around since 2010/11 when file_usage was added in the first place.

From the issue summary there:

When deleting large datasets the delete multiple API does not invoke all dependent hook_[node|comment|user]_delete.
e.g. deleting a node does not invoke hook_comment_delete()

i.e. fixing that issue, then applying it to revisions, should mean this issue only needs test coverage.

I've bumped #89181: Use queue API for node and comment, user, node multiple deletes to critical for now.

Discussed with @catch, @xjm, @effulgentsia, @Cottser and @webchick. Whilst #89181: Use queue API for node and comment, user, node multiple deletes is a good solution to this problem maybe not doing revision deletes before an entity delete causes problems for contrib that are worse than the potential timeout issues if we do this.

Re-opening the issue to explore doing entity revision deletes to fix it.

Something like this.

We could improve the query to load revision IDs if we could exclude default revision ID if the entity is the default revision. However we can;t do that because #2766135: EntityQuery with condition on the revision field leads to wrong results

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -312,6 +312,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +  public function delete(array $entities) {
   

   Had to double check, but this is called by SqlContentEntityStorage which wraps its parent::delete() call in a transaction, so individually deleting the revisions still happens within a single transaction.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -312,6 +312,29 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +          // The delete revision will be deleted later.
   

   s/delete/default/

   But if we have the entity, don't we also have the default revision ID available, so could do an if check instead?

Otherwise this looks great. It will fall over when entities have hundreds or thousands of revisions (and I've seen one of those recently, albeit via a bug), but that's an issue we can deal with separately - at least in that case with luck the transaction gets rolled back, which is better than just bits of an entity getting deleted silently.

Re #64.1 well we can't be sure that the entities passed are the default revision - so how would we know every entities default revision. Ideally what we could do is exclude the revision IDs for entities that are default revisions that are passed in. However we can't do that because of #2766135: EntityQuery with condition on the revision field leads to wrong results. And even if we could we'd still need the exception handling.

The other option is to load all the revisions here and check but that is exactly what happens in \Drupal\Core\Entity\ContentEntityStorageBase::deleteRevision so that seems meh.

One thing to explore might be a loadMultipleRevision.

Re #64.1 well we can't be sure that the entities passed are the default revision

Good point.

Isn't this faster?

EDIT: I wonder how the 'OR' operator works. Probably is not documented.

IN is a tidier query whether it's faster or not.

One thing I am wondering: Could we add an update path, which cleans up the file_usage table? That sounds though like a major follow up or so, to be honest.

Check if there are entities earlier. In this way we avoid the call to parent::delete() that is checking again the same thing.

@dawehner, added #2788571: Cleanup file_usage table after #1239558.

Well we shouldn't be loading revisions one by one - not doing that is the biggest performance optimisation we can be making here.

I'm also pretty sure that we should have some more explicit testing of this.

Pretty sure we just exposed a nasty bug in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromDedicatedTables()

Hmmm \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables is going to have the same problem but it looks like we don't have a test to catch that. And now were doing something different from loadRevision... mapFromStorageRecords is just meant to work on records keyed by entity ID and not revision ID... ho hum.

But that is fixable :)

Some cleanup in tests.

I'm also pretty sure that we should have some more explicit testing of this.

@alexpott, what exactly do you think it need some additional testing?

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -438,17 +438,67 @@ protected function getFromStorage(array $ids = NULL) {
   +   * {@inheritdoc}
   +   */
   +  protected function loadMultipleRevisions(array $revision_ids) {
   

   Where does this inherit the doc from? And.. why not make it public?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -438,17 +438,67 @@ protected function getFromStorage(array $ids = NULL) {
   +      $revision_ids = $this->cleanIds($revision_ids);
   

   \Drupal\Core\Entity\ContentEntityStorageBase::cleanIds() relies on the entity 'id' key but we're acting on revision IDs.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -644,18 +703,20 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   +  protected function buildQuery($ids, $revision_id = FALSE, $revision_ids = []) {
   

   This feels a bit icky :) Since it is a protected method, why don't we change the existing $revision_id param to also accept an array? Or only an array?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -644,18 +703,20 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   -      $query->join($this->revisionTable, 'revision', "revision.{$this->idKey} = base.{$this->idKey} AND revision.{$this->revisionKey} = :revisionId", array(':revisionId' => $revision_id));
   
   @@ -692,6 +753,13 @@ protected function buildQuery($ids, $revision_id = FALSE) {
   +    if ($revision_id) {
   +      $query->condition("revision.{$this->revisionKey}", $revision_id);
   +    }
   

   Could this change have any performance impact on the query?

Re #79
1. Because making it public can happen in another issue that uses it where appropriate. Maybe we should mark it internal till that followup issue. It inherits from the abstract base class
2. Yerp - needs fixing
3. Because BC but it is all protected
4. If it does our indexes are totally out of whack... I'll run an explain.

A new one which seems related #2787187: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity .

Needs work for more tests, alexpott will need to describe which ones.

Just a note #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger went in yesterday with a @todo for this issue. If revisions are enabled in the test for removing a translation, this issue prevents the file usage from being removed so the tests fail. We should be able to enable revisions in that test as part of fixing this bug (although it would be good to keep a non-revision variant of the test so both cases are covered).

Just a note to anyone who happens to come across this issue while trying to figure out why deleting node revisions doesn't also delete the files associated with those revisions, if you're using workbench moderation you'll need this patch plus the one found here: https://www.drupal.org/node/2353491

I won't link the issue unless somebody thinks it's worthwhile, they're not directly related but it does seem to be a common issue.

It is an issue, not sure how common though, it is the first time I have encountered it in five years, but we have a suite of 93 sites, with issue raised on 2 of them but not ironically the sites using Workbench Moderation.

As they increase usage and complexity of use, I expect this to be raised as more and more of an issue. Currently the solution is to adopt an image naming format to differentiate older images from current ones.

Regards

Tim

So we need more tests to ensure the hooks are being fired in the order we expect. Looking at the previous comments the things that needs addressing is #79.2 and #79.4.

Addressing #79.2

Here's some additional tests around hook firing during entity revision delete. Also fixed the DbLogTest. That one is super interesting. Our hook_*_load hooks take lists of entities keyed by entity ID - not so good for multi-loading revisions. We should probably change that is D9. Or introduce *_load_revision() and deprecate *_load() in D8.

#1730874: Add support for loading multiple revisions at once is still open, although that won't help with the hook signature.

#83 is not addressed. Note that the link from #83 is wrong, should be #2787187: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity .

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -312,6 +312,53 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +  protected function loadMultipleRevisions(array $revision_ids) {
   

   loadMultipleRevision() kind of sounds better to me, but not sure. or loadRevisionMultiple() ? the issue below used loadRevisions()...

   the problem with adding this as a protected method is that adding it as a public method later on in #1730874: Add support for loading multiple revisions at once isn't possible anymore as that would be an API change for subclasses.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -438,17 +438,71 @@ protected function getFromStorage(array $ids = NULL) {
   +      foreach ($revisions as &$revision) {
   

   why are we doing this by reference? the only reason for that would be able to replace the object, that doesn't happen?

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -438,17 +438,71 @@ protected function getFromStorage(array $ids = NULL) {
   +   * @param array|null $revision_ids
   +   *   If not empty, return entities that match these IDs. Return all entities
   +   *   when NULL.
   ...
   +  protected function doLoadMutlipleRevisions(array $revision_ids) {
   

   I think null can only be paassed in if you default to NULL when you have a type hint. definitely for objects, haven't tested array.

   possibly just copied code, I don't think loading all revisions of all entities is something we need to support?

4. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -644,18 +707,20 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   +   * @param $revision_ids
   +   *   Revision IDs to load.
   

   missing type (yes, the exising params too)

5. +++ b/sites/default/default.settings.php
   @@ -144,6 +144,11 @@
   + *
   + * Per-table prefixes are deprecated as of Drupal 8.2, and will be removed in
   + * Drupal 9.0. After that, only a single prefix for all tables will be
   + * supported.
   

   unrelated?

Thanks for the review @Berdir

1. Multiple and then a single thing doesn't make much sense. I've made the method public and @internal. We can remove the @internal in #1730874: Add support for loading multiple revisions at once
2. No good reason - fixed
3. I agree - loading all revisions of all entities seems excessive. However that makes the API a bit weird considering how loadMultiple() works. I've removed for now since adding this back in in #1730874: Add support for loading multiple revisions at once would be simple.
4. Fixed
5. Think that is only part of @claudiu.cristea's patch in #93 and not in his interdiff... and therefore not on my branch.

This will still because I'm not sure that delete translations fires the entity revision delete hooks :(

Here's an improvement of the test coverage that is failing because #93. Once revisions are enabled on a content type removing a translation will not cause any deletes or changes in file usage because removing a translation will only remove the translation from the revision that is being saved. Therefore the file is still being used by the old revision. I've discussed this with @hchonov on IRC and he agreed that calling removeTranslation on an entity should not result in an entity revision delete firing.

So the assumption that the just changing the flag on the content type made in #93 is what we need to do is incorrect. The patch attached adds test cases where revisions are enabled and where they are not.

I've done a bit of digging and come across ContentEntityStorageBase::invokeFieldMethod, which when called for the "postSave" method will invoke the FieldItemList::delete method on each removed translation, which is a bit confusing as the doc of the method FieldItemList::delete says :

"This method is called during the process of deleting an entity, just before values are deleted from storage."

So currently deleting an entity translation still results in calling the field delete method on each removed translation, which is independent of if we are saving the default or a non-default revision with a removed translation.

P.S. This means that FileFieldItemList::delete will be called and should decrement the usage count of the referenced file.

Here is the path how it comes to invoke the delete method on each field of a removed translation:

1. EntityStorageBase::save ->
2. ContentEntityStorageBase::doPostSave ->
3. EntityStorageBase::doPostSave ->
4. ContentEntityStorageBase::invokeHook(update/insert) ->
5. ContentEntityStorageBase::invokeFieldPostSave ->
6. ContentEntityStorageBase::invokeFieldMethod('postSave')

and the last method has the following in its body :

    // 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());
      foreach (array_diff($original_langcodes, $langcodes) as $removed_langcode) {
        $translation = $entity->original->getTranslation($removed_langcode);
        $fields = $translation->getTranslatableFields();
        foreach ($fields as $name => $items) {
          $items->delete();
        }
      }
    }

Okay after discussion translation deletion a bit more and creating #2815779: Deleting a translation leaves behind orphaned revisions. I've added further testing here to show what's happens with revisions, translations and file counting.

I've not followed the issue too closely, but the patch in #102 looks pretty good and has nice test coverage. So going to RTBC to see if we can get things moving again.

I'm not sure that we can fix #2788571: Cleanup file_usage table after #1239558 afterwards. I think we should fix it here.

ok, back to needs work then to add a cleanup for the file_usage table.

Hi all,
I have tested this and:

• I can reproduce the problem following the instructions
• Patch in #102 applied correctly and fixed the problem with revisions and orphan files
• I would like to help on the file_usage table cleanup (detailed in: #2788571). I'm working on it right now and I have some doubts I hope someone can answer:

1. I think we can cleanup the table creating a function that returns a query (draft query pasted below). If this is correct, where should we place this function? In file/src/FileUsage/ files?
2. If previous point is not correct, maybe you can suggest another approach so I can investigate?

$query = db_delete('file_usage', 'fu'); 
$query->leftJoin('node', 'n', 'n.nid=fu.id'); 
$query->isNull('n.nid');
$query->execute();

Will assign the issue to myself so everyone is aware I'm working on this. I hope that's ok.

Thanks,

m

@anairamzap - you've had this assigned to yourself for 2 months. Any progress? Or should we un-assign it at this point?

Hi @kattekrab,
I was waiting for an answer, but given that no movement has been done in this issue I have unassigned it.

Cheers,
m

Working on this at DrupalDevDays Seville.

First, I had to re-roll the patch.
I had an issue when creating the interdiff (because of lines number been different in 2 places).
Here is the re-rolled patch.
Then I will work on adding the file_usage table cleanup.

Same patch as 111 + fix of failed test.

So patch 1239558-114.patch passes tests.
We still have to work on adding the file_usage table cleanup.
I hadn't time in Seville and hope to do this soon.

In #2753971: ContentEntityStorageBase::deleteRevision() function does not remove node_field_revision entries we have a similar situation as was ok to create a follow up to clean the tables later. I think we can do the same here.

Patch looks good, just a small nitpick:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1132,21 +1203,24 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
+       // if (!isset($values[$row->entity_id][$field_name][$langcode])) {
+       //LC   $values[$row->entity_id][$field_name][$langcode] = [];

We should remove these lines if they're not necessary.

Overall, looks good. +1 to RTBC.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -335,6 +335,57 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +   * @internal
   

   nice.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -644,18 +704,20 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
   +  protected function buildQuery($ids, $revision_id = FALSE, $revision_ids = []) {
   

   Minor: Ideally default of $revision_id should be NULL instead of FALSE as it holds integer value. but not changed by this issue. so we can ignore it.

1. #116: Created follow up to clean up file_usage table - #2885434: Deleting a node with file usage doesn't delete file
2. #117: Removed.

Do we still need to work on this issue in 8.3.x?

I've follow the steps described in the issue summary.
Without the patch, files are not deleted.
After applying the patch, files are correctly deleted physically and marked as to be deleted (status=0 in database).

+1 for RTBC

Since this affects the storage I queued a bunch of tests on non-primary environments.

@tstoeckler++

https://www.drupal.org/pift-ci-job/696689 looks like a real postgres failure.

I went through the entire issue.

The following discussions/comments were especially important:

1. @aaron's steps to reproduce in #4 from August 2011 (!!! — this is almost 6 years ago!) are still perfectly reproducible!
2. @chaby's initial solution in #10 plus caveat in #11 about his solution not working when there are many revisions.
3. @joshua.albert asking in #17 why simply removing all file usages for a certain node is not sufficient — which is … a very good point obviously. The funny thing is that we appear to have a hook implementation for that in file.module: file_file_predelete(). But its implementation is a @todo. Turns out this TODO was added in #391330: File Field for Core, almost 8 years ago. @catch approved it there, assuming there'd be a follow-up. That follow-up never happened.
4. From #23–#31, rebuilding all the file usage data from scratch is being considered.
5. @David_Rothstein explained the problem very clearly in #32: revisions don't get formally deleted when an entity is deleted, by which he means revisions are not each individually explicitly deleted.
6. @catch opened an issue in #34-#37 to formally delete each revision: #2743345: Revisions should be individually deleted when deleting entities
7. @mtodor implemented a patch in #43 that uses the approach that @jsohua.albert proposed in #17. He had to expand the API of \Drupal\file\FileUsage\FileUsageInterface to make this possible.
8. @amateescu explains in #53 that the approach taken in #43 (and hence proposed in #17) cannot work, because translations are not taken into account. (This reminds me of #2892377: Document relationship of entities, entity revisions and entity translations.) He proposes an alternative approach, which works for translations, but fails in a different edge case.
9. After discussing it outside this issue, @catch and @amateescu decided in #58 that this most be postponed on deleting individual revisions: #2743345: Revisions should be individually deleted when deleting entities, which is blocked on #89181: Use queue API for node and comment, user, node multiple deletes.
10. In #61 through #66, @alexpott unpostponed this issue, and provided yet another approach: making ContentEntityStorageBase::delete() iterate over all revisions, timeout problems be damned, because many sites won't run in timeout issues, only sites with many revisions for an individual entity. @catch reviewed and +1'd.
11. In #67–#99, this is refined further.
12. @hchonov dug into the code and explains in #100 and #101 that deleting a translation calls FieldItemList::delete(), even though that is meant to only be called when deleting the entity (i.e. deleting all translations, not just a single translation).
13. From #103 onwards, we're on the verge of RTBC'ing, but we want the update path to fix the file_usage table first (in #104 + #105), and then shifted back to "nah let's do it in a follow-up" (in #119 + #121).

Why is it suddenly okay to do this in a follow-up, and why wasn't it before?

Updated issue summary, updated issue tags, updated issue title, updated related issues.

I also stepped through the code with a debugger. Based on the >100 comments above, many of which show very smart people attempting solutions but then running into dead ends … that shows that the semantics & architecture of deleting entities vs deleting translations vs deleting revisions is still a weak area of the Entity/Field API.

When deleting an entity (with no translations)

$entity->delete() calls SqlContentEntityStorage::delete() which calls EntityStorageBase::delete() which calls ContentEntityStorageBase::doDelete() which calls ContentEntityStorageBase::invokeFieldMethod(), which iterates over all translations and invokes the delete() method on all FieldItemList objects. After ContentEntityStorageBase::doDelete() called invokeFieldMethod(), it also calls doDeleteFieldItems(), which is what e.g. deletes all revisions for this entity.

In case of deletion, FieldItemList::delete() should iterate over all revisions, (so it's not ContentEntityStorageBase::invokeFieldMethod() that should iterate over all revisions), because the docs of \Drupal\Core\Field\FieldItemListInterface::delete() say:

  /**
   * Defines custom delete behavior for field values.
   *
   * This method is called during the process of deleting an entity, just before
   * values are deleted from storage.
   */
  public function delete();

That's only removing the usage of the default revision, not of all files across all revisions. It's not reasonable to expect some hook to be invoked for every revision, because there might be hundreds, and most things don't need to do per-revision clean-up. As the documentation indicates, this is the method that is called when deleting an entity. Therefore if this field type does revision-specific stuff, it also is itself responsible for doing revision-specific clean-up.

When deleting a non-default entity translation

(Because deleting the default entity translation is equivalent with deleting the entire entity — the UI makes this clear fortunately.)

@amateescu pointed out in #53 that this is wrong because it handles translations incorrectly. But as @hchonov pointed out in #100/#101: it's wrong that translation deletion calls FieldItemList::delete(), because it's designed to be used for "entire entity deletion, which also means all translations".

I stepped through this too. ContentEntityDeleteForm() calls $entity->removeTranslation($langcode) and then calls $entity->save(). Via the call chain shown in #101, this eventually results in FieldItemList::delete() in being called. Which is wrong per the \Drupal\Core\Field\FieldItemListInterface::delete() docs.

Therefore I think that the current patch is wrong.

I think we need to fix two bugs:

1. Deleting an entity with a file/image field does not release all usages of those files by the entity that is being deleted.
2. Entity translation deletion is implemented incorrectly.

The current patch only fixes the first bug, and does it in a way that makes it harder to fix the second bug.

Multiple solutions can be thought of for both of those:

1. 1. Fix FileFieldItemList::delete() by making it iterate over all revisions, and deleting those individual usages.
   2. Fix FileFieldItemList::delete() by deleting all usages of this file by this entity in one go (implemented by @mtodor in #43, based on #17).
   3. Equivalent with #2, but implemented more generically: remove FileFieldItemList::delete()'s logic to deal with file usage altogether, and instead move that into hook_entity_delete, which means it will also work for e.g. editor.module
      file usage and #2826127: Usage of Files Referenced in Entity Reference Fields Not Tracked. Note that this was always the plan in #391330: File Field for Core, 8 years ago, it just never happened…
2. 1. Fix \Drupal\Core\Entity\ContentEntityStorageBase::invokeFieldMethod()'s special casing: stop calling delete() on all fields when removing a translation. We have \Drupal\Core\Field\FieldItemListInterface::deleteRevision() and we have hook_entity_translation_delete(). It seems we're missing \Drupal\Core\Field\FieldItemListInterface::deleteTranslation()?

   Related: #2787187: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity fixed the case of "deleting a translation causes all file usage to be fixed", but this too was due to translation deletion calling FieldItemList::delete(), so that attempted to change the logic in FieldItemList::delete() to only delete usage in case the default translation is being deleted, but … that too is wrong! When file A is used in translation X and file B is used in translation Y, then deleting the non-default translation means that file B's usage will never reach zero. Reopened that issue because of that: #2787187-47: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity .

All in all, I think it's important that Entity/Field API subsystem maintainers make a decision here that considers all ramifications. Either we go with a different patch than the current one that fixes both bugs, or we go with the current patch, but then we must fix the documentation of \Drupal\Core\Field\FieldItemListInterface::delete() and document how to detect whether a single translation is being deleted or whether the entire entity is being deleted.

I did not dive as deep into this issue as @Wim Leers did (wim++), but off the top of my head I totally agree with everything said in #127.

Currently, my way forward would be: implement solutions #127.1.3 and #127.2. A ::deleteTranslation() method should also allow us to properly address the follow-up mentioned in #2787187-47: Data loss: Deleting a translation of an entity deletes all file_usage entries for files referenced by the entity .

I'd like to explore this solution and provide a new patch if there's consensus this should be the way forward.

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.

Spoke with @catch and we agreed this no longer needs to be a MUST-HAVE for the Workflow Initiative, as lingering files are now accepted "by design".

Having looked/proposing a patch at https://www.drupal.org/node/2810355 (Before noticing this thread, is now linked there aswel).

"Deleting an entity with a file/image field does not release all usages of those files by the entity that is being deleted."
In an ideal conceptual code world I guess this could be by handling every one of revisions/translation instances separately however in reality this seems incredibly heavy handed (200 revisions x 4 languages would be incredibly painful at best).

The file usage however does aggregate to the "root" entity id so in theory it can be flushed out in a single go when deleting the main entity.

Assumptions in the "delete" callback.
There appear to be a few ways that the "delete" callback on the fieldList is called. But as far as I can see from the previous comments these don't always seem to match to the idea that it"s only when the entire node is deleted.
e.g:
- The default language of a node is changed (causing a "new" field instance to be added in the new language and the old one removed (invoking the delete callback) as seen in the related https://www.drupal.org/node/2787187
- Deleting a revision (basically a node, which assumes only this revision)
- Deleting a "current" revision (assuming all the connected revisions)
- Deleting a translation.
- Deleting the default translation (which assumes all the other translations)

Now in this sense I disagree with the assumption of the delete callback automatically originating from a "full entity delete". It might be the most common usecase but it's not an inherent given and trying to start guessing the case in this context feels odd. Conceptually speaking the we're deleting the "field instance" and we shouldn't be concerned about the larger context of why it's being deleted. The larger context (e.g the deleting of a node) should be concerned about calling the correct callbacks.

Roughly the current flow:
Something is changed / deleted --> Invokes the invokeFieldMethode [ which then tries to guess it's context from the passed entity ] --> edits the file usage.

But in my mind the flow should be
Something is changed / deleted --> This something decides the context (or asks some form of central point to do it) --> invokes the associated method --> edits the file usage.

So (as mentioned by Wim)
1. Fix \Drupal\Core\Entity\ContentEntityStorageBase::invokeFieldMethod()'s special casing: stop calling delete() on all fields when removing a translation. We have \Drupal\Core\Field\FieldItemListInterface::deleteRevision() and we have hook_entity_translation_delete(). It seems we're missing \Drupal\Core\Field\FieldItemListInterface::deleteTranslation()?
Seems the more logical case to implement without overhauling toomuch.

2. The other way would is to "expand" / solidify the internal checks in the invoker by using
$entity::TRANSLATION_REMOVED type checks more rigorously. But that would mean that detecting revisions is done the other way that detecting translations, even if the use case is pretty much the same. And the hook / invoke logic feels more general in how drupal does things.

bump

Hm. This might need to be re-escalated. The lingering files, while no longer resulting in data loss (which, hooray!), now results in non-compliance with GDPR or other laws that require the deletion of files.

@webchick: We already have #2821423: Dealing with unexpected file deletion due to incorrect file usage as a meta issue for file usage problems and it's still critical. It's just that nobody cared about it anymore :) And I think there is also an issue about being able to delete manually, which I think would already mostly adress the legal concerns. We have generic entity actions now, so that shouldn't be too hard. If we can combine it with a view that shows files without usage and bulk-delete, then that should be pretty OK.

Right, I realize about the existence of #2821423: Dealing with unexpected file deletion due to incorrect file usage, but that feels a lot more like "in order to fix this bug, you must first create the universe" issue. :P Whereas this one seems, at least on the surface, to have more targeted fixing possibilities.

Hm, adding a way to manually delete files is something we can do any time, that's fairly easy. There are no blockers for that.

This issue is open for 8 years now, there is a reason for that. And it's not just about fixing this single issue, we have to fix every file usage problem to enable *automatic* deletion again. There are issues that remove too much usage or don't add enough, and others, like this one don't *remove* enough usages, so an issue like this might be hiding other issues that we haven't even found yet.

Re #136:

This might need to be re-escalated. The lingering files, while no longer resulting in data loss (which, hooray!), now results in non-compliance with GDPR or other laws that require the deletion of files.

Indeed.

And I think there is also an issue about being able to delete manually, which I think would already mostly adress the legal concerns.

I don’t think so. While being a stop-gap, the mere fact that it’s possible to delete lingering files manually does not qualify as data protection by design (Art. 25 GDPR) in regard to the right to erasure (Art. 17 GDPR), see also the recitals 39, 46, and 65.

Now we’re no magicians, so as Berdir said, have to fix every file usage bug erring on one side or the other, before we can reenable automatic clean-up. However, I suggest we don’t accept lingering files as “by design” and pick up the proposal laid out in #127 and 128 by Wim and plach.

Maybe related issue in contrib: #2910053: Used by another application

Hello everyone,

i am also facing this issue but i didn't get any solution. any solution for this problem?

Thanks

I think comment #28 should be fixed, and it shouldn't be that hard to do. In my specific case, I reached 100% of my quota on the web server I use because there are tons of orphaned PDF, Word, etc... files which have not been deleted from the server when the corresponding nodes have been deleted from Drupal. The file_usage table contains many entries which point to a non-existent node, and these entries should be easy to catch.

I agree this issue should have more attention, this can become a serious problem for users with smaller disk spaces.

Added credit for maseyuk from duplicate #3051952: Editor file usage count not decreasing correctly if someone ever saves a node without editing

Hi,

Can anyone confirm that #119 patch is safe to use?

Kind regards,
Michal. K.

Patch #119 with drupal 9.4.7 causes Fatal error: Cannot redeclare Drupal\Core\Entity\ContentEntityStorageBase::loadMultipleRevisions() in /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php on line 836

Patch #119 can't be applied to Drupal 9.5.10

I think we should postponed this on #2821423: Dealing with unexpected file deletion due to incorrect file usage

@steinmb This is one of the issues noted in the table in #2821423: Dealing with unexpected file deletion due to incorrect file usage. And I don't know of a reason work on this cannot proceed in parallel to the others there?

As the notes column there says regarding this issue: "No data loss, but irrepairable, therefore still critical."

file_usage does not seem to get touched when a filefield gets deleted.
So we can not even find those files anymore that had been referenced (= used) by the deleted field. At least not in bulk.
All files still keep value "1" in column "status" .
At least this value should have changed after deletion of the field.
Am I wrong?

This should really be postponed on #3364744: Add a reliable entity-usage system to core, the file_usage system is broken beyond repair, there is no way to rebuild file usage, so as soon as it's off by one or more, it's like that forever.

I posted a potential solution/workaround in #3427640: File not marked temporary and usage not updated if only used in past revisions when node is deleted

I also disagree that this should be postponed. Leaving it as-is will continue to create incorrect file usage data any time someone deletes content, which will only make their problems worse. The solution I shared in the previous comment isn't that difficult or complicated.

Please review and/or try the code posted in the linked issue summary. Note that I didn't test it with media, so the code might need to be further generalized to handle that case.