From 22c5f3129c24ce8b1b39b29012f37f10f23f4893 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 10 Aug 2012 12:45:37 +1200 Subject: [PATCH 1/5] FIXED: Issue where viewing an archived version of a page caused invalid SQL to be generated. This would only occur with subclasses of Page. --- model/Versioned.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/Versioned.php b/model/Versioned.php index 5e938676c..151810f62 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -154,7 +154,7 @@ class Versioned extends DataExtension { $query->selectField(sprintf('"%s_versions"."%s"', $baseTable, 'RecordID'), "ID"); if($table != $baseTable) { - $query->addFrom(array($table => " AND \"{$table}_versions\".\"Version\" = \"{$baseTable}_versions\".\"Version\"")); + $query->addWhere("\"{$table}_versions\".\"Version\" = \"{$baseTable}_versions\".\"Version\""); } } From c55b018feb0f89541f1524ba2a744f0b992c6eef Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 10 Aug 2012 13:54:29 +1200 Subject: [PATCH 2/5] FIXED: Issue where versioned would join _versions tables on ID,Version instead of RecordID,Version --- model/Versioned.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/model/Versioned.php b/model/Versioned.php index 151810f62..a78d46af4 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -145,7 +145,8 @@ class Versioned extends DataExtension { $date = $dataQuery->getQueryParam('Versioned.date'); foreach($query->getFrom() as $table => $dummy) { $query->renameTable($table, $table . '_versions'); - $query->replaceText("\"$table\".\"ID\"", "\"$table\".\"RecordID\""); + $query->replaceText("\"{$table}_versions\".\"ID\"", "\"{$table}_versions\".\"RecordID\""); + $query->replaceText("`{$table}_versions`.`ID`", "`{$table}_versions`.`RecordID`"); // Add all _versions columns foreach(self::$db_for_versions_table as $name => $type) { From 0f09305e3d31a98e41cfb9158dda14ba584931db Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 20 Aug 2012 13:24:28 +1200 Subject: [PATCH 3/5] FIXED: Issue where temporary table would cause unpredictable behaviour. Temporary table functionality was substituted with subqueries in each use case. ADDED: Test case for version archive functionality. --- model/Versioned.php | 59 ++++++++++++++--------------------- tests/model/VersionedTest.php | 53 +++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/model/Versioned.php b/model/Versioned.php index a78d46af4..78e71c451 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -160,10 +160,19 @@ class Versioned extends DataExtension { } // Link to the version archived on that date - $archiveTable = $this->requireArchiveTempTable($baseTable, $date); - $query->addFrom(array($archiveTable => "INNER JOIN \"$archiveTable\" - ON \"$archiveTable\".\"ID\" = \"{$baseTable}_versions\".\"RecordID\" - AND \"$archiveTable\".\"Version\" = \"{$baseTable}_versions\".\"Version\"")); + $safeDate = Convert::raw2sql($date); + $query->addWhere( + "`{$baseTable}_versions`.`Version` IN + (SELECT LatestVersion FROM + (SELECT + `{$baseTable}_versions`.`RecordID`, + MAX(`{$baseTable}_versions`.`Version`) AS LatestVersion + FROM `{$baseTable}_versions` + WHERE `{$baseTable}_versions`.`LastEdited` <= '$safeDate' + GROUP BY `{$baseTable}_versions`.`RecordID` + ) AS `{$baseTable}_versions_latest` + WHERE `{$baseTable}_versions_latest`.`RecordID` = `{$baseTable}_versions`.`RecordID` + )"); break; // Reading a specific stage (Stage or Live) @@ -204,10 +213,18 @@ class Versioned extends DataExtension { // Return latest version instances, regardless of whether they are on a particular stage // This provides "show all, including deleted" functonality if($dataQuery->getQueryParam('Versioned.mode') == 'latest_versions') { - $archiveTable = self::requireArchiveTempTable($baseTable); - $query->addInnerJoin($archiveTable, "\"$archiveTable\".\"ID\" = \"{$baseTable}_versions\".\"RecordID\" AND \"$archiveTable\".\"Version\" = \"{$baseTable}_versions\".\"Version\""); + $query->addWhere( + "`{$alias}_versions`.`Version` IN + (SELECT LatestVersion FROM + (SELECT + `{$alias}_versions`.`RecordID`, + MAX(`{$alias}_versions`.`Version`) AS LatestVersion + FROM `{$alias}_versions` + GROUP BY `{$alias}_versions`.`RecordID` + ) AS `{$alias}_versions_latest` + WHERE `{$alias}_versions_latest`.`RecordID` = `{$alias}_versions`.`RecordID` + )"); } - break; default: throw new InvalidArgumentException("Bad value for query parameter Versioned.mode: " . $dataQuery->getQueryParam('Versioned.mode')); @@ -234,34 +251,6 @@ class Versioned extends DataExtension { // Remove references to them self::$archive_tables = array(); } - - /** - * Create a temporary table mapping each database record to its version on the given date. - * This is used by the versioning system to return database content on that date. - * @param string $baseTable The base table. - * @param string $date The date. If omitted, then the latest version of each page will be returned. - * @todo Ensure that this is DB abstracted - */ - protected static function requireArchiveTempTable($baseTable, $date = null) { - if(!isset(self::$archive_tables[$baseTable])) { - self::$archive_tables[$baseTable] = DB::createTable("_Archive$baseTable", array( - "ID" => "INT NOT NULL", - "Version" => "INT NOT NULL", - ), null, array('temporary' => true)); - } - - if(!DB::query("SELECT COUNT(*) FROM \"" . self::$archive_tables[$baseTable] . "\"")->value()) { - if($date) $dateClause = "WHERE \"LastEdited\" <= '$date'"; - else $dateClause = ""; - - DB::query("INSERT INTO \"" . self::$archive_tables[$baseTable] . "\" - SELECT \"RecordID\", max(\"Version\") FROM \"{$baseTable}_versions\" - $dateClause - GROUP BY \"RecordID\""); - } - - return self::$archive_tables[$baseTable]; - } /** * An array of DataObject extensions that may require versioning for extra tables diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index ef871f64e..de9ced67a 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -266,6 +266,59 @@ class VersionedTest extends SapphireTest { $this->assertInstanceOf("VersionedTest_DataObject", $obj3); } + + public function testArchiveVersion() { + + // In 2005 this file was created + SS_Datetime::set_mock_now('2005-01-01 00:00:00'); + $testPage = new VersionedTest_Subclass(); + $testPage->Title = 'Archived page'; + $testPage->Content = 'This is the content from 2005'; + $testPage->ExtraField = '2005'; + $testPage->write(); + $testPage->publish('Stage', 'Live'); + + // In 2007 we updated it + SS_Datetime::set_mock_now('2007-01-01 00:00:00'); + $testPage->Content = "It's 2007 already!"; + $testPage->ExtraField = '2007'; + $testPage->write(); + $testPage->publish('Stage', 'Live'); + + // In 2009 we updated it again + SS_Datetime::set_mock_now('2009-01-01 00:00:00'); + $testPage->Content = "I'm enjoying 2009"; + $testPage->ExtraField = '2009'; + $testPage->write(); + $testPage->publish('Stage', 'Live'); + + // End mock, back to the present day:) + SS_Datetime::clear_mock_now(); + + // Test 1 - 2006 Content + singleton('VersionedTest_Subclass')->flushCache(true); + Versioned::set_reading_mode('Archive.2006-01-01 00:00:00'); + $testPage2006 = DataObject::get('VersionedTest_Subclass')->filter(array('Title' => 'Archived page'))->first(); + $this->assertInstanceOf("VersionedTest_Subclass", $testPage2006); + $this->assertEquals("2005", $testPage2006->ExtraField); + $this->assertEquals("This is the content from 2005", $testPage2006->Content); + + // Test 2 - 2008 Content + singleton('VersionedTest_Subclass')->flushCache(true); + Versioned::set_reading_mode('Archive.2008-01-01 00:00:00'); + $testPage2008 = DataObject::get('VersionedTest_Subclass')->filter(array('Title' => 'Archived page'))->first(); + $this->assertInstanceOf("VersionedTest_Subclass", $testPage2008); + $this->assertEquals("2007", $testPage2008->ExtraField); + $this->assertEquals("It's 2007 already!", $testPage2008->Content); + + // Test 3 - Today + singleton('VersionedTest_Subclass')->flushCache(true); + Versioned::set_reading_mode('Stage.Stage'); + $testPageCurrent = DataObject::get('VersionedTest_Subclass')->filter(array('Title' => 'Archived page'))->first(); + $this->assertInstanceOf("VersionedTest_Subclass", $testPageCurrent); + $this->assertEquals("2009", $testPageCurrent->ExtraField); + $this->assertEquals("I'm enjoying 2009", $testPageCurrent->Content); + } } class VersionedTest_DataObject extends DataObject implements TestOnly { From 56fe7f881a71ad0d8582d16bdd116c408199700e Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 20 Aug 2012 13:44:56 +1200 Subject: [PATCH 4/5] REMOVED: Unnecessary publish actions from test cases ADDED: Test case for get_all_versions --- tests/model/VersionedTest.php | 42 ++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index de9ced67a..9da327f3e 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -276,21 +276,18 @@ class VersionedTest extends SapphireTest { $testPage->Content = 'This is the content from 2005'; $testPage->ExtraField = '2005'; $testPage->write(); - $testPage->publish('Stage', 'Live'); // In 2007 we updated it SS_Datetime::set_mock_now('2007-01-01 00:00:00'); $testPage->Content = "It's 2007 already!"; $testPage->ExtraField = '2007'; $testPage->write(); - $testPage->publish('Stage', 'Live'); // In 2009 we updated it again SS_Datetime::set_mock_now('2009-01-01 00:00:00'); $testPage->Content = "I'm enjoying 2009"; $testPage->ExtraField = '2009'; $testPage->write(); - $testPage->publish('Stage', 'Live'); // End mock, back to the present day:) SS_Datetime::clear_mock_now(); @@ -319,6 +316,45 @@ class VersionedTest extends SapphireTest { $this->assertEquals("2009", $testPageCurrent->ExtraField); $this->assertEquals("I'm enjoying 2009", $testPageCurrent->Content); } + + public function testAllVersions() + { + // In 2005 this file was created + SS_Datetime::set_mock_now('2005-01-01 00:00:00'); + $testPage = new VersionedTest_Subclass(); + $testPage->Title = 'Archived page'; + $testPage->Content = 'This is the content from 2005'; + $testPage->ExtraField = '2005'; + $testPage->write(); + + // In 2007 we updated it + SS_Datetime::set_mock_now('2007-01-01 00:00:00'); + $testPage->Content = "It's 2007 already!"; + $testPage->ExtraField = '2007'; + $testPage->write(); + + // In 2009 we updated it again + SS_Datetime::set_mock_now('2009-01-01 00:00:00'); + $testPage->Content = "I'm enjoying 2009"; + $testPage->ExtraField = '2009'; + $testPage->write(); + + // End mock, back to the present day:) + SS_Datetime::clear_mock_now(); + + $versions = Versioned::get_all_versions('VersionedTest_Subclass', $testPage->ID); + $content = array(); + $extraFields = array(); + foreach($versions as $version) + { + $content[] = $version->Content; + $extraFields[] = $version->ExtraField; + } + + $this->assertEquals($versions->Count(), 3, 'All versions returned'); + $this->assertEquals($content, array('This is the content from 2005', "It's 2007 already!", "I'm enjoying 2009"), 'Version fields returned'); + $this->assertEquals($extraFields, array('2005', '2007', '2009'), 'Version fields returned'); + } } class VersionedTest_DataObject extends DataObject implements TestOnly { From 89728acece2611574c39516994022bf478e17b1a Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 20 Aug 2012 13:48:48 +1200 Subject: [PATCH 5/5] UPDATED: Improved get_all_versions test case to test versions in the middle of version updates. --- tests/model/VersionedTest.php | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 9da327f3e..eb9ed65e7 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -333,6 +333,20 @@ class VersionedTest extends SapphireTest { $testPage->ExtraField = '2007'; $testPage->write(); + // Check both versions are returned + $versions = Versioned::get_all_versions('VersionedTest_Subclass', $testPage->ID); + $content = array(); + $extraFields = array(); + foreach($versions as $version) + { + $content[] = $version->Content; + $extraFields[] = $version->ExtraField; + } + + $this->assertEquals($versions->Count(), 2, 'All versions returned'); + $this->assertEquals($content, array('This is the content from 2005', "It's 2007 already!"), 'Version fields returned'); + $this->assertEquals($extraFields, array('2005', '2007'), 'Version fields returned'); + // In 2009 we updated it again SS_Datetime::set_mock_now('2009-01-01 00:00:00'); $testPage->Content = "I'm enjoying 2009"; @@ -351,9 +365,9 @@ class VersionedTest extends SapphireTest { $extraFields[] = $version->ExtraField; } - $this->assertEquals($versions->Count(), 3, 'All versions returned'); - $this->assertEquals($content, array('This is the content from 2005', "It's 2007 already!", "I'm enjoying 2009"), 'Version fields returned'); - $this->assertEquals($extraFields, array('2005', '2007', '2009'), 'Version fields returned'); + $this->assertEquals($versions->Count(), 3, 'Additional all versions returned'); + $this->assertEquals($content, array('This is the content from 2005', "It's 2007 already!", "I'm enjoying 2009"), 'Additional version fields returned'); + $this->assertEquals($extraFields, array('2005', '2007', '2009'), 'Additional version fields returned'); } }