Merge pull request #5074 from tractorcow/pulls/3.2/fix-publish-version

BUG Correct behaviour of publish with $createNewVersion = true

model/Versioned.php

@@ -5,6 +5,8 @@
  * allowing you to rollback changes and view history. An example of this is
  * the pages used in the CMS.
  *
+ * @property int $Version
+ *
  * @package framework
  * @subpackage model
  */
@@ -487,12 +489,12 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 
 						foreach($versionedTables as $child) {
 							if($table === $child) break; // only need subclasses
-							
+
 							// Select all orphaned version records
 							$orphanedQuery = SQLSelect::create()
 								->selectField("\"{$table}_versions\".\"ID\"")
 								->setFrom("\"{$table}_versions\"");
-								
+
 							// If we have a parent table limit orphaned records
 							// to only those that exist in this
 							if(DB::get_schema()->hasTable("{$child}_versions")) {
@@ -568,7 +570,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	 */
 	protected function augmentWriteVersioned(&$manipulation, $table, $recordID) {
 		$baseDataClass = ClassInfo::baseDataClass($table);
-		
+
 		// Set up a new entry in (table)_versions
 		$newManipulation = array(
 			"command" => "insert",
@@ -606,15 +608,17 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 		}
 		$nextVersion = $nextVersion ?: 1;
 
-		// Add the version number to this data
-		$manipulation[$table]['fields']['Version'] = $nextVersion;
-		$newManipulation['fields']['Version'] = $nextVersion;
-
-		// Write AuthorID for baseclass
 		if($table === $baseDataClass) {
+			// Write AuthorID for baseclass
 			$userID = (Member::currentUser()) ? Member::currentUser()->ID : 0;
 			$newManipulation['fields']['AuthorID'] = $userID;
+
+			// Update main table version if not previously known
+			$manipulation[$table]['fields']['Version'] = $nextVersion;
 		}
+
+		// Update _versions table manipulation
+		$newManipulation['fields']['Version'] = $nextVersion;
 		$manipulation["{$table}_versions"] = $newManipulation;
 	}
 
@@ -641,6 +645,19 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 
 
 	public function augmentWrite(&$manipulation) {
+		// get Version number from base data table on write
+		$version = null;
+		$baseDataClass = ClassInfo::baseDataClass($this->owner->class);
+		if(isset($manipulation[$baseDataClass]['fields'])) {
+			if ($this->migratingVersion) {
+				$manipulation[$baseDataClass]['fields']['Version'] = $this->migratingVersion;
+			}
+			if (isset($manipulation[$baseDataClass]['fields']['Version'])) {
+				$version = $manipulation[$baseDataClass]['fields']['Version'];
+			}
+		}
+
+		// Update all tables
 		$tables = array_keys($manipulation);
 		foreach($tables as $table) {
 
@@ -649,20 +666,15 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 				unset($manipulation[$table]);
 				continue;
 			}
-			
+
 			// Get ID field
-			$id = $manipulation[$table]['id'] ? $manipulation[$table]['id'] : $manipulation[$table]['fields']['ID'];
+			$id = $manipulation[$table]['id']
+				? $manipulation[$table]['id']
+				: $manipulation[$table]['fields']['ID'];
 			if(!$id) {
 				user_error("Couldn't find ID in " . var_export($manipulation[$table], true), E_USER_ERROR);
 			}
 
-			if($this->migratingVersion) {
-				$manipulation[$table]['fields']['Version'] = $this->migratingVersion;
-			}
-
-			$version = isset($manipulation[$table]['fields']['Version'])
-				? $manipulation[$table]['fields']['Version']
-				: null;
 			if($version < 0 || $this->_nextWriteWithoutVersion) {
 				// Putting a Version of -1 is a signal to leave the version table alone, despite their being no version
 				unset($manipulation[$table]['fields']['Version']);
@@ -672,7 +684,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 				$this->augmentWriteVersioned($manipulation, $table, $id);
 			}
 
-			// For base classes of versioned data objects
+			// Remove "Version" column from subclasses of baseDataClass
 			if(!$this->hasVersionField($table)) {
 				unset($manipulation[$table]['fields']['Version']);
 			}
@@ -805,56 +817,64 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 	/**
 	 * Move a database record from one stage to the other.
 	 *
-	 * @param fromStage Place to copy from.  Can be either a stage name or a version number.
+	 * @param int|string $fromStage Place to copy from.  Can be either a stage name or a version number.
-	 * @param toStage Place to copy to.  Must be a stage name.
+	 * @param string $toStage Place to copy to.  Must be a stage name.
-	 * @param createNewVersion Set this to true to create a new version number.  By default, the existing version
+	 * @param bool $createNewVersion Set this to true to create a new version number.
-	 *                         number will be copied over.
+	 *  By default, the existing version number will be copied over.
 	 */
 	public function publish($fromStage, $toStage, $createNewVersion = false) {
 		$this->owner->extend('onBeforeVersionedPublish', $fromStage, $toStage, $createNewVersion);
 
-		$baseClass = $this->owner->class;
+		$baseClass = ClassInfo::baseDataClass($this->owner->class);
-		while( ($p = get_parent_class($baseClass)) != "DataObject") $baseClass = $p;
 		$extTable = $this->extendWithSuffix($baseClass);
 
+		/** @var Versioned|DataObject $from */
 		if(is_numeric($fromStage)) {
 			$from = Versioned::get_version($baseClass, $this->owner->ID, $fromStage);
 		} else {
 			$this->owner->flushCache();
-			$from = Versioned::get_one_by_stage($baseClass, $fromStage, "\"{$baseClass}\".\"ID\"={$this->owner->ID}");
+			$from = Versioned::get_one_by_stage($baseClass, $fromStage, array(
+				"\"{$baseClass}\".\"ID\" = ?" => $this->owner->ID
+			));
+		}
+		if(!$from) {
+			user_error("Can't find {$this->owner->class}/{$this->owner->ID} in stage {$fromStage}", E_USER_WARNING);
+			return;
 		}
 
-		$publisherID = isset(Member::currentUser()->ID) ? Member::currentUser()->ID : 0;
+		// Set version of new record
-		if($from) {
+		$from->forceChange();
-			$from->forceChange();
+		if($createNewVersion) {
-			if($createNewVersion) {
+			// Clear version to be automatically created on write
-				$latest = self::get_latest_version($baseClass, $this->owner->ID);
+			$from->Version = null;
-				$this->owner->Version = $latest->Version + 1;
+		} else {
-			} else {
+			$from->migrateVersion($from->Version);
-				$from->migrateVersion($from->Version);
-			}
 
 			// Mark this version as having been published at some stage
+			$publisherID = isset(Member::currentUser()->ID) ? Member::currentUser()->ID : 0;
 			DB::prepared_query("UPDATE \"{$extTable}_versions\"
 				SET \"WasPublished\" = ?, \"PublisherID\" = ?
 				WHERE \"RecordID\" = ? AND \"Version\" = ?",
 				array(1, $publisherID, $from->ID, $from->Version)
 			);
-
-			$oldMode = Versioned::get_reading_mode();
-			Versioned::reading_stage($toStage);
-
-			$conn = DB::get_conn();
-			if(method_exists($conn, 'allowPrimaryKeyEditing')) $conn->allowPrimaryKeyEditing($baseClass, true);
-			$from->write();
-			if(method_exists($conn, 'allowPrimaryKeyEditing')) $conn->allowPrimaryKeyEditing($baseClass, false);
-
-			$from->destroy();
-
-			Versioned::set_reading_mode($oldMode);
-		} else {
-			user_error("Can't find {$this->owner->URLSegment}/{$this->owner->ID} in stage $fromStage", E_USER_WARNING);
 		}
+
+		// Change to new stage, write, and revert state
+		$oldMode = Versioned::get_reading_mode();
+		Versioned::reading_stage($toStage);
+
+		$conn = DB::get_conn();
+		if(method_exists($conn, 'allowPrimaryKeyEditing')) {
+			$conn->allowPrimaryKeyEditing($baseClass, true);
+			$from->write();
+			$conn->allowPrimaryKeyEditing($baseClass, false);
+		} else {
+			$from->write();
+		}
+
+		$from->destroy();
+
+		Versioned::set_reading_mode($oldMode);
 	}
 
 	/**

tests/model/VersionedTest.php

@@ -31,7 +31,7 @@ class VersionedTest extends SapphireTest {
 			'VersionedTest_WithIndexes_Live' =>
 				array('value' => false, 'message' => 'Unique indexes are no longer unique in _Live table'),
 		);
-	
+
 		// Test each table's performance
 		foreach ($tableExpectations as $tableName => $expectation) {
 			$indexes = DB::get_schema()->indexList($tableName);
@@ -196,15 +196,37 @@ class VersionedTest extends SapphireTest {
 		$page1 = $this->objFromFixture('VersionedTest_DataObject', 'page1');
 		$page1->Content = 'orig';
 		$page1->write();
-		$oldVersion = $page1->Version;
+		$firstVersion = $page1->Version;
 		$page1->publish('Stage', 'Live', false);
-		$this->assertEquals($oldVersion, $page1->Version, 'publish() with $createNewVersion=FALSE');
+		$this->assertEquals(
+			$firstVersion,
+			$page1->Version,
+			'publish() with $createNewVersion=FALSE does not create a new version'
+		);
 
 		$page1->Content = 'changed';
 		$page1->write();
-		$oldVersion = $page1->Version;
+		$secondVersion = $page1->Version;
+		$this->assertTrue($firstVersion < $secondVersion, 'write creates new version');
+
 		$page1->publish('Stage', 'Live', true);
-		$this->assertTrue($oldVersion < $page1->Version, 'publish() with $createNewVersion=TRUE');
+		$thirdVersion = Versioned::get_latest_version('VersionedTest_DataObject', $page1->ID)->Version;
+		$liveVersion = Versioned::get_versionnumber_by_stage('VersionedTest_DataObject', 'Live', $page1->ID);
+		$stageVersion = Versioned::get_versionnumber_by_stage('VersionedTest_DataObject', 'Stage', $page1->ID);
+		$this->assertTrue(
+			$secondVersion < $thirdVersion,
+			'publish() with $createNewVersion=TRUE creates a new version'
+		);
+		$this->assertEquals(
+			$liveVersion,
+			$thirdVersion,
+			'publish() with $createNewVersion=TRUE publishes to live'
+		);
+		$this->assertEquals(
+			$stageVersion,
+			$secondVersion,
+			'publish() with $createNewVersion=TRUE does not affect stage'
+		);
 	}
 
 	public function testRollbackTo() {
@@ -220,10 +242,11 @@ class VersionedTest extends SapphireTest {
 		$changedVersion = $page1->Version;
 
 		$page1->doRollbackTo($origVersion);
-		$page1 = Versioned::get_one_by_stage('VersionedTest_DataObject', 'Stage',
+		$page1 = Versioned::get_one_by_stage('VersionedTest_DataObject', 'Stage', array(
-			sprintf('"VersionedTest_DataObject"."ID" = %d', $page1->ID));
+			'"VersionedTest_DataObject"."ID" = ?' => $page1->ID
+		));
 
-		$this->assertTrue($page1->Version > $changedVersion, 'Create a new higher version number');
+		$this->assertTrue($page1->Version == $changedVersion + 1, 'Create a new higher version number');
 		$this->assertEquals('orig', $page1->Content, 'Copies the content from the old version');
 
 		// check db entries