BUG Prevent Versioned::doRollbackTo from creating incorrect versions on subclasses of Versioned DataObjects

Document correct configuration of Versioned DataObjects
Fixes #4936

docs/en/02_Developer_Guides/00_Model/10_Versioning.md

@@ -27,6 +27,11 @@ The extension is automatically applied to `SiteTree` class. For more information
 [Extending](../extending) and the [Configuration](../configuration) documentation.
 </div>
 
+<div class="warning" markdown="1">
+Versioning only works if you are adding the extension to the base class. That is, the first subclass
+of `DataObject`. Adding this extension to children of the base class will have unpredictable behaviour.
+</div>
+
 ## Database Structure
 
 Depending on how many stages you configured, two or more new tables will be created for your records. In the above, this

model/Versioned.php

@@ -652,26 +652,31 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 			
 			// Get ID field
 			$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(!$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']);
+			} elseif(empty($version)) {
 				// If we haven't got a version #, then we're creating a new version.
 				// Otherwise, we're just copying a version to another table
-			if(empty($manipulation[$table]['fields']['Version'])) {
 				$this->augmentWriteVersioned($manipulation, $table, $id);
 			}
 
-			// Putting a Version of -1 is a signal to leave the version table alone, despite their being no version
+			// For base classes of versioned data objects
-			if($manipulation[$table]['fields']['Version'] < 0 || $this->_nextWriteWithoutVersion) {
+			if(!$this->hasVersionField($table)) {
 				unset($manipulation[$table]['fields']['Version']);
 			}
 
-			// For base classes of versioned data objects
-			if(!$this->hasVersionField($table)) unset($manipulation[$table]['fields']['Version']);
-
 			// Grab a version number - it should be the same across all tables.
 			if(isset($manipulation[$table]['fields']['Version'])) {
 				$thisVersion = $manipulation[$table]['fields']['Version'];

tests/model/DataObjectLazyLoadingTest.php

@@ -13,6 +13,7 @@ class DataObjectLazyLoadingTest extends SapphireTest {
 
 	// These are all defined in DataObjectTest.php and VersionedTest.php
 	protected $extraDataObjects = array(
+		// From DataObjectTest
 		'DataObjectTest_Team',
 		'DataObjectTest_Fixture',
 		'DataObjectTest_SubTeam',
@@ -24,8 +25,24 @@ class DataObjectLazyLoadingTest extends SapphireTest {
 		'DataObjectTest_TeamComment',
 		'DataObjectTest_EquipmentCompany',
 		'DataObjectTest_SubEquipmentCompany',
+		'DataObjectTest\NamespacedClass',
+		'DataObjectTest\RelationClass',
+		'DataObjectTest_ExtendedTeamComment',
+		'DataObjectTest_Company',
+		'DataObjectTest_Staff',
+		'DataObjectTest_CEO',
+		'DataObjectTest_Fan',
+		'DataObjectTest_Play',
+		'DataObjectTest_Ploy',
+		'DataObjectTest_Bogey',
+		// From VersionedTest
 		'VersionedTest_DataObject',
 		'VersionedTest_Subclass',
+		'VersionedTest_AnotherSubclass',
+		'VersionedTest_RelatedWithoutVersion',
+		'VersionedTest_SingleStage',
+		'VersionedTest_WithIndexes',
+		// From DataObjectLazyLoadingTest
 		'VersionedLazy_DataObject',
 		'VersionedLazySub_DataObject',
 	);

tests/model/VersionedTest.php

@@ -154,13 +154,13 @@ class VersionedTest extends SapphireTest {
 			"\"VersionedTest_DataObject\".\"ID\" ASC");
 		// Check that page 3 has gone
 		$this->assertNotNull($remainingPages);
-		$this->assertEquals(array("Page 1", "Page 2"), $remainingPages->column('Title'));
+		$this->assertEquals(array("Page 1", "Page 2", "Subclass Page 1"), $remainingPages->column('Title'));
 
 		// Get all including deleted
 		$allPages = Versioned::get_including_deleted("VersionedTest_DataObject", "\"ParentID\" = 0",
 			"\"VersionedTest_DataObject\".\"ID\" ASC");
 		// Check that page 3 is still there
-		$this->assertEquals(array("Page 1", "Page 2", "Page 3"), $allPages->column('Title'));
+		$this->assertEquals(array("Page 1", "Page 2", "Page 3", "Subclass Page 1"), $allPages->column('Title'));
 
 		// Check that the returned pages have the correct IDs
 		$this->assertEquals($allPageIDs, $allPages->column('ID'));
@@ -169,7 +169,7 @@ class VersionedTest extends SapphireTest {
 		Versioned::reading_stage("Live");
 		$allPages = Versioned::get_including_deleted("VersionedTest_DataObject", "\"ParentID\" = 0",
 			"\"VersionedTest_DataObject\".\"ID\" ASC");
-		$this->assertEquals(array("Page 1", "Page 2", "Page 3"), $allPages->column('Title'));
+		$this->assertEquals(array("Page 1", "Page 2", "Page 3", "Subclass Page 1"), $allPages->column('Title'));
 
 		// Check that the returned pages still have the correct IDs
 		$this->assertEquals($allPageIDs, $allPages->column('ID'));
@@ -208,7 +208,7 @@ class VersionedTest extends SapphireTest {
 	}
 
 	public function testRollbackTo() {
-		$page1 = $this->objFromFixture('VersionedTest_DataObject', 'page1');
+		$page1 = $this->objFromFixture('VersionedTest_AnotherSubclass', 'subclass1');
 		$page1->Content = 'orig';
 		$page1->write();
 		$page1->publish('Stage', 'Live');
@@ -225,6 +225,17 @@ class VersionedTest extends SapphireTest {
 
 		$this->assertTrue($page1->Version > $changedVersion, 'Create a new higher version number');
 		$this->assertEquals('orig', $page1->Content, 'Copies the content from the old version');
+
+		// check db entries
+		$version = DB::prepared_query("SELECT MAX(\"Version\") FROM \"VersionedTest_DataObject_versions\" WHERE \"RecordID\" = ?",
+			array($page1->ID)
+		)->value();
+		$this->assertEquals($page1->Version, $version, 'Correct entry in VersionedTest_DataObject_versions');
+
+		$version = DB::prepared_query("SELECT MAX(\"Version\") FROM \"VersionedTest_AnotherSubclass_versions\" WHERE \"RecordID\" = ?",
+			array($page1->ID)
+		)->value();
+		$this->assertEquals($page1->Version, $version, 'Correct entry in VersionedTest_AnotherSubclass_versions');
 	}
 
 	public function testDeleteFromStage() {
@@ -318,6 +329,7 @@ class VersionedTest extends SapphireTest {
 		$noversion    = new DataObject();
 		$versioned    = new VersionedTest_DataObject();
 		$versionedSub = new VersionedTest_Subclass();
+		$versionedAno = new VersionedTest_AnotherSubclass();
 		$versionField = new VersionedTest_UnversionedWithField();
 
 		$this->assertFalse(
@@ -329,8 +341,14 @@ class VersionedTest extends SapphireTest {
 			'The versioned ext adds an Int version field.'
 		);
 		$this->assertEquals(
-			'Int', $versionedSub->hasOwnTableDatabaseField('Version'),
+			null,
-			'Sub-classes of a versioned model have a Version field.'
+			$versionedSub->hasOwnTableDatabaseField('Version'),
+			'Sub-classes of a versioned model don\'t have a Version field.'
+		);
+		$this->assertEquals(
+			null,
+			$versionedAno->hasOwnTableDatabaseField('Version'),
+			'Sub-classes of a versioned model don\'t have a Version field.'
 		);
 		$this->assertEquals(
 			'Varchar', $versionField->hasOwnTableDatabaseField('Version'),
@@ -818,10 +836,6 @@ class VersionedTest_Subclass extends VersionedTest_DataObject implements TestOnl
 	private static $db = array(
 		"ExtraField" => "Varchar",
 	);
-
-	private static $extensions = array(
-		"Versioned('Stage', 'Live')"
-	);
 }
 
 /**

tests/model/VersionedTest.yml

@@ -17,3 +17,8 @@ VersionedTest_DataObject:
   page3b:
     Parent: =>VersionedTest_DataObject.page3
     Title: Page 3b
+
+VersionedTest_AnotherSubclass:
+  subclass1:
+    Title: 'Subclass Page 1'
+    AnotherField: 'Bob'