- Patch #966238 by cosmicdreams, bfroehle, catch, mfb, carlos8f, marcingy: system_update_7061() stops migrating data if upload.fid is not found in files table, resulting in data loss.

3 files changed, 148 insertions, 104 deletions

diff --git a/modules/simpletest/tests/upgrade/drupal-6.upload.database.php b/modules/simpletest/tests/upgrade/drupal-6.upload.database.php
index 048aee0c4..94733ed4c 100644
--- a/modules/simpletest/tests/upgrade/drupal-6.upload.database.php
+++ b/modules/simpletest/tests/upgrade/drupal-6.upload.database.php
@@ -11,6 +11,10 @@ db_insert('files')->fields(array(
   'status',
   'timestamp',
 ))
+/*
+ * This entry is deliberately omitted to test the upgrade routine when facing
+ * possible data corruption.
+ *
 ->values(array(
   'fid' => '1',
   'uid' => '1',
@@ -20,7 +24,7 @@ db_insert('files')->fields(array(
   'filesize' => '1011',
   'status' => '1',
   'timestamp' => '1285700240',
-))
+)) */
 ->values(array(
   'fid' => '2',
   'uid' => '1',
@@ -132,14 +136,14 @@ db_insert('node')->fields(array(
 ))
 ->values(array(
   'nid' => '38',
-  'vid' => '51',
+  'vid' => '50',
   'type' => 'page',
   'language' => '',
-  'title' => 'node title 38 revision 51',
+  'title' => 'node title 38 revision 50',
   'uid' => '1',
   'status' => '1',
-  'created' => '1285700317',
-  'changed' => '1285700600',
+  'created' => '1285603317',
+  'changed' => '1285603317',
   'comment' => '0',
   'promote' => '0',
   'moderate' => '0',
@@ -152,7 +156,24 @@ db_insert('node')->fields(array(
   'vid' => '52',
   'type' => 'page',
   'language' => '',
-  'title' => 'node title 39 revision 53',
+  'title' => 'node title 39 revision 52',
+  'uid' => '1',
+  'status' => '1',
+  'created' => '1285700317',
+  'changed' => '1285700600',
+  'comment' => '0',
+  'promote' => '0',
+  'moderate' => '0',
+  'sticky' => '0',
+  'tnid' => '0',
+  'translate' => '0',
+))
+->values(array(
+  'nid' => '40',
+  'vid' => '53',
+  'type' => 'page',
+  'language' => '',
+  'title' => 'node title 40 revision 53',
   'uid' => '1',
   'status' => '1',
   'created' => '1285709012',
@@ -182,6 +203,17 @@ db_insert('node_revisions')->fields(array(
   'vid' => '50',
   'uid' => '1',
   'title' => 'node title 38 revision 50',
+  'body' => "Attachments:\r\npowered-blue-80x15.png",
+  'teaser' => "Attachments:\r\npowered-blue-80x15.png",
+  'log' => '',
+  'timestamp' => '1285603317',
+  'format' => '1',
+))
+->values(array(
+  'nid' => '39',
+  'vid' => '51',
+  'uid' => '1',
+  'title' => 'node title 39 revision 51',
   'body' => "Attachments:\r\npowered-blue-80x15.png\r\npowered-blue-88x31.png\r\npowered-blue-135x42.png",
   'teaser' => "Attachments:\r\npowered-blue-80x15.png\r\npowered-blue-88x31.png\r\npowered-blue-135x42.png",
   'log' => '',
@@ -189,10 +221,10 @@ db_insert('node_revisions')->fields(array(
   'format' => '1',
 ))
 ->values(array(
-  'nid' => '38',
-  'vid' => '51',
+  'nid' => '39',
+  'vid' => '52',
   'uid' => '1',
-  'title' => 'node title 38 revision 51',
+  'title' => 'node title 39 revision 52',
   'body' => "Attachments:\r\npowered-blue-88x31.png\r\npowered-black-80x15.png\r\npowered-black-135x42.png",
   'teaser' => "Attachments:\r\npowered-blue-88x31.png\r\npowered-black-80x15.png\r\npowered-black-135x42.png",
   'log' => '',
@@ -200,10 +232,10 @@ db_insert('node_revisions')->fields(array(
   'format' => '1',
 ))
 ->values(array(
-  'nid' => '39',
-  'vid' => '52',
+  'nid' => '40',
+  'vid' => '53',
   'uid' => '1',
-  'title' => 'node title 39 revision 53',
+  'title' => 'node title 40 revision 53',
   'body' => "Attachments:\r\nforum-hot-new.png\r\nforum-hot.png\r\nforum-sticky.png\r\nforum-new.png",
   'teaser' => "Attachments:\r\nforum-hot-new.png\r\nforum-hot.png\r\nforum-sticky.png\r\nforum-new.png",
   'log' => '',
@@ -276,7 +308,7 @@ db_insert('upload')->fields(array(
   'weight',
 ))
 ->values(array(
-  'fid' => '2',
+  'fid' => '1',
   'nid' => '38',
   'vid' => '50',
   'description' => 'powered-blue-80x15.png',
@@ -284,73 +316,81 @@ db_insert('upload')->fields(array(
   'weight' => '0',
 ))
 ->values(array(
+  'fid' => '2',
+  'nid' => '39',
+  'vid' => '51',
+  'description' => 'powered-blue-80x15.png',
+  'list' => '1',
+  'weight' => '0',
+))
+->values(array(
   'fid' => '3',
-  'nid' => '38',
-  'vid' => '50',
+  'nid' => '39',
+  'vid' => '51',
   'description' => 'powered-blue-88x31.png',
   'list' => '1',
   'weight' => '0',
 ))
 ->values(array(
   'fid' => '4',
-  'nid' => '38',
-  'vid' => '50',
+  'nid' => '39',
+  'vid' => '51',
   'description' => 'powered-blue-135x42.png',
   'list' => '1',
   'weight' => '0',
 ))
 ->values(array(
   'fid' => '3',
-  'nid' => '38',
-  'vid' => '51',
+  'nid' => '39',
+  'vid' => '52',
   'description' => 'powered-blue-88x31.png',
   'list' => '1',
   'weight' => '0',
 ))
 ->values(array(
   'fid' => '5',
-  'nid' => '38',
-  'vid' => '51',
+  'nid' => '39',
+  'vid' => '52',
   'description' => 'powered-black-80x15.png',
   'list' => '1',
   'weight' => '0',
 ))
 ->values(array(
   'fid' => '6',
-  'nid' => '38',
-  'vid' => '51',
+  'nid' => '39',
+  'vid' => '52',
   'description' => 'powered-black-135x42.png',
   'list' => '1',
   'weight' => '0',
 ))
 ->values(array(
   'fid' => '7',
-  'nid' => '39',
-  'vid' => '52',
+  'nid' => '40',
+  'vid' => '53',
   'description' => 'forum-hot-new.png',
   'list' => '1',
   'weight' => '-4',
 ))
 ->values(array(
   'fid' => '8',
-  'nid' => '39',
-  'vid' => '52',
+  'nid' => '40',
+  'vid' => '53',
   'description' => 'forum-hot.png',
   'list' => '1',
   'weight' => '-3',
 ))
 ->values(array(
   'fid' => '10',
-  'nid' => '39',
-  'vid' => '52',
+  'nid' => '40',
+  'vid' => '53',
   'description' => 'forum-sticky.png',
   'list' => '1',
   'weight' => '-2',
 ))
 ->values(array(
   'fid' => '9',
-  'nid' => '39',
-  'vid' => '52',
+  'nid' => '40',
+  'vid' => '53',
   'description' => 'forum-new.png',
   'list' => '1',
   'weight' => '-1',
diff --git a/modules/simpletest/tests/upgrade/upgrade.upload.test b/modules/simpletest/tests/upgrade/upgrade.upload.test
index efbff1114..761d6558d 100644
--- a/modules/simpletest/tests/upgrade/upgrade.upload.test
+++ b/modules/simpletest/tests/upgrade/upgrade.upload.test
@@ -20,6 +20,8 @@ class UploadUpgradePathTestCase extends UpgradePathTestCase {
       drupal_get_path('module', 'simpletest') . '/tests/upgrade/drupal-6.upload.database.php',
     );
     parent::setUp();
+    // Set a small batch size to test multiple iterations of the batch.
+    $this->variable_set('upload_update_batch_size', 2);
 
     $this->uninstallModulesExcept(array('upload'));
   }
@@ -36,8 +38,11 @@ class UploadUpgradePathTestCase extends UpgradePathTestCase {
     $query->fieldCondition('upload');
     $entities = $query->execute();
     $revisions = $entities['node'];
-    // Node revisions 50-52 should have uploaded files.
-    $this->assertTrue((isset($revisions[50]) && isset($revisions[51]) && isset($revisions[52])), 'Nodes with uploaded files now contain filefield data.');
+    // Node revision 50 should not have uploaded files, as the entry in {files}
+    // is corrupted.
+    $this->assertFalse((isset($revisions[50])), 'Nodes with missing files do not contain filefield data.');
+    // Node revisions 51-53 should have uploaded files.
+    $this->assertTrue((isset($revisions[51]) && isset($revisions[52]) && isset($revisions[53])), 'Nodes with uploaded files now contain filefield data.');
     // The test database lists uploaded filenames in the body of each node with
     // uploaded files attached. Make sure all files are there in the same order.
     foreach ($revisions as $vid => $revision) {
@@ -58,9 +63,7 @@ class UploadUpgradePathTestCase extends UpgradePathTestCase {
       foreach ($files as $file) {
         $filenames[] = $file['filename'];
       }
-
-      $diff = array_diff($filenames, $recorded_filenames);
-      $this->assertTrue(empty($diff), 'The uploaded files are present in the same order after the upgrade.');
+      $this->assertIdentical($filenames, $recorded_filenames, 'The uploaded files are present in the same order after the upgrade.');
     }
   }
 }
diff --git a/modules/system/system.install b/modules/system/system.install
index f56ef4055..94c0bb2ee 100644
--- a/modules/system/system.install
+++ b/modules/system/system.install
@@ -2772,93 +2772,94 @@ function system_update_7061(&$sandbox) {
     $sandbox['max'] = db_query("SELECT COUNT(*) FROM {system_update_7061}")->fetchField();
   }
 
-  $node_revisions = array();
-
   // Determine vids for this batch.
   // Process all files attached to a given revision during the same batch.
-  $limit = 100;
+  $limit = variable_get('upload_update_batch_size', 100);
   $vids = db_query_range('SELECT vid FROM {system_update_7061} WHERE vid > :lastvid ORDER BY vid', 0, $limit, array(':lastvid' => $sandbox['last_vid_processed']))
     ->fetchCol();
 
   // Retrieve information on all the files attached to these revisions.
-  $result = db_query('SELECT u.fid, u.vid, u.list, u.description, n.nid, n.type, u.weight FROM {upload} u INNER JOIN {node_revision} nr ON u.vid = nr.vid INNER JOIN {node} n ON n.nid = nr.nid WHERE u.vid IN (:vids) ORDER BY u.vid, u.weight, u.fid', array(':vids' => $vids));
-  foreach ($result as $record) {
-    // For each uploaded file, retrieve the corresponding data from the old
-    // files table (since upload doesn't know about the new entry in the
-    // file_managed table).
-    $file = db_select('files', 'f')
-      ->fields('f', array('fid', 'uid', 'filename', 'filepath', 'filemime', 'filesize', 'status', 'timestamp'))
-      ->condition('f.fid', $record->fid)
-      ->execute()
-      ->fetchAssoc();
-    if (!$file) {
-      continue;
-    }
+  if (!empty($vids)) {
+    $node_revisions = array();
+    $result = db_query('SELECT u.fid, u.vid, u.list, u.description, n.nid, n.type, u.weight FROM {upload} u INNER JOIN {node_revision} nr ON u.vid = nr.vid INNER JOIN {node} n ON n.nid = nr.nid WHERE u.vid IN (:vids) ORDER BY u.vid, u.weight, u.fid', array(':vids' => $vids));
+    foreach ($result as $record) {
+      // For each uploaded file, retrieve the corresponding data from the old
+      // files table (since upload doesn't know about the new entry in the
+      // file_managed table).
+      $file = db_select('files', 'f')
+        ->fields('f', array('fid', 'uid', 'filename', 'filepath', 'filemime', 'filesize', 'status', 'timestamp'))
+        ->condition('f.fid', $record->fid)
+        ->execute()
+        ->fetchAssoc();
+      if (!$file) {
+        continue;
+      }
 
-    // Add in the file information from the upload table.
-    $file['description'] = $record->description;
-    $file['display'] = $record->list;
+      // Add in the file information from the upload table.
+      $file['description'] = $record->description;
+      $file['display'] = $record->list;
 
-    // Create one record for each revision that contains all the uploaded files.
-    $node_revisions[$record->vid]['nid'] = $record->nid;
-    $node_revisions[$record->vid]['vid'] = $record->vid;
-    $node_revisions[$record->vid]['type'] = $record->type;
-    $node_revisions[$record->vid]['file'][LANGUAGE_NONE][] = $file;
-  }
+      // Create one record for each revision that contains all the uploaded
+      // files.
+      $node_revisions[$record->vid]['nid'] = $record->nid;
+      $node_revisions[$record->vid]['vid'] = $record->vid;
+      $node_revisions[$record->vid]['type'] = $record->type;
+      $node_revisions[$record->vid]['file'][LANGUAGE_NONE][] = $file;
+    }
 
-  // Now that we know which files belong to which revisions, update the files'
-  // database entries, and save a reference to each file in the upload field on
-  // their node revisions.
-  $basename = variable_get('file_directory_path', conf_path() . '/files');
-  $scheme = file_default_scheme() . '://';
-  foreach ($node_revisions as $vid => $revision) {
-    foreach ($revision['file'][LANGUAGE_NONE] as $delta => $file) {
-      // We will convert filepaths to uri using the default scheme
-      // and stripping off the existing file directory path.
-      $file['uri'] = $scheme . str_replace($basename, '', $file['filepath']);
-      $file['uri'] = file_stream_wrapper_uri_normalize($file['uri']);
-      unset($file['filepath']);
-      // Insert into the file_managed table.
-      // Each fid should only be stored once in file_managed.
-      db_merge('file_managed')
-        ->key(array(
-          'fid' => $file['fid'],
-        ))
-        ->fields(array(
-          'uid' => $file['uid'],
-          'filename' => $file['filename'],
-          'uri' => $file['uri'],
-          'filemime' => $file['filemime'],
-          'filesize' => $file['filesize'],
-          'status' => $file['status'],
-          'timestamp' => $file['timestamp'],
-        ))
-        ->execute();
+    // Now that we know which files belong to which revisions, update the
+    // files'// database entries, and save a reference to each file in the
+    // upload field on their node revisions.
+    $basename = variable_get('file_directory_path', conf_path() . '/files');
+    $scheme = file_default_scheme() . '://';
+    foreach ($node_revisions as $vid => $revision) {
+      foreach ($revision['file'][LANGUAGE_NONE] as $delta => $file) {
+        // We will convert filepaths to uri using the default scheme
+        // and stripping off the existing file directory path.
+        $file['uri'] = $scheme . str_replace($basename, '', $file['filepath']);
+        $file['uri'] = file_stream_wrapper_uri_normalize($file['uri']);
+        unset($file['filepath']);
+        // Insert into the file_managed table.
+        // Each fid should only be stored once in file_managed.
+        db_merge('file_managed')
+          ->key(array(
+            'fid' => $file['fid'],
+          ))
+          ->fields(array(
+            'uid' => $file['uid'],
+            'filename' => $file['filename'],
+            'uri' => $file['uri'],
+            'filemime' => $file['filemime'],
+            'filesize' => $file['filesize'],
+            'status' => $file['status'],
+            'timestamp' => $file['timestamp'],
+          ))
+          ->execute();
+
+        // Add the usage entry for the file.
+        $file = (object) $file;
+        file_usage_add($file, 'file', 'node', $revision['nid']);
+
+        // Update the node revision's upload file field with the file data.
+        $revision['file'][LANGUAGE_NONE][$delta] = array('fid' => $file->fid, 'display' => $file->display, 'description' => $file->description);
+      }
 
-      // Add the usage entry for the file.
-      $file = (object) $file;
-      file_usage_add($file, 'file', 'node', $revision['nid']);
+      // Write the revision's upload field data into the field_upload tables.
+      $node = (object) $revision;
+      _update_7000_field_sql_storage_write('node', $node->type, $node->nid, $node->vid, 'upload', $node->file);
 
-      // Update the node revision's upload file field with the file data.
-      $revision['file'][LANGUAGE_NONE][$delta] = array('fid' => $file->fid, 'display' => $file->display, 'description' => $file->description);
+      // Update our progress information for the batch update.
+      $sandbox['progress']++;
+      $sandbox['last_vid_processed'] = $vid;
     }
-
-    // Write the revision's upload field data into the field_upload tables.
-    $node = (object) $revision;
-    _update_7000_field_sql_storage_write('node', $node->type, $node->nid, $node->vid, 'upload', $node->file);
-
-    // Update our progress information for the batch update.
-    $sandbox['progress']++;
-    $sandbox['last_vid_processed'] = $vid;
   }
 
   // If less than limit node revisions were processed, the update process is
   // finished.
-  if (count($node_revisions) < $limit) {
+  if (count($vids) < $limit) {
     $finished = TRUE;
   }
 
-
   // If there's no max value then there's nothing to update and we're finished.
   if (empty($sandbox['max']) || isset($finished)) {
     db_drop_table('upload');