#261258 by pwolanin, Damien Tournoud, David_Rothstein: Fix node_save() insertion logic. (with tests)

2 files changed, 80 insertions, 20 deletions

diff --git a/modules/node/node.module b/modules/node/node.module
index 3791ebf8f..13fb334df 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -940,36 +940,43 @@ function node_save($node) {
     module_invoke_all('node_presave', $node);
     global $user;
 
+    // Determine if we will be inserting a new node.
     if (!isset($node->is_new)) {
       $node->is_new = empty($node->nid);
     }
 
-    // Apply filters to some default node fields:
-    if ($node->is_new) {
-      // Insert a new node.
-      $node->is_new = TRUE;
-
-      // When inserting a node, $node->log must be set because
-      // {node_revision}.log does not (and cannot) have a default
-      // value. If the user does not have permission to create
-      // revisions, however, the form will not contain an element for
-      // log so $node->log will be unset at this point.
+    if ($node->is_new || !empty($node->revision)) {
+      // When inserting either a new node or a new node revision, $node->log
+      // must be set because {node_revision}.log is a text column and therefore
+      // cannot have a default value. However, it might not be set at this
+      // point (for example, if the user submitting a node form does not have
+      // permission to create revisions), so we ensure that it is at least an
+      // empty string in that case.
+      // @todo: Make the {node_revision}.log column nullable so that we can
+      // remove this check.
       if (!isset($node->log)) {
         $node->log = '';
       }
     }
-    elseif (!empty($node->revision)) {
+    elseif (empty($node->log)) {
+      // If we are updating an existing node without adding a new revision, we
+      // need to make sure $node->log is unset whenever it is empty. As long as
+      // $node->log is unset, drupal_write_record() will not attempt to update
+      // the existing database column when re-saving the revision; therefore,
+      // this code allows us to avoid clobbering an existing log entry with an
+      // empty one.
+      unset($node->log);
+    }
+
+    // When saving a new node revision, unset any existing revision ID so as to
+    // ensure that a new revision will actually be created, and store the old
+    // revision ID in a separate property for use by node hook implementations.
+    if (!$node->is_new && !empty($node->revision) && $node->vid) {
       $node->old_vid = $node->vid;
       unset($node->vid);
     }
-    else {
-      // When updating a node, avoid clobbering an existing log entry with an empty one.
-      if (empty($node->log)) {
-        unset($node->log);
-      }
-    }
 
-    // Set some required fields:
+    // Set the timestamp fields.
     if (empty($node->created)) {
       $node->created = REQUEST_TIME;
     }
@@ -979,14 +986,21 @@ function node_save($node) {
     $node->timestamp = REQUEST_TIME;
     $update_node = TRUE;
 
-    // Generate the node table query and the node_revisions table query.
+    // Save the node and node revision.
     if ($node->is_new) {
+      // For new nodes, save new records for both the node itself and the node
+      // revision.
       drupal_write_record('node', $node);
       _node_save_revision($node, $user->uid);
       $op = 'insert';
     }
     else {
+      // For existing nodes, update the node record which matches the value of
+      // $node->nid.
       drupal_write_record('node', $node, 'nid');
+      // Then, if a new node revision was requested, save a new record for
+      // that; otherwise, update the node revision record which matches the
+      // value of $node->vid.
       if (!empty($node->revision)) {
         _node_save_revision($node, $user->uid);
       }
diff --git a/modules/node/node.test b/modules/node/node.test
index c9f4fbd76..7ec078d35 100644
--- a/modules/node/node.test
+++ b/modules/node/node.test
@@ -88,7 +88,7 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
   public static function getInfo() {
     return array(
       'name' => 'Node revisions',
-      'description' => 'Create a node with revisions and test viewing, reverting, and deleting revisions.',
+      'description' => 'Create a node with revisions and test viewing, saving, reverting, and deleting revisions.',
       'group' => 'Node',
     );
   }
@@ -164,6 +164,52 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
                               '@type' => 'Basic page', '%title' => $nodes[1]->title)), t('Revision deleted.'));
     $this->assertTrue(db_query('SELECT COUNT(vid) FROM {node_revision} WHERE nid = :nid and vid = :vid', array(':nid' => $node->nid, ':vid' => $nodes[1]->vid))->fetchField() == 0, t('Revision not found.'));
   }
+
+  /**
+   * Checks that revisions are correctly saved without log messages.
+   */
+  function testNodeRevisionWithoutLogMessage() {
+    // Create a node with an initial log message.
+    $log = $this->randomName(10);
+    $node = $this->drupalCreateNode(array('log' => $log));
+
+    // Save over the same revision but do not provide a log message, and check
+    // that the original log message is preserved.
+    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage1';
+    $updated_node = (object) array(
+      'nid' => $node->nid,
+      'vid' => $node->vid,
+      'uid' => $node->uid,
+      'type' => $node->type,
+      'title' => array(LANGUAGE_NONE => array(array('value' => $new_title))),
+      'log' => '',
+    );
+    node_save($updated_node);
+    $this->drupalGet('node/' . $node->nid);
+    $this->assertText($new_title, t('New node title appears on the page.'));
+    $node_revision = node_load($node->nid, NULL, TRUE);
+    $this->assertEqual($node_revision->log, $log, t('After an existing node revision is re-saved without a log message, the original log message is preserved.'));
+
+    // Create another node with an initial log message.
+    $node = $this->drupalCreateNode(array('log' => $log));
+
+    // Save a new node revision without providing a log message, and check that
+    // this revision has an empty log message.
+    $new_title = $this->randomName(10) . 'testNodeRevisionWithoutLogMessage2';
+    $updated_node = (object) array(
+      'nid' => $node->nid,
+      'vid' => $node->vid,
+      'uid' => $node->uid,
+      'type' => $node->type,
+      'title' => array(LANGUAGE_NONE => array(array('value' => $new_title))),
+      'revision' => 1,
+    );
+    node_save($updated_node);
+    $this->drupalGet('node/' . $node->nid);
+    $this->assertText($new_title, t('New node title appears on the page.'));
+    $node_revision = node_load($node->nid, NULL, TRUE);
+    $this->assertTrue(empty($node_revision->log), t('After a new node revision is saved with an empty log message, the log message for the node is empty.'));
+  }
 }
 
 class PageEditTestCase extends DrupalWebTestCase {