Issue #1401558 by exlin, Berdir, tim.plunkett: Remove the usage handling logic from file_delete().

core/includes/file.inc

@@ -696,9 +696,6 @@ function file_usage_list(stdClass $file) {
 /**
  * Records that a module is using a file.
  *
- * This usage information will be queried during file_delete() to ensure that
- * a file is not in use before it is physically removed from disk.
- *
  * Examples:
  * - A module that associates files with nodes, so $type would be
  *   'node' and $id would be the node's nid. Files for all revisions are stored
@@ -731,14 +728,17 @@ function file_usage_add(stdClass $file, $module, $type, $id, $count = 1) {
     ->fields(array('count' => $count))
     ->expression('count', 'count + :count', array(':count' => $count))
     ->execute();
+
+  // Make sure that a used file is not permament.
+  if ($file->status != FILE_STATUS_PERMANENT) {
+    $file->status = FILE_STATUS_PERMANENT;
+    file_save($file);
+  }
 }
 
 /**
  * Removes a record to indicate that a module is no longer using a file.
  *
- * The file_delete() function is typically called after removing a file usage
- * to remove the record from the file_managed table and delete the file itself.
- *
  * @param $file
  *   A file object.
  * @param $module
@@ -786,6 +786,14 @@ function file_usage_delete(stdClass $file, $module, $type = NULL, $id = NULL, $c
     $query->expression('count', 'count - :count', array(':count' => $count));
     $query->execute();
   }
+
+  // If there are no more remaining usages of this file, mark it as temporary,
+  // which result in a delete through system_cron().
+  $usage = file_usage_list($file);
+  if (empty($usage)) {
+    $file->status = 0;
+    file_save($file);
+  }
 }
 
 /**
@@ -1109,8 +1117,8 @@ function file_move(stdClass $source, $destination = NULL, $replace = FILE_EXISTS
     // Inform modules that the file has been moved.
     module_invoke_all('file_move', $file, $source);
 
-    if ($delete_source) {
-      // Try a soft delete to remove original if it's not in use elsewhere.
+    // Delete the original if it's not in use elsewhere.
+    if ($delete_source && !file_usage_list($source)) {
       file_delete($source);
     }
 
@@ -1283,19 +1291,12 @@ function file_create_filename($basename, $directory) {
 /**
  * Deletes a file and its database record.
  *
- * If the $force parameter is not TRUE, file_usage_list() will be called to
- * determine if the file is being used by any modules. If the file is being
- * used the delete will be canceled.
+ * Instead of directly deleting a file, it is strongly recommended to delete
+ * file usages instead. That will automatically mark the file as temporary and
+ * remove it during cleanup.
  *
  * @param $file
  *   A file object.
- * @param $force
- *   Boolean indicating that the file should be deleted even if the file is
- *   reported as in use by the file_usage table.
- *
- * @return mixed
- *   TRUE for success, FALSE in the event of an error, or an array if the file
- *   is being used by any modules.
  *
  * @see file_unmanaged_delete()
  * @see file_usage_list()
@@ -1303,42 +1304,21 @@ function file_create_filename($basename, $directory) {
  * @see hook_file_predelete()
  * @see hook_file_delete()
  */
-function file_delete(stdClass $file, $force = FALSE) {
-  if (!file_valid_uri($file->uri)) {
-    if (($realpath = drupal_realpath($file->uri)) !== FALSE) {
-      watchdog('file', 'File %file (%realpath) could not be deleted because it is not a valid URI. This may be caused by improper use of file_delete() or a missing stream wrapper.', array('%file' => $file->uri, '%realpath' => $realpath));
-    }
-    else {
-      watchdog('file', 'File %file could not be deleted because it is not a valid URI. This may be caused by improper use of file_delete() or a missing stream wrapper.', array('%file' => $file->uri));
-    }
-    drupal_set_message(t('The specified file %file could not be deleted because it is not a valid URI. More information is available in the system log.', array('%file' => $file->uri)), 'error');
-    return FALSE;
-  }
-
-  // If any module still has a usage entry in the file_usage table, the file
-  // will not be deleted, but file_delete() will return a populated array
-  // that tests as TRUE.
-  if (!$force && ($references = file_usage_list($file))) {
-    return $references;
-  }
-
+function file_delete(stdClass $file) {
   // Let other modules clean up any references to the file prior to deletion.
   module_invoke_all('file_predelete', $file);
   module_invoke_all('entity_predelete', $file, 'file');
 
-  // Make sure the file is deleted before removing its row from the
-  // database, so UIs can still find the file in the database.
-  if (file_unmanaged_delete($file->uri)) {
-    db_delete('file_managed')->condition('fid', $file->fid)->execute();
-    db_delete('file_usage')->condition('fid', $file->fid)->execute();
+  // Attempt to delete the file on the filesystem. Failures will be logged in
+  // watchdog.
+  file_unmanaged_delete($file->uri);
 
-    // Let other modules respond to file deletion.
-    module_invoke_all('file_delete', $file);
-    module_invoke_all('entity_delete', $file, 'file');
+  db_delete('file_managed')->condition('fid', $file->fid)->execute();
+  db_delete('file_usage')->condition('fid', $file->fid)->execute();
 
-    return TRUE;
-  }
-  return FALSE;
+  // Let other modules respond to file deletion.
+  module_invoke_all('file_delete', $file);
+  module_invoke_all('entity_delete', $file, 'file');
 }
 
 /**
@@ -1442,8 +1422,8 @@ function file_space_used($uid = NULL, $status = FILE_STATUS_PERMANENT) {
  * Saves a file upload to a new location.
  *
  * The file will be added to the {file_managed} table as a temporary file.
- * Temporary files are periodically cleaned. To make the file a permanent file,
- * assign the status and use file_save() to save the changes.
+ * Temporary files are periodically cleaned. Use file_usage_add() to register
+ * the usage of the file which will automatically mark it as permanent.
  *
  * @param $source
  *   A string specifying the filepath or URI of the uploaded file to save.

core/modules/file/file.field.inc

@@ -224,22 +224,6 @@ function file_field_prepare_view($entity_type, $entities, $field, $instances, $l
   }
 }
 
-/**
- * Implements hook_field_presave().
- */
-function file_field_presave($entity_type, $entity, $field, $instance, $langcode, &$items) {
-  // Make sure that each file which will be saved with this object has a
-  // permanent status, so that it will not be removed when temporary files are
-  // cleaned up.
-  foreach ($items as $item) {
-    $file = file_load($item['fid']);
-    if (!$file->status) {
-      $file->status = FILE_STATUS_PERMANENT;
-      file_save($file);
-    }
-  }
-}
-
 /**
  * Implements hook_field_insert().
  */
@@ -248,8 +232,7 @@ function file_field_insert($entity_type, $entity, $field, $instance, $langcode,
 
   // Add a new usage of each uploaded file.
   foreach ($items as $item) {
-    $file = (object) $item;
-    file_usage_add($file, 'file', $entity_type, $id);
+    file_usage_add(file_load($item['fid']), 'file', $entity_type, $id);
   }
 }
 
@@ -265,8 +248,7 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
   // deletion of previous file usages are necessary.
   if (!empty($entity->revision)) {
     foreach ($items as $item) {
-      $file = (object) $item;
-      file_usage_add($file, 'file', $entity_type, $id);
+      file_usage_add(file_load($item['fid']), 'file', $entity_type, $id);
     }
     return;
   }
@@ -287,8 +269,8 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
     foreach ($original->{$field['field_name']}[$langcode] as $original_item) {
       $original_fids[] = $original_item['fid'];
       if (isset($original_item['fid']) && !in_array($original_item['fid'], $current_fids)) {
-        // Decrement the file usage count by 1 and delete the file if possible.
-        file_field_delete_file($original_item, $field, $entity_type, $id);
+        // Decrement the file usage count by 1.
+        file_usage_delete(file_load($original_item['fid']), 'file', $entity_type, $id);
       }
     }
   }
@@ -296,8 +278,7 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
   // Add new usage entries for newly added files.
   foreach ($items as $item) {
     if (!in_array($item['fid'], $original_fids)) {
-      $file = (object) $item;
-      file_usage_add($file, 'file', $entity_type, $id);
+      file_usage_add(file_load($item['fid']), 'file', $entity_type, $id);
     }
   }
 }
@@ -310,7 +291,7 @@ function file_field_delete($entity_type, $entity, $field, $instance, $langcode,
 
   // Delete all file usages within this entity.
   foreach ($items as $delta => $item) {
-    file_field_delete_file($item, $field, $entity_type, $id, 0);
+    file_usage_delete(file_load($item['fid']), 'file', $entity_type, $id, 0);
   }
 }
 
@@ -320,52 +301,11 @@ function file_field_delete($entity_type, $entity, $field, $instance, $langcode,
 function file_field_delete_revision($entity_type, $entity, $field, $instance, $langcode, &$items) {
   list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);
   foreach ($items as $delta => $item) {
-    // Decrement the file usage count by 1 and delete the file if possible.
-    if (file_field_delete_file($item, $field, $entity_type, $id)) {
-      $items[$delta] = NULL;
-    }
+    // Decrement the file usage count by 1.
+    file_usage_delete(file_load($item['fid']), 'file', $entity_type, $id);
   }
 }
 
-/**
- * Decrements the usage count for a file and attempts to delete it.
- *
- * This function only has an effect if the file being deleted is used only by
- * File module.
- *
- * @param $item
- *   The field item that contains a file array.
- * @param $field
- *   The field structure for the operation.
- * @param $entity_type
- *   The type of $entity.
- * @param $id
- *   The entity ID which contains the file being deleted.
- * @param $count
- *   (optional) The number of references to decrement from the object
- *   containing the file. Defaults to 1.
- *
- * @return
- *   Boolean TRUE if the file was deleted, or an array of remaining references
- *   if the file is still in use by other modules. Boolean FALSE if an error
- *   was encountered.
- */
-function file_field_delete_file($item, $field, $entity_type, $id, $count = 1) {
-  // To prevent the file field from deleting files it doesn't know about, check
-  // the file reference count. Temporary files can be deleted because they
-  // are not yet associated with any content at all.
-  $file = (object) $item;
-  $file_usage = file_usage_list($file);
-  if ($file->status == 0 || !empty($file_usage['file'])) {
-    file_usage_delete($file, 'file', $entity_type, $id, $count);
-    return file_delete($file);
-  }
-
-  // Even if the file is not deleted, return TRUE to indicate the file field
-  // record can be removed from the field database tables.
-  return TRUE;
-}
-
 /**
  * Implements hook_field_is_empty().
  */

core/modules/file/file.module

@@ -598,7 +598,8 @@ function file_managed_file_submit($form, &$form_state) {
   // only remove a file in response to its remove button being clicked.
   if ($button_key == 'remove_button') {
     // If it's a temporary file we can safely remove it immediately, otherwise
-    // it's up to the implementing module to clean up files that are in use.
+    // it's up to the implementing module to remove usages of files to have them
+    // removed.
     if ($element['#file'] && $element['#file']->status == 0) {
       file_delete($element['#file']);
     }

core/modules/file/tests/file.test

@@ -728,11 +728,31 @@ class FileFieldRevisionTestCase extends FileFieldTestCase {
     // not be necessary here. The file really is deleted, but stream wrappers
     // doesn't seem to think so unless we clear the PHP file stat() cache.
     clearstatcache();
+
+    // Call system_cron() to clean up the file. Make sure the timestamp
+    // of the file is older than DRUPAL_MAXIMUM_TEMP_FILE_AGE.
+    db_update('file_managed')
+      ->fields(array(
+        'timestamp' => REQUEST_TIME - (DRUPAL_MAXIMUM_TEMP_FILE_AGE + 1),
+      ))
+      ->condition('fid', $node_file_r3->fid)
+      ->execute();
+    drupal_cron_run();
+
     $this->assertFileNotExists($node_file_r3, t('Second file is now deleted after deleting third revision, since it is no longer being used by any other nodes.'));
     $this->assertFileEntryNotExists($node_file_r3, t('Second file entry is now deleted after deleting third revision, since it is no longer being used by any other nodes.'));
 
     // Delete the entire node and check that the original file is deleted.
     $this->drupalPost('node/' . $nid . '/delete', array(), t('Delete'));
+    // Call system_cron() to clean up the file. Make sure the timestamp
+    // of the file is older than DRUPAL_MAXIMUM_TEMP_FILE_AGE.
+    db_update('file_managed')
+      ->fields(array(
+        'timestamp' => REQUEST_TIME - (DRUPAL_MAXIMUM_TEMP_FILE_AGE + 1),
+      ))
+      ->condition('fid', $node_file_r1->fid)
+      ->execute();
+    drupal_cron_run();
     $this->assertFileNotExists($node_file_r1, t('Original file is deleted after deleting the entire node with two revisions remaining.'));
     $this->assertFileEntryNotExists($node_file_r1, t('Original file entry is deleted after deleting the entire node with two revisions remaining.'));
   }

core/modules/image/image.field.inc

@@ -219,7 +219,6 @@ function image_field_prepare_view($entity_type, $entities, $field, $instances, $
  * Implements hook_field_presave().
  */
 function image_field_presave($entity_type, $entity, $field, $instance, $langcode, &$items) {
-  file_field_presave($entity_type, $entity, $field, $instance, $langcode, $items);
 
   // Determine the dimensions if necessary.
   foreach ($items as &$item) {

core/modules/system/system.module

@@ -3075,7 +3075,10 @@ function system_cron() {
     if ($file = file_load($row->fid)) {
       $references = file_usage_list($file);
       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);
         }
       }

core/modules/system/tests/file.test

@@ -1564,7 +1564,7 @@ class FileDeleteTest extends FileHookTestCase {
 
     // Check that deletion removes the file and database record.
     $this->assertTrue(is_file($file->uri), t('File exists.'));
-    $this->assertIdentical(file_delete($file), TRUE, t('Delete worked.'));
+    file_delete($file);
     $this->assertFileHooksCalled(array('delete'));
     $this->assertFalse(file_exists($file->uri), t('Test file has actually been deleted.'));
     $this->assertFalse(file_load($file->fid), t('File was removed from the database.'));
@@ -1579,7 +1579,6 @@ class FileDeleteTest extends FileHookTestCase {
     file_usage_add($file, 'testing', 'test', 1);
 
     file_usage_delete($file, 'testing', 'test', 1);
-    file_delete($file);
     $usage = file_usage_list($file);
     $this->assertEqual($usage['testing']['test'], array(1 => 1), t('Test file is still in use.'));
     $this->assertTrue(file_exists($file->uri), t('File still exists on the disk.'));
@@ -1589,10 +1588,27 @@ class FileDeleteTest extends FileHookTestCase {
     file_test_reset();
 
     file_usage_delete($file, 'testing', 'test', 1);
-    file_delete($file);
     $usage = file_usage_list($file);
-    $this->assertFileHooksCalled(array('delete'));
+    $this->assertFileHooksCalled(array('load', 'update'));
     $this->assertTrue(empty($usage), t('File usage data was removed.'));
+    $this->assertTrue(file_exists($file->uri), 'File still exists on the disk.');
+    $file = file_load($file->fid);
+    $this->assertTrue($file, 'File still exists in the database.');
+    $this->assertEqual($file->status, 0, 'File is temporary.');
+    file_test_reset();
+
+    // Call system_cron() to clean up the file. Make sure the timestamp
+    // of the file is older than DRUPAL_MAXIMUM_TEMP_FILE_AGE.
+    db_update('file_managed')
+      ->fields(array(
+        'timestamp' => REQUEST_TIME - (DRUPAL_MAXIMUM_TEMP_FILE_AGE + 1),
+      ))
+      ->condition('fid', $file->fid)
+      ->execute();
+    drupal_cron_run();
+
+    // system_cron() loads
+    $this->assertFileHooksCalled(array('load', 'delete'));
     $this->assertFalse(file_exists($file->uri), t('File has been deleted after its last usage was removed.'));
     $this->assertFalse(file_load($file->fid), t('File was removed from the database.'));
   }