Managed file garbage collector should delete file entity even if physical file does not exst. [#2725457]

Problem/Motivation

The managed file garbage collector deletes unused temporary file entities and related files if they haven't been changed for a specified age. The entity is only deleted if the file exists. If the file does not exist then the entity is not deleted and a "Could not delete temporary file "%path" during garbage collection" message is written to the log.

It seems to me that if the desired end result is that both the entity and file be deleted then I don't see why the absence of the file should stop the entity from being deleted.

Of course, in a perfect world, this should never happen. Files and entities should not get out of sync but if it does happen, whatever caused it is lost in history as far as the file module is concerned. It happened to me when I was doing some module development with a form involving an upload. As far as I can see, as an admin, I have no way to delete those entities.

Proposed resolution

I think the entity deletion should proceed if the file (exists and is_writeable) or does not exist. I'm suggesting the is_writeable test because I think that pretty much implies "is_deleteable". If there is some sort of file permissions problem then we probably don't want to delete the entity and leave an orphan file. As it is now, we leave an orphan file if the file delete fails.

Remaining tasks

Discuss if this makes sense. I may be missing a good reason why it was coded like this in file.module.

Create a patch
I'm happy to have a go at creating a patch.

Create a test?
I'm not sure if it would be possible to create a meaningful test for the code in hook_cron.
Perhaps the code in hook_cron which decides "should this be GC'd" could be moved somewhere else. Perhaps an isGarbageCollectable($age) method in the file entity. That would be more testable.

User interface changes

None

API changes

None

Data model changes

None

Comment	File	Size	Author
#3	file_garbage_collection-2725457-3.patch	1.13 KB	Reuben Unruh
#3	8.3.x: PHP 5.5 & MySQL 5.5 21,060 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

14 May 2016 at 19:30

tetranz created an issue. See original summary.

Comment #2

14 May 2016 at 19:30

Version:

8.2.x-dev

» 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #3

Reuben Unruh CreditAttribution: Reuben Unruh commented 13 October 2016 at 15:10

Status:

Active

» Needs review

File	Size
file_garbage_collection-2725457-3.patch	1.13 KB
8.3.x: PHP 5.5 & MySQL 5.5 21,060 pass

Comment #4

13 October 2016 at 15:10

Version:

8.3.x-dev

» 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #5

gapple

he/they

English

CreditAttribution: gapple as a volunteer commented 19 May 2017 at 21:22

Some history trawling:

Looks file_cron was added by #1468328: Move file entity info, managed file, and file usage functionality into File module in August 2012, before which it had the same behaviour in system_cron().
The particular check was added by #1401558: Remove the usage handling logic from file_delete() in May 2012 with this change:

       if (empty($references)) {
-        if (!file_delete($file)) {
+        if (file_exists($file->uri)) {
+          file_delete($file);
+        }
+        else {
           watchdog('file system', 'Could not delete temporary file "%path" during garbage collection', array('%path' => $file->uri), WATCHDOG_ERROR);
         }
       }

From what I can tell, the concern that kept the file_exists() check is if the file used a disabled stream wrapper. If that is still a concern, I think a more targeted check could be made so that a valid stream wrapper with a non-existent file doesn't result in perpetual error messages. This was discussed briefly in that issue, but appeared to be forgotten as a concern in the final patch reviews.

On to the patch in this issue:

- I think if the file exists but is not writable, the watchdog should still receive an error, versus the change to a warning.
- If the file doesn't exist and $file->delete() was successful, I don't think that a warning should be logged at all. The current message also implies that the delete was not successful.