diff --git a/core/includes/file.inc b/core/includes/file.inc index 7559608..0cf51de 100644 --- a/core/includes/file.inc +++ b/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. diff --git a/core/modules/file/file.field.inc b/core/modules/file/file.field.inc index a1a2ef9..e6b2f4d 100644 --- a/core/modules/file/file.field.inc +++ b/core/modules/file/file.field.inc @@ -220,22 +220,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(). */ function file_field_insert($entity_type, $entity, $field, $instance, $langcode, &$items) { @@ -243,8 +227,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); } } @@ -260,8 +243,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; } @@ -282,8 +264,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); } } } @@ -291,8 +273,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); } } } @@ -305,7 +286,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); } } @@ -315,53 +296,12 @@ 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(). */ function file_field_is_empty($item, $field) { diff --git a/core/modules/file/file.module b/core/modules/file/file.module index f31fdfa..9251186 100644 --- a/core/modules/file/file.module +++ b/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']); } diff --git a/core/modules/file/tests/file.test b/core/modules/file/tests/file.test index febd1da..a826c14 100644 --- a/core/modules/file/tests/file.test +++ b/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.')); } diff --git a/core/modules/image/image.field.inc b/core/modules/image/image.field.inc index 0fb657c..2a32c15 100644 --- a/core/modules/image/image.field.inc +++ b/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) { diff --git a/core/modules/system/system.module b/core/modules/system/system.module index e685c16..ebc5dcf 100644 --- a/core/modules/system/system.module +++ b/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); } } diff --git a/core/modules/system/tests/file.test b/core/modules/system/tests/file.test index 78455e4..f679f98 100644 --- a/core/modules/system/tests/file.test +++ b/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.')); }