Drop hook_file_references() in favor of a table based approach http://drupal.org/node/353458 From: andrew morton --- includes/file.inc | 111 +++++++++++++++++++++++++--- modules/simpletest/tests/file.test | 117 ++++++++++++++++++++++++++++- modules/simpletest/tests/file_test.module | 18 +--- modules/system/system.api.php | 25 ------ modules/system/system.install | 97 ++++++++++++++++++++++++ modules/upload/upload.install | 9 ++ modules/upload/upload.module | 15 +--- modules/user/user.module | 2 8 files changed, 325 insertions(+), 69 deletions(-) diff --git includes/file.inc includes/file.inc index 4e61e8e..1800fa4 100644 --- includes/file.inc +++ includes/file.inc @@ -349,6 +349,88 @@ function file_save($file) { } /** + * Determine where a file is used. + * + * @param $file + * A file object. + * @return + * An nested array of 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)); + $return = array(); + foreach ($result as $usage) { + $return[$usage->module][$usage->type] = array('id' => $usage->id, 'count' => $usage->count); + } + + return $return; +} + +/** + * 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 + * $type would be 'node_revision' and $id would be the node's vid. + * - The user module associates an image with a user so the $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. + * + * @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_add_usage() + * @see file_get_usage() + */ +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(); +} + +/** * 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 @@ -728,9 +810,9 @@ 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. + * If the $force parameter is FALSE then file_get_usage() will be called to + * determine if the file is being used by any modules. If the file is in use + * the file will not be deleted and the usage data will be returned. * * @param $file * A file object. @@ -738,22 +820,26 @@ function file_create_filename($basename, $directory) { * 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. + * - TRUE for success. + * - FALSE in the event of an error. + * - An array of usage data from file_get_usage() when $force is FALSE and + * the file is in use. * * @see file_unmanaged_delete() - * @see hook_file_references() + * @see file_get_usage() * @see hook_file_delete() */ function file_delete($file, $force = FALSE) { $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; + // 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. + if (!$force) { + $references = file_get_usage($file); + if (count($references)) { + return $references; + } } // Let other modules clean up any references to the deleted 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/simpletest/tests/file.test modules/simpletest/tests/file.test index 988f045..714d1ef 100644 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -1225,18 +1225,40 @@ 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); + + $usage = file_delete($file, FALSE); + $this->assertEqual($usage['testing']['test'], array('id' => 1, 'count' => 1), t("Delete failed and returned usage data.")); + $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(); + + $this->assertIdentical(file_delete($file, TRUE), TRUE, 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 +1360,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 +1722,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 index f5c1230..09afbfa 100644 --- 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 index fd6d5d4..3dfae08 100644 --- modules/system/system.api.php +++ modules/system/system.api.php @@ -1155,31 +1155,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 index aaf8ed6..c6d7093 100644 --- 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/upload/upload.install modules/upload/upload.install index 5e04267..6572820 100644 --- 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 index 8d296a2..fbbd1e8 100644 --- 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) { @@ -494,6 +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(); + file_remove_usage($file, 'upload', 'node_revision', $node->vid); // Try a soft delete, if the file isn't used elsewhere it'll be deleted. file_delete($file); // Remove it from the session in the case of new uploads, @@ -530,6 +519,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 index 23f208f..c15b8ca 100644 --- modules/user/user.module +++ modules/user/user.module @@ -317,6 +317,7 @@ 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($file, 'user', 'user', $account->uid); } } } @@ -341,6 +342,7 @@ function user_save($account, $edit = array(), $category = 'account') { // 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_remove_usage($file, 'user', 'user', $account->uid); file_delete($account->picture); }