Symptoms

Files from a file field linger even after a node has been deleted.

Problem

Revisioning adds lots of data to the file_usage table and never removes any of it. This prevents file_field_delete() from actually removing files.

Proposed Solution

A. Revisioning does not add file_usage data at all, and leaves everything up to the File module.

Perhaps the maintainers can explain why usage data is being added in the first place. It looks like the File module already adds usage for revisions.

B. Revisioning deletes its usage data when appropriate, i.e. in hook_node_delete() and hook_node_revision_delete().

This would require some fixes to Revisioning's use of file_usage_add(). Currently file usage is always incremented regardless of whether or not a new revision is being created. It also uses the $node->vid instead of $node->nid, which complicates things and is incorrect according to the file_usage_add() documentation.

CommentFileSizeAuthor
#26 interdiff_25-26.txt1.83 KBmarco-s
#26 revisioning-2285629-26-file-usage.patch10.56 KBmarco-s
#25 interdiff_20-25.txt1.73 KBmarco-s
#25 revisioning-2285629-25-file-usage.patch10.55 KBmarco-s
#20 revisioning-2285629-20-file-usage.patch10.86 KBducktape
#18 revisioning-2285629-18-file-usage.patch10.75 KBducktape
#15 revisioning-2285629-15-file-usage.patch9.24 KBmilesw
#13 revisioning-file_usage-2285629-13.patch9.19 KBdrasgardian
#12 revisioning-file_usage-2285629-12.patch9.16 KBdrasgardian
#11 revisioning-file_usage-2285629-11.patch9.13 KBdrasgardian
#10 revisioning-file_usage-2285629-10.patch7.56 KBdrasgardian

Comments

milesw’s picture

milesw commented

Here is a workaround to allow files to be deleted.

/**
 * Implements hook_node_delete().
 */
function mymodule_node_delete($node) {
  if ($revisions = node_revision_list($node)) {
    $vids = array_keys($revisions);
    db_delete('file_usage')->condition('module', 'revisioning')->condition('id', $vids, 'IN')->execute();
  }
}
milesw’s picture

milesw commented

There is also a core issue that causes files on old revisions to linger:

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

milesw’s picture

milesw commented
Related issues: +#1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger

  • RdeBoer committed 2febc12 on 7.x-1.x authored by milesw 
    Clean up file usage when deleting a node [#2285629]
rdeboer’s picture

rdeboer
Location Melbourne
commented

I'm not sure anymore about the history of the code adding file usage and whether it was always the case that the File module added this.

I've adopted your solution B and incorporated your hook in revisioning.module with attribution.

Thanks Miles!
Rik

rdeboer’s picture

rdeboer
Location Melbourne
commented
Assigned: Unassigned » rdeboer
Status: Active » Fixed

Status: Fixed » Closed (fixed)

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

drasgardian’s picture

drasgardian commented
Status: Closed (fixed) » Needs work

Sorry to reopen, but I don't think node deletes are the only cases where this needs to be addressed.

If a file is removed from a node and replaced with another then the original file is not cleaned up because of revisioning's file_usage entries.

drasgardian’s picture

drasgardian commented

small update.. the origins of all the file_usage_add calls are from this issue: #1901272: Editing an unpublished revision more than once causes any files removed in first draft edit to disappear from published revision.

Revisioning needs to be decrementing file_usage counts when a revision is deleted and where a revision is updated and a file is removed.

drasgardian’s picture

drasgardian commented
Status: Needs work » Needs review
StatusFileSize
new revisioning-file_usage-2285629-10.patch7.56 KB

The attached patch takes better care of the file_usage records by decrementing them when files are removed. It also uses the nid instead of vid for the file_usage id.

drasgardian’s picture

drasgardian commented
StatusFileSize
new revisioning-file_usage-2285629-11.patch9.13 KB

Attached is another patch. This adds an update hook to rebuild all the existing revisioning file_usage records and should leave the usage counts in the correct state.

drasgardian’s picture

drasgardian commented
StatusFileSize
new revisioning-file_usage-2285629-12.patch9.16 KB

update to patch. last one was throwing an error when there wasn't a file field.

drasgardian’s picture

drasgardian commented
StatusFileSize
new revisioning-file_usage-2285629-13.patch9.19 KB

updated patch attached. I found it was throwing an error when saving user profiles because there was no bundle available.

braindrift’s picture

braindrift commented
Status: Needs review » Needs work

After that patch I got errors when I save other entities than nodes with file fields

milesw’s picture

milesw commented
Status: Needs work » Needs review
StatusFileSize
new revisioning-2285629-15-file-usage.patch9.24 KB

Updated #13 to fix issue with the list of file fields.

Also addresses #14 by only handling nodes in revisioning_field_attach_update().

davy-r’s picture

davy-r commented

I've successfully tested patch #15, it fixed the issue, thanks!

corentin.crouvisier’s picture

corentin.crouvisier commented

I think this line needs to be added in revisioning_uninstall() in revisioning.install to remove file_usage with the module uninstall:

db_delete('file_usage')->condition('module', 'revisioning')->execute();

ducktape’s picture

ducktape commented
StatusFileSize
new revisioning-2285629-18-file-usage.patch10.75 KB

I updated the database update hook with batch api for large sites.

I've also added corentin's suggestion to the uninstall hook.

Status: Needs review » Needs work

The last submitted patch, 18: revisioning-2285629-18-file-usage.patch, failed testing. View results

ducktape’s picture

ducktape commented
StatusFileSize
new revisioning-2285629-20-file-usage.patch10.86 KB

Fixed the patch.

koekto333’s picture

koekto333 commented

Patch from https://www.drupal.org/node/2285629#comment-10388265 still doesn't work properly for me.

erwangel’s picture

erwangel commented

Thank you for patch #20 that cleaned fine all unwished revisioning entries on updating. However I was expecting (although it is not in code) it should also remove the entries in file_managed table as well as delete the corresponding physical files. Removing only file_usage entries makes them orphaned and still we have entries in file_managed table. Perhaps it is enough to just set managed entries status to 0 (temporary files) so files are deleted with next cron. I think this way update_7108 would be complete. I had to manually delete more than 40,000 files with which "revisioning" had pollute the drive, hopefully with some help of Fancy File Delete module.

koekto333’s picture

koekto333 commented

It appears to be a patch from #20 works well for me either.
erwangel, i'm using patch #20 for about 2 month, i don't know its structure, but Fancy File Delete doesn't show me any new orphaned files.

erwangel’s picture

erwangel commented

@koekto333 For me also, it works fine for new revisions. At #22 I was talking about revisions created before installing the patch. The patch was of great help as removing entries from file_managed table it permitted the Fancy File Delete to see them as orphaned; and perhaps it is safer to clean like that in two steps. But from a user/admin point of view this is not clear enough. One could think that the (old) files are cleared after running the patch. This is not the case. This has to be done in a second step with Fancy File Delete.

marco-s’s picture

marco-s
Location Switzerland🇨🇭
commented
StatusFileSize
new revisioning-2285629-25-file-usage.patch10.55 KB
new interdiff_20-25.txt1.73 KB

I have updated the patch #20 according to the current dev version (dev-1.x f16bbfb).

marco-s’s picture

marco-s
Location Switzerland🇨🇭
commented
StatusFileSize
new revisioning-2285629-26-file-usage.patch10.56 KB
new interdiff_25-26.txt1.83 KB

Fixed 'array to string conversion' issue which I had with newer PHP versions.