BUG Correct behaviour of publish with $createNewVersion = true

Fixes #5040
Cleanup code to make behaviour more apparent
This commit is contained in:
Damian Mooyman 2016-02-22 18:42:48 +13:00
parent 94cec68c7e
commit 65a0981c08
2 changed files with 99 additions and 56 deletions

View File

@ -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 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
* number will be copied over.
* @param int|string $fromStage Place to copy from. Can be either a stage name or a version number.
* @param string $toStage Place to copy to. Must be a stage name.
* @param bool $createNewVersion Set this to true to create a new version number.
* 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;
while( ($p = get_parent_class($baseClass)) != "DataObject") $baseClass = $p;
$baseClass = ClassInfo::baseDataClass($this->owner->class);
$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;
if($from) {
$from->forceChange();
if($createNewVersion) {
$latest = self::get_latest_version($baseClass, $this->owner->ID);
$this->owner->Version = $latest->Version + 1;
} else {
$from->migrateVersion($from->Version);
}
// Set version of new record
$from->forceChange();
if($createNewVersion) {
// Clear version to be automatically created on write
$from->Version = null;
} else {
$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);
}
/**

View File

@ -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',
sprintf('"VersionedTest_DataObject"."ID" = %d', $page1->ID));
$page1 = Versioned::get_one_by_stage('VersionedTest_DataObject', 'Stage', array(
'"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