From d813e3679c57da312cc0de61da90ff2c2eff70a6 Mon Sep 17 00:00:00 2001
From: Dries Buytaert <dries@buytaert.net>
Date: Wed, 31 Dec 2008 11:08:47 +0000
Subject: [PATCH] - Patch #348201 by catch: make it possible to load multiple
files with fewer queries.
---
includes/file.inc | 77 +++++++++++----------
modules/simpletest/tests/file.test | 81 +++++++++++++++++------
modules/simpletest/tests/file_test.module | 12 ++--
modules/system/system.api.php | 25 +++----
modules/upload/upload.module | 55 +++++++++------
5 files changed, 157 insertions(+), 93 deletions(-)
diff --git a/includes/file.inc b/includes/file.inc
index 6a0ef2334a2e..360b66fc4869 100644
--- a/includes/file.inc
+++ b/includes/file.inc
@@ -267,55 +267,60 @@ function file_check_location($source, $directory = '') {
}
/**
- * Load a file object from the database.
+ * Load file objects from the database.
*
- * @param $param
- * Either the id of a file or an array of conditions to match against in the
- * database query.
- * @param $reset
- * Whether to reset the internal file_load cache.
+ * @param $fids
+ * An array of file IDs.
+ * @param $conditions
+ * An array of conditions to match against the {files} table. These
+ * should be supplied in the form array('field_name' => 'field_value').
* @return
- * A file object.
+ * An array of file objects, indexed by fid.
*
* @see hook_file_load()
+ * @see file_load()
*/
-function file_load($param, $reset = NULL) {
- static $files = array();
+function file_load_multiple($fids = array(), $conditions = array()) {
+ $query = db_select('files', 'f')->fields('f');
- if ($reset) {
- $files = array();
+ // If the $fids array is populated, add those to the query.
+ if ($fids) {
+ $query->condition('f.fid', $fids, 'IN');
}
- if (is_numeric($param)) {
- if (isset($files[(string) $param])) {
- return is_object($files[$param]) ? clone $files[$param] : $files[$param];
+ // If the conditions array is populated, add those to the query.
+ if ($conditions) {
+ foreach ($conditions as $field => $value) {
+ $query->condition('f.' . $field, $value);
}
- $result = db_query('SELECT f.* FROM {files} f WHERE f.fid = :fid', array(':fid' => $param));
}
- elseif (is_array($param)) {
- // Turn the conditions into a query.
- $cond = array();
- $arguments = array();
- foreach ($param as $key => $value) {
- $cond[] = 'f.' . db_escape_table($key) . " = '%s'";
- $arguments[] = $value;
+ $files = $query->execute()->fetchAllAssoc('fid');
+
+ // Invoke hook_file_load() on the terms loaded from the database
+ // and add them to the static cache.
+ if (!empty($files)) {
+ foreach (module_implements('file_load') as $module) {
+ $function = $module . '_file_load';
+ $function($files);
}
- $result = db_query('SELECT f.* FROM {files} f WHERE ' . implode(' AND ', $cond), $arguments);
- }
- else {
- return FALSE;
- }
- $file = $result->fetch(PDO::FETCH_OBJ);
-
- if ($file && $file->fid) {
- // Allow modules to add or change the file object.
- module_invoke_all('file_load', $file);
-
- // Cache the fully loaded value.
- $files[(string) $file->fid] = clone $file;
}
+ return $files;
+}
- return $file;
+/**
+ * Load a file object from the database.
+ *
+ * @param $fid
+ * A file ID.
+ * @return
+ * A file object.
+ *
+ * @see hook_file_load()
+ * @see file_load_multiple()
+ */
+function file_load($fid) {
+ $files = file_load_multiple(array($fid), array());
+ return reset($files);
}
/**
diff --git a/modules/simpletest/tests/file.test b/modules/simpletest/tests/file.test
index eadafcc83674..861cbdc1714e 100644
--- a/modules/simpletest/tests/file.test
+++ b/modules/simpletest/tests/file.test
@@ -338,7 +338,7 @@ class FileSaveUploadTest extends FileHookTestCase {
* Test the file_save_upload() function.
*/
function testFileSaveUpload() {
- $max_fid_before = db_result(db_query('SELECT MAX(fid) AS fid FROM {files}'));
+ $max_fid_before = db_query('SELECT MAX(fid) AS fid FROM {files}')->fetchField();
$upload_user = $this->drupalCreateUser(array('access content'));
$this->drupalLogin($upload_user);
@@ -354,7 +354,23 @@ class FileSaveUploadTest extends FileHookTestCase {
$max_fid_after = db_result(db_query('SELECT MAX(fid) AS fid FROM {files}'));
$this->assertTrue($max_fid_after > $max_fid_before, t('A new file was created.'));
- $this->assertTrue(file_load($max_fid_after), t('Loaded the file.'));
+ $file1 = file_load($max_fid_after);
+ $this->assertTrue($file1, t('Loaded the file.'));
+
+ // Upload a second file.
+ $max_fid_before = db_query('SELECT MAX(fid) AS fid FROM {files}')->fetchField();
+ $image2 = current($this->drupalGetTestFiles('image'));
+ $edit = array('files[file_test_upload]' => realpath($image2->filename));
+ $this->drupalPost('file-test/upload', $edit, t('Submit'));
+ $max_fid_after = db_query('SELECT MAX(fid) AS fid FROM {files}')->fetchField();
+
+ $file2 = file_load($max_fid_after);
+ $this->assertTrue($file2);
+
+ // Load both files using file_load_multiple().
+ $files = file_load_multiple(array($file1->fid, $file2->fid));
+ $this->assertTrue(isset($files[$file1->fid]), t('File was loaded successfully'));
+ $this->assertTrue(isset($files[$file2->fid]), t('File was loaded successfully'));
}
}
@@ -781,7 +797,7 @@ class FileDeleteTest extends FileHookTestCase {
$this->assertFileHookCalled('references');
$this->assertFileHookCalled('delete');
$this->assertFalse(file_exists($file->filepath), t("Test file has actually been deleted."));
- $this->assertFalse(file_load(array('filepath' => $file->filepath)), t("File was removed from the database."));
+ $this->assertFalse(file_load($file->fid), t('File was removed from the database.'));
// TODO: implement hook_file_references() in file_test.module and report a
// file in use and test the $force parameter.
@@ -814,7 +830,7 @@ class FileMoveTest extends FileHookTestCase {
$this->assertFileHookCalled('update');
$this->assertEqual($file->fid, $file->fid, t("File id $file->fid is unchanged after move."));
- $loaded_file = file_load($file->fid, TRUE);
+ $loaded_file = file_load($file->fid);
$this->assertTrue($loaded_file, t("File can be loaded from the database."));
$this->assertEqual($file->filename, $loaded_file->filename, t("File name was updated correctly in the database."));
$this->assertEqual($file->filepath, $loaded_file->filepath, t("File path was updated correctly in the database."));
@@ -852,7 +868,7 @@ class FileCopyTest extends FileHookTestCase {
$this->assertTrue(file_exists($file->filepath), t('The copied file exists.'));
// Check that the changes were actually saved to the database.
- $loaded_file = file_load($file->fid, TRUE);
+ $loaded_file = file_load($file->fid);
$this->assertTrue($loaded_file, t("File can be loaded from the database."));
$this->assertEqual($file->filename, $loaded_file->filename, t("File name was updated correctly in the database."));
$this->assertEqual($file->filepath, $loaded_file->filepath, t("File path was updated correctly in the database."));
@@ -884,7 +900,7 @@ class FileLoadTest extends FileHookTestCase {
* Try to load a non-existent file by filepath.
*/
function testLoadMissingFilepath() {
- $this->assertFalse(file_load(array('filepath' => 'misc/druplicon.png')), t("Try to load a file that doesn't exist in the database fails."));
+ $this->assertFalse(reset(file_load_multiple(array(), array('filepath' => 'misc/druplicon.png'))), t("Try to load a file that doesn't exist in the database fails."));
$this->assertFileHookCalled('load', 0);
}
@@ -892,14 +908,40 @@ class FileLoadTest extends FileHookTestCase {
* Try to load a non-existent file by status.
*/
function testLoadInvalidStatus() {
- $this->assertFalse(file_load(array('status' => -99)), t("Trying to load a file with an invalid status fails."));
+ $this->assertFalse(reset(file_load_multiple(array(), array('status' => -99))), t("Trying to load a file with an invalid status fails."));
$this->assertFileHookCalled('load', 0);
}
/**
- * This will test lading file data from the database.
+ * Load a single file and ensure that the correct values are returned.
+ */
+ function testSingleValues() {
+ // Create a new file object from scratch so we know the values.
+ $file = array(
+ 'uid' => 1,
+ 'filename' => 'druplicon.png',
+ 'filepath' => 'misc/druplicon.png',
+ 'filemime' => 'image/png',
+ 'timestamp' => 1,
+ 'status' => FILE_STATUS_PERMANENT,
+ );
+ $file = file_save($file);
+
+ $by_fid_file = file_load($file->fid);
+ $this->assertFileHookCalled('load', 1);
+ $this->assertTrue(is_object($by_fid_file), t('file_load() returned an object.'));
+ $this->assertEqual($by_fid_file->fid, $file->fid, t("Loading by fid got the same fid."), 'File');
+ $this->assertEqual($by_fid_file->filepath, $file->filepath, t("Loading by fid got the correct filepath."), 'File');
+ $this->assertEqual($by_fid_file->filename, $file->filename, t("Loading by fid got the correct filename."), 'File');
+ $this->assertEqual($by_fid_file->filemime, $file->filemime, t("Loading by fid got the correct MIME type."), 'File');
+ $this->assertEqual($by_fid_file->status, $file->status, t("Loading by fid got the correct status."), 'File');
+ $this->assertTrue($by_fid_file->file_test['loaded'], t('file_test_file_load() was able to modify the file during load.'));
+ }
+
+ /**
+ * This will test loading file data from the database.
*/
- function testFileLoad() {
+ function testMultiple() {
// Create a new file object.
$file = array(
'uid' => 1,
@@ -913,25 +955,24 @@ class FileLoadTest extends FileHookTestCase {
// Load by path.
file_test_reset();
- $by_path_file = file_load(array('filepath' => $file->filepath));
- $this->assertFileHookCalled('load');
+ $by_path_files = file_load_multiple(array(), array('filepath' => $file->filepath));
+ $this->assertFileHookCalled('load', 1);
+ $this->assertEqual(1, count($by_path_files), t('file_load_multiple() returned an array of the correct size.'));
+ $by_path_file = reset($by_path_files);
$this->assertTrue($by_path_file->file_test['loaded'], t('file_test_file_load() was able to modify the file during load.'));
$this->assertEqual($by_path_file->fid, $file->fid, t("Loading by filepath got the correct fid."), 'File');
// Load by fid.
file_test_reset();
- $by_fid_file = file_load($file->fid);
- $this->assertFileHookCalled('load', 0);
+ $by_fid_files = file_load_multiple(array($file->fid), array());
+ $this->assertFileHookCalled('load', 1);
+ $this->assertEqual(1, count($by_fid_files), t('file_load_multiple() returned an array of the correct size.'));
+ $by_fid_file = reset($by_fid_files);
$this->assertTrue($by_fid_file->file_test['loaded'], t('file_test_file_load() was able to modify the file during load.'));
$this->assertEqual($by_fid_file->filepath, $file->filepath, t("Loading by fid got the correct filepath."), 'File');
-
- // Load again by fid but use the reset param to reload.
- file_test_reset();
- $by_fid_file = file_load($file->fid, TRUE);
- $this->assertFileHookCalled('load');
- $this->assertEqual($by_fid_file->filepath, $file->filepath, t("Loading by fid got the correct filepath."), 'File');
}
}
+
/**
* Tests the file_save() function.
*/
@@ -1061,4 +1102,4 @@ class FileSaveDataTest extends FileHookTestCase {
$file = file_save_data($contents, 'asdf.txt', FILE_EXISTS_ERROR);
$this->assertFalse($file, t("Overwriting a file fails when FILE_EXISTS_ERROR is specified."));
}
-}
\ No newline at end of file
+}
diff --git a/modules/simpletest/tests/file_test.module b/modules/simpletest/tests/file_test.module
index 4e7c7e965833..7a9644f0dac0 100644
--- a/modules/simpletest/tests/file_test.module
+++ b/modules/simpletest/tests/file_test.module
@@ -148,11 +148,13 @@ function file_test_set_return($op, $value) {
/**
* Implementation of hook_file_load().
*/
-function file_test_file_load($file) {
- _file_test_log_call('load', array($file));
- // Assign a value on the object so that we can test that the $file is passed
- // by reference.
- $file->file_test['loaded'] = TRUE;
+function file_test_file_load($files) {
+ foreach ($files as $file) {
+ _file_test_log_call('load', array($file));
+ // Assign a value on the object so that we can test that the $file is passed
+ // by reference.
+ $file->file_test['loaded'] = TRUE;
+ }
}
/**
diff --git a/modules/system/system.api.php b/modules/system/system.api.php
index 4738e2347b6f..3ac517fd4177 100644
--- a/modules/system/system.api.php
+++ b/modules/system/system.api.php
@@ -983,23 +983,24 @@ function custom_url_rewrite_inbound(&$result, $path, $path_language) {
}
/**
- * Load additional information into a file object.
+ * Load additional information into file objects.
*
- * file_load() calls this hook to allow modules to load additional information
- * into the $file.
+ * file_load_multiple() calls this hook to allow modules to load
+ * additional information into each file.
*
- * @param $file
- * The file object being loaded.
- * @return
- * None.
+ * @param $files
+ * An array of file objects, indexed by fid.
*
- * @see file_load()
+ * @see file_load_multiple()
+ * @see upload_file_load()
*/
-function hook_file_load(&$file) {
+function hook_file_load($files) {
// Add the upload specific data into the file object.
- $values = db_query('SELECT * FROM {upload} u WHERE u.fid = :fid', array(':fid' => $file->fid))->fetch(PDO::FETCH_ASSOC);
- foreach ((array)$values as $key => $value) {
- $file->{$key} = $value;
+ $result = db_query('SELECT * FROM {upload} u WHERE u.fid IN (:fids)', array(':fids' => array_keys($files)))->fetchAll(PDO::FETCH_ASSOC);
+ foreach ($result as $record) {
+ foreach ($record as $key => $value) {
+ $files[$record['fid']]->$key = $value;
+ }
}
}
diff --git a/modules/upload/upload.module b/modules/upload/upload.module
index 74f95783defc..3b9a87b6b9b7 100644
--- a/modules/upload/upload.module
+++ b/modules/upload/upload.module
@@ -269,11 +269,13 @@ function upload_form_alter(&$form, $form_state, $form_id) {
/**
* Implementation of hook_file_load().
*/
-function upload_file_load(&$file) {
+function upload_file_load($files) {
// Add the upload specific data into the file object.
- $values = db_query('SELECT * FROM {upload} u WHERE u.fid = :fid', array(':fid' => $file->fid))->fetch(PDO::FETCH_ASSOC);
- foreach ((array)$values as $key => $value) {
- $file->{$key} = $value;
+ $result = db_query('SELECT * FROM {upload} u WHERE u.fid IN (:fids)', array(':fids' => array_keys($files)))->fetchAll(PDO::FETCH_ASSOC);
+ foreach ($result as $record) {
+ foreach ($record as $key => $value) {
+ $files[$record['fid']]->$key = $value;
+ }
}
}
@@ -297,16 +299,42 @@ function upload_file_delete(&$file) {
db_delete('upload')->condition('fid', $file->fid)->execute();
}
-
/**
* Implementation of hook_nodeapi_load().
*/
function upload_nodeapi_load($nodes, $types) {
+ // Collect all the revision ids for nodes with upload enabled.
+ $node_vids = array();
foreach ($nodes as $node) {
if (variable_get("upload_$node->type", 1) == 1) {
- $node->files = upload_load($node);
+ $node_vids[$node->vid] = $node->vid;
+ $node->files = array();
}
}
+ // If there are no vids then there's no point trying to load files.
+ if (empty($node_vids)) {
+ return;
+ }
+
+ // Fetch the fids associated with these node revisions.
+ $result = db_query('SELECT u.fid, u.nid, u.vid FROM {upload} u WHERE u.vid IN (:node_vids) ORDER BY u.weight, u.fid', array(':node_vids' => $node_vids));
+
+ // The same file may be attached to several nodes (e.g. translated nodes) so
+ // simply calling db_query()->fetchAllAssoc('fid') would return one node
+ // per file. Instead we build one array with the file ids for
+ // file_load_multiple() and another array with upload records so we can match
+ // files back to the nodes.
+ $fids = array();
+ $uploads = array();
+ foreach ($result as $record) {
+ $fids[] = $record->fid;
+ $uploads[] = $record;
+ }
+
+ $files = file_load_multiple($fids);
+ foreach ($uploads as $upload) {
+ $nodes[$upload->nid]->files[$upload->fid] = $files[$upload->fid];
+ }
}
/**
@@ -324,7 +352,7 @@ function upload_nodeapi_view($node, $teaser, $page) {
);
}
}
-
+
upload_nodeapi_links($node, $teaser);
}
}
@@ -609,19 +637,6 @@ function theme_upload_form_new($form) {
return $output;
}
-function upload_load($node) {
- $files = array();
-
- if ($node->vid) {
- $result = db_query('SELECT u.fid FROM {upload} u WHERE u.vid = :vid ORDER BY u.weight, u.fid', array(':vid' => $node->vid));
- foreach ($result as $file) {
- $files[$file->fid] = file_load($file->fid);
- }
- }
-
- return $files;
-}
-
/**
* Menu-callback for JavaScript-based uploads.
*/
--
GitLab