Drop hook_file_references() in favor of a table based approach http://drupal.org/node/353458 From: andrew morton --- includes/file.inc | 127 ++++++++++++++++++++++++----- modules/node/node.api.php | 2 modules/simpletest/tests/file.test | 121 ++++++++++++++++++++++++++-- modules/simpletest/tests/file_test.module | 18 +--- modules/system/system.api.php | 29 +------ modules/system/system.install | 97 ++++++++++++++++++++++ modules/system/system.module | 3 - modules/upload/upload.install | 9 ++ modules/upload/upload.module | 21 +---- modules/user/user.module | 24 +---- 10 files changed, 349 insertions(+), 102 deletions(-) mode change 100755 => 100644 includes/file.inc mode change 100755 => 100644 modules/simpletest/tests/file.test mode change 100755 => 100644 modules/simpletest/tests/file_test.module mode change 100755 => 100644 modules/system/system.api.php mode change 100755 => 100644 modules/system/system.install mode change 100755 => 100644 modules/upload/upload.install mode change 100755 => 100644 modules/upload/upload.module mode change 100755 => 100644 modules/user/user.module diff --git includes/file.inc includes/file.inc old mode 100755 new mode 100644 index 670cb25..8141b3b --- includes/file.inc +++ includes/file.inc @@ -349,6 +349,104 @@ function file_save($file) { } /** + * Determine where a file is used. + * + * @param $file + * A file object. + * @return + * An nested array with usage data. The first level is keyed by module + * name, the second by table name, the third has 'id' and 'count' keys. + * + * @see file_add_usage() + * @see file_remove_usage() + */ +function file_get_usage(stdClass $file) { + $result = db_query('SELECT f.module, f.type, f.id, f.count FROM {file_usage} f WHERE f.fid = :fid AND count > 0', array(':fid' => $file->fid)); + $references = array(); + + foreach ($result as $usage) { + $references[$usage->module][$usage->type] = array('id' => $usage->id, 'count' => $usage->count); + } + return $references; +} + +/** + * Inform Drupal that a module is using a file. + * + * This usage information will be queried during file_delete(). + * + * Examples: + * - The upload module that associates files with node revisions so the + * @p $type would be 'node_revision' and @p $id would be the node's vid. + * - The user module associates an image with a user so the @p $type would be + * 'user' and the $id would be the user's id. + * + * @param $file + * A file object. + * @param $module + * The name of the module using the file. + * @param $type + * The name of the table where the file is referenced. + * @param $id + * The id of the row in the table. + * + * @see file_get_usage() + * @see file_remove_usage() + */ +function file_add_usage(stdClass $file, $module, $type, $id) { + db_merge('file_usage') + ->key(array( + 'fid' => $file->fid, + 'module' => $module, + 'type' => $type, + 'id' => $id, + )) + ->fields(array('count' => 1)) + ->expression('count', 'count + 1') + ->execute(); +} + +/** + * Inform Drupal that a module is no longer using a file. + * + * If no usages of the file remain in the database, the file itself will be + * deleted using file_delete(). + * + * @param $file + * A file object. + * @param $module + * The name of the module using the file. + * @param $type + * The name of the table where the file is referenced. + * @param $id + * The id of the row in the table. + * @return + * TRUE for success, or FALSE if no usages remain but the file still could + * not be deleted. + * + * @see file_add_usage() + * @see file_get_usage() + * @see file_delete() + */ +function file_remove_usage(stdClass $file, $module, $type, $id) { + db_update('file_usage')->expression('count', 'count - 1') + ->condition('fid', $file->fid) + ->condition('module', $module) + ->condition('type', $type) + ->condition('id', $id) + ->execute(); + + // If file is still in use and $force is FALSE, the file will not be deleted + // from Drupal, but file_delete will return a populated array that tests as + // TRUE. + $references = db_result(db_query('SELECT COUNT(*) FROM {file_usage} f WHERE f.fid = :fid AND count > 0', array(':fid' => $file->fid))); + if (empty($references)) { + return file_delete($file); + } + return TRUE; +} + +/** * Copy a file to a new location and adds a file record to the database. * * This function should be used when manipulating files that have records @@ -589,6 +687,9 @@ function file_move($source, $destination = NULL, $replace = FILE_EXISTS_RENAME) // Inform modules that the file has been moved. module_invoke_all('file_move', $file, $source); + ///TODO: move fids in {file_usage} if the fid changed... probably needs to + /// be done by the various hook_file_move() implementations (e.g. core + /// can't update filefield's tables - or can we?) if ($delete_source) { // Try a soft delete to remove original if it's not in use elsewhere. @@ -728,34 +829,19 @@ function file_create_filename($basename, $directory) { /** * Delete a file and its database record. * - * If the $force parameter is not TRUE hook_file_references() will be called - * to determine if the file is being used by any modules. If the file is being - * used is the delete will be canceled. - * * @param $file * A file object. - * @param $force - * Boolean indicating that the file should be deleted even if - * hook_file_references() reports that the file is in use. - * @return mixed - * TRUE for success, FALSE in the event of an error, or an array if the file - * is being used by another module. The array keys are the module's name and - * the values are the number of references. + * @return + * TRUE for success, or FALSE in the event of an error. * * @see file_unmanaged_delete() - * @see hook_file_references() + * @see file_get_usage() + * @see file_remove_usage() * @see hook_file_delete() */ -function file_delete($file, $force = FALSE) { +function file_delete($file) { $file = (object)$file; - // If any module returns a value from the reference hook, the file will not - // be deleted from Drupal, but file_delete will return a populated array that - // tests as TRUE. - if (!$force && ($references = module_invoke_all('file_references', $file))) { - return $references; - } - // Let other modules clean up any references to the deleted file. module_invoke_all('file_delete', $file); @@ -763,6 +849,7 @@ function file_delete($file, $force = FALSE) { // database, so UIs can still find the file in the database. if (file_unmanaged_delete($file->filepath)) { db_delete('files')->condition('fid', $file->fid)->execute(); + db_delete('file_usage')->condition('fid', $file->fid)->execute(); return TRUE; } return FALSE; diff --git modules/node/node.api.php modules/node/node.api.php index 8f35233..df4c865 100755 --- modules/node/node.api.php +++ modules/node/node.api.php @@ -200,7 +200,7 @@ function hook_nodeapi_delete_revision($node) { return; } foreach ($node->files as $file) { - file_delete($file); + file_remove_usage($file); } } diff --git modules/simpletest/tests/file.test modules/simpletest/tests/file.test old mode 100755 new mode 100644 index 9a4a3ad..fe6a026 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -1225,18 +1225,44 @@ class FileDeleteTest extends FileHookTestCase { /** * Try deleting a normal file (as opposed to a directory, symlink, etc). */ - function testNormal() { + function testUnused() { $file = $this->createFile(); // Check that deletion removes the file and database record. $this->assertTrue(is_file($file->filepath), t("File exists.")); $this->assertIdentical(file_delete($file), TRUE, t("Delete worked.")); - $this->assertFileHooksCalled(array('references', 'delete')); + $this->assertFileHooksCalled(array('delete')); $this->assertFalse(file_exists($file->filepath), t("Test file has actually been deleted.")); $this->assertFalse(file_load($file->fid), t('File was removed from the database.')); + $this->assertFalse(file_get_usage($file), t('File usage data was removed.')); + } + + /** + * Try deleting a file that is in use. + */ + function testInUse() { + $file = $this->createFile(); + file_add_usage($file, 'testing', 'test', 1); + file_add_usage($file, 'testing', 'test', 1); + + file_remove_usage($file, 'testing', 'test', 1); + $usage = file_get_usage(); + $this->assertEqual($usage['testing']['test'], array('id' => 1, 'count' => 1), t("Delete failed, because it is known to be used somewhere still.")); + $this->assertFileHooksCalled(array()); - // TODO: implement hook_file_references() in file_test.module and report a - // file in use and test the $force parameter. + $this->assertTrue(file_exists($file->filepath), t("Test file still exists on the disk.")); + $this->assertTrue(file_load($file->fid), t('File still exists in the database.')); + $this->assertTrue(file_get_usage($file), t('File usage data still exists.')); + // Clear out the call to hook_file_load(). + file_test_reset(); + + file_remove_usage($file); + $usage = file_get_usage(); + $this->assert(empty($usage), t("Delete worked.")); + $this->assertFileHooksCalled(array('delete')); + $this->assertFalse(file_exists($file->filepath), t("Test file has actually been deleted.")); + $this->assertFalse(file_load($file->fid), t('File was removed from the database.')); + $this->assertFalse(file_get_usage($file), t('File usage data was removed.')); } } @@ -1338,7 +1364,7 @@ class FileMoveTest extends FileHookTestCase { $this->assertTrue($result, t('File moved sucessfully.')); // Check that the correct hooks were called. - $this->assertFileHooksCalled(array('move', 'update', 'delete', 'references', 'load')); + $this->assertFileHooksCalled(array('move', 'update', 'delete', 'load')); // Reload the file from the database and check that the changes were // actually saved. @@ -1700,6 +1726,91 @@ class FileSaveTest extends FileHookTestCase { } } +/** + * Tests the file_get_usage(), file_add_usage() and file_remove_usage() + * functions. + */ +class FileUsageTest extends FileTestCase { + function getInfo() { + return array( + 'name' => t('File usage'), + 'description' => t('Tests the file usage functions.'), + 'group' => t('File'), + ); + } + + /** + * Test the file_get_usage() function. + */ + function testGetUsage() { + $file = $this->createFile(); + db_insert('file_usage')->fields(array( + 'fid' => $file->fid, + 'module' => 'testing', + 'type' => 'foo', + 'id' => 1, + 'count' => 1 + ))->execute(); + db_insert('file_usage')->fields(array( + 'fid' => $file->fid, + 'module' => 'testing', + 'type' => 'bar', + 'id' => 2, + 'count' => 2 + ))->execute(); + + $usage = file_get_usage($file); + + $this->assertEqual(count($usage['testing']), 2, t('Returned the correct number of items.')); + $this->assertEqual($usage['testing']['foo']['id'], 1, t('Returned the correct id.')); + $this->assertEqual($usage['testing']['bar']['id'], 2, t('Returned the correct id.')); + $this->assertEqual($usage['testing']['foo']['count'], 1, t('Returned the correct count.')); + $this->assertEqual($usage['testing']['bar']['count'], 2, t('Returned the correct count.')); + } + + /** + * Test the file_add_usage() function. + */ + function testAddUsage() { + $file = $this->createFile(); + file_add_usage($file, 'testing', 'foo', 1); + // Add the file twice to ensure that the count is incremented rather than + // creating additional records. + file_add_usage($file, 'testing', 'bar', 2); + file_add_usage($file, 'testing', 'bar', 2); + + $usage = db_select('file_usage', 'f')->fields('f')->condition('f.fid', $file->fid)->execute()->fetchAllAssoc('id'); + $this->assertEqual(count($usage), 2, t('Created two records')); + $this->assertEqual($usage[1]->module, 'testing', t('Correct module')); + $this->assertEqual($usage[2]->module, 'testing', t('Correct module')); + $this->assertEqual($usage[1]->type, 'foo', t('Correct type')); + $this->assertEqual($usage[2]->type, 'bar', t('Correct type')); + $this->assertEqual($usage[1]->count, 1, t('Correct count')); + $this->assertEqual($usage[2]->count, 2, t('Correct count')); + } + + /** + * Test the file_remove_usage() function. + */ + function testRemoveUsage() { + $file = $this->createFile(); + db_insert('file_usage')->fields(array( + 'fid' => $file->fid, + 'module' => 'testing', + 'type' => 'bar', + 'id' => 2, + 'count' => 2 + ))->execute(); + + file_remove_usage($file, 'testing', 'bar', 2); + $count = db_select('file_usage', 'f')->fields('f', array('count'))->condition('f.fid', $file->fid)->execute()->fetchField(); + $this->assertEqual(1, $count, t('The count was decremented correctly.')); + + file_remove_usage($file, 'testing', 'bar', 2); + $count = db_select('file_usage', 'f')->fields('f', array('count'))->condition('f.fid', $file->fid)->execute()->fetchField(); + $this->assertEqual(0, $count, t('The count was decremented correctly.')); + } +} /** * Tests the file_validate() function.. diff --git modules/simpletest/tests/file_test.module modules/simpletest/tests/file_test.module old mode 100755 new mode 100644 index c72610e..9f2d386 --- modules/simpletest/tests/file_test.module +++ modules/simpletest/tests/file_test.module @@ -78,7 +78,6 @@ function file_test_reset() { 'load' => array(), 'validate' => array(), 'download' => array(), - 'references' => array(), 'insert' => array(), 'update' => array(), 'copy' => array(), @@ -91,7 +90,6 @@ function file_test_reset() { $return = array( 'validate' => array(), 'download' => NULL, - 'references' => NULL, ); variable_set('file_test_return', $return); } @@ -102,7 +100,7 @@ function file_test_reset() { * * @param $op * One of the hook_file_* operations: 'load', 'validate', 'download', - * 'references', 'insert', 'update', 'copy', 'move', 'delete'. + * 'insert', 'update', 'copy', 'move', 'delete'. * @returns * Array of the parameters passed to each call. * @see _file_test_log_call() and file_test_reset() @@ -129,7 +127,7 @@ function file_test_get_all_calls() { * * @param $op * One of the hook_file_* operations: 'load', 'validate', 'download', - * 'references', 'insert', 'update', 'copy', 'move', 'delete'. + * 'insert', 'update', 'copy', 'move', 'delete'. * @param $args * Values passed to hook. * @see file_test_get_calls() and file_test_reset() @@ -144,7 +142,7 @@ function _file_test_log_call($op, $args) { * Load the appropriate return value. * * @param $op - * One of the hook_file_[validate,download,references] operations. + * One of the hook_file_[validate,download] operations. * @return * Value set by file_test_set_return(). * @see file_test_set_return() and file_test_reset(). @@ -158,7 +156,7 @@ function _file_test_get_return($op) { * Assign a return value for a given operation. * * @param $op - * One of the hook_file_[validate,download,references] operations. + * One of the hook_file_[validate,download] operations. * @param $value * Value for the hook to return. * @see _file_test_get_return() and file_test_reset(). @@ -198,14 +196,6 @@ function file_test_file_download($file) { } /** - * Implementation of hook_file_references(). - */ -function file_test_file_references($file) { - _file_test_log_call('references', array($file)); - return _file_test_get_return('references'); -} - -/** * Implementation of hook_file_insert(). */ function file_test_file_insert($file) { diff --git modules/system/system.api.php modules/system/system.api.php old mode 100755 new mode 100644 index 2237b22..789481d --- modules/system/system.api.php +++ modules/system/system.api.php @@ -1141,6 +1141,10 @@ function hook_file_copy($file, $source) { /** * Respond to a file that has been moved. * + * $file->fid stays the same except if the moved file replaced a previously + * existing one, in which case the fid and other properties of the existing + * target file is adopted for the new file object. + * * @param $file * The updated file object after the move. * @param $source @@ -1155,31 +1159,6 @@ function hook_file_move($file, $source) { } /** - * Report the number of times a file is referenced by a module. - * - * This hook is called to determine if a files is in use. Multiple modules may - * be referencing the same file and to prevent one from deleting a file used by - * another this hook is called. - * - * @param $file - * The file object being checked for references. - * @return - * If the module uses this file return an array with the module name as the - * key and the value the number of times the file is used. - * - * @see file_delete() - * @see upload_file_references() - */ -function hook_file_references($file) { - // If upload.module is still using a file, do not let other modules delete it. - $count = db_query('SELECT COUNT(*) FROM {upload} WHERE fid = :fid', array(':fid' => $file->fid))->fetchField(); - if ($count) { - // Return the name of the module and how many references it has to the file. - return array('upload' => $count); - } -} - -/** * Respond to a file being deleted. * * @param $file diff --git modules/system/system.install modules/system/system.install old mode 100755 new mode 100644 index c0a6713..8590d47 --- modules/system/system.install +++ modules/system/system.install @@ -664,6 +664,50 @@ function system_schema() { 'primary key' => array('fid'), ); + $schema['file_usage'] = array( + 'description' => 'Track where a file is used.', + 'fields' => array( + 'fid' => array( + 'description' => 'File ID.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + ), + 'module' => array( + 'description' => 'The name of the module that is using the file.', + 'type' => 'varchar', + 'length' => 255, + 'not null' => TRUE, + 'default' => '', + ), + 'type' => array( + 'description' => 'The name of the table where the file is used.', + 'type' => 'varchar', + 'length' => 64, + 'not null' => TRUE, + 'default' => '', + ), + 'id' => array( + 'description' => 'The primary key of the object using the file.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + 'default' => 0, + ), + 'count' => array( + 'description' => 'The number of times this file is used by this object.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + 'default' => 0, + ), + ), + 'primary key' => array('fid', 'type', 'id', 'module'), + 'indexes' => array( + 'type_id' => array('type', 'id'), + ), + ); + $schema['flood'] = array( 'description' => 'Flood controls the threshold of events, such as the number of contact attempts.', 'fields' => array( @@ -3237,6 +3281,59 @@ function system_update_7020() { } /** + * Create the file_usage table. + */ +function system_update_7021() { + $schema['file_usage'] = array( + 'description' => 'Track where a file is used.', + 'fields' => array( + 'fid' => array( + 'description' => 'File ID.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + ), + 'module' => array( + 'description' => 'The name of the module that is using the file.', + 'type' => 'varchar', + 'length' => 255, + 'not null' => TRUE, + 'default' => '', + ), + 'type' => array( + 'description' => 'The name of the table where the file is used.', + 'type' => 'varchar', + 'length' => 64, + 'not null' => TRUE, + 'default' => '', + ), + 'id' => array( + 'description' => 'The primary key of the object using the file.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + 'default' => 0, + ), + 'count' => array( + 'description' => 'The number of times this file is used by this object.', + 'type' => 'int', + 'unsigned' => TRUE, + 'not null' => TRUE, + 'default' => 0, + ), + ), + 'primary key' => array('fid', 'type', 'id', 'module'), + 'indexes' => array( + 'type_id' => array('type', 'id'), + ), + ); + + $ret = array(); + db_create_table($ret, 'file_usage', $schema['file_usage']); + return $ret; +} + +/** * @} End of "defgroup updates-6.x-to-7.x" * The next series of updates should start at 8000. */ diff --git modules/system/system.module modules/system/system.module index c112d03..3319baa 100755 --- modules/system/system.module +++ modules/system/system.module @@ -1560,7 +1560,8 @@ function system_cron() { $result = db_query('SELECT fid FROM {files} WHERE status & :permanent1 != :permanent2 AND timestamp < :timestamp', array(':permanent1' => FILE_STATUS_PERMANENT, ':permanent2' => FILE_STATUS_PERMANENT, ':timestamp' => REQUEST_TIME - DRUPAL_MAXIMUM_TEMP_FILE_AGE)); foreach ($result as $row) { if ($file = file_load($row->fid)) { - if (!file_delete($file)) { + $references = file_get_usage($file); + if (empty($references)) { watchdog('file system', 'Could not delete temporary file "%path" during garbage collection', array('%path' => $file->filepath), WATCHDOG_ERROR); } } diff --git modules/upload/upload.install modules/upload/upload.install old mode 100755 new mode 100644 index 0d825c8..5899b02 --- modules/upload/upload.install +++ modules/upload/upload.install @@ -82,4 +82,11 @@ function upload_schema() { return $schema; } - +/** + * Create file_usage records for our files. + */ +function upload_update_7000() { + $ret = array(); + $ret[] = update_sql("INSERT INTO {file_usage} (fid, module, type, id, count) SELECT fid, 'upload', 'node_revision', vid, 1 FROM {upload}"); + return $ret; +} diff --git modules/upload/upload.module modules/upload/upload.module old mode 100755 new mode 100644 index d487f6b..a26b9cb --- modules/upload/upload.module +++ modules/upload/upload.module @@ -280,18 +280,6 @@ function upload_file_load($files) { } /** - * Implementation of hook_file_references(). - */ -function upload_file_references($file) { - // If upload.module is still using a file, do not let other modules delete it. - $count = db_query('SELECT COUNT(*) FROM {upload} WHERE fid = :fid', array(':fid' => $file->fid))->fetchField(); - if ($count) { - // Return the name of the module and how many references it has to the file. - return array('upload' => $count); - } -} - -/** * Implementation of hook_file_delete(). */ function upload_file_delete($file) { @@ -384,7 +372,7 @@ function upload_nodeapi_delete($node) { return; } foreach($node->files as $file) { - file_delete($file); + file_remove_usage($file); } } @@ -397,7 +385,7 @@ function upload_nodeapi_delete_revision($node) { return; } foreach ($node->files as $file) { - file_delete($file); + file_remove_usage($file); } } @@ -494,8 +482,7 @@ function upload_save(&$node) { if (!empty($file->remove)) { // Remove the reference from this revision. db_delete('upload')->condition('fid', $file->fid)->condition('vid', $node->vid)->execute(); - // Try a soft delete, if the file isn't used elsewhere it'll be deleted. - file_delete($file); + file_remove_usage($file, 'upload', 'node_revision', $node->vid); // Remove it from the session in the case of new uploads, // that you want to disassociate before node submission. unset($node->files[$fid]); @@ -530,6 +517,8 @@ function upload_save(&$node) { } $file->status |= FILE_STATUS_PERMANENT; $file = file_save($file); + + file_add_usage($file, 'upload', 'node_revision', $node->vid); } } diff --git modules/user/user.module modules/user/user.module old mode 100755 new mode 100644 index 63cdcf7..0245ceb --- modules/user/user.module +++ modules/user/user.module @@ -317,9 +317,14 @@ function user_save($account, $edit = array(), $category = 'account') { if ($picture = file_move($picture, $destination, FILE_EXISTS_REPLACE)) { $picture->status |= FILE_STATUS_PERMANENT; $edit['picture'] = file_save($picture); + file_add_usage($picture, 'user', 'user', $account->uid); } } } + // If the picture existed before and was unset, remove our reference to it. + elseif (!empty($account->picture->fid)) { + file_remove_usage($account->picture, 'user', 'user', $account->uid); + } $edit['picture'] = empty($edit['picture']->fid) ? 0 : $edit['picture']->fid; $edit['data'] = $data; @@ -337,13 +342,6 @@ function user_save($account, $edit = array(), $category = 'account') { // return FALSE; } - // If the picture changed or was unset, remove the old one. This step needs - // to occur after updating the {users} record so that user_file_references() - // doesn't report it in use and block the deletion. - if (!empty($account->picture->fid) && ($edit['picture'] != $account->picture->fid)) { - file_delete($account->picture); - } - // Reload user roles if provided. if (isset($edit['roles']) && is_array($edit['roles'])) { db_query('DELETE FROM {users_roles} WHERE uid = %d', $account->uid); @@ -694,18 +692,6 @@ function user_file_download($filepath) { } /** - * Implementation of hook_file_references(). - */ -function user_file_references($file) { - // Determine if the file is used by this module. - $count = db_query('SELECT COUNT(*) FROM {users} WHERE picture = :fid', array(':fid' => $file->fid))->fetchField(); - if ($count) { - // Return the name of the module and how many references it has to the file. - return array('user' => $count); - } -} - -/** * Implementation of hook_file_delete(). */ function user_file_delete($file) {