From 6aa16e1f01d884cbce408e5caa6df9d3c3b5ddce Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 17 Dec 2012 17:15:26 +1300 Subject: [PATCH] BUG Test case for versioned now correctly checks IDs returned from Versioned::get_including_deleted BUG Issue with deleted records not being queried properly. API DataQuery::expressionForField no longer requires a second parameter. Rather the query object is inferred from the DataQuery itself. This should improve consistency of use of this function. --- model/DataQuery.php | 30 +++++++++++++++++------------- model/Versioned.php | 24 +++++++++++++++++++----- tests/model/VersionedTest.php | 16 ++++++++++++++-- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/model/DataQuery.php b/model/DataQuery.php index 16d3a1276..bc0382423 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -647,12 +647,12 @@ class DataQuery { * @param string $field */ public function subtract(DataQuery $subtractQuery, $field='ID') { - $subSelect= $subtractQuery->getFinalisedQuery(); - $fieldExpression = $this->expressionForField($field, $subSelect); + $fieldExpression = $subtractQuery->expressionForField($field); + $subSelect = $subtractQuery->getFinalisedQuery(); $subSelect->setSelect(array()); $subSelect->selectField($fieldExpression, $field); $subSelect->setOrderBy(null); - $this->where($this->expressionForField($field, $this).' NOT IN ('.$subSelect->sql().')'); + $this->where($this->expressionForField($field).' NOT IN ('.$subSelect->sql().')'); return $this; } @@ -679,9 +679,9 @@ class DataQuery { * @param String $field See {@link expressionForField()}. */ public function column($field = 'ID') { + $fieldExpression = $this->expressionForField($field); $query = $this->getFinalisedQuery(array($field)); $originalSelect = $query->getSelect(); - $fieldExpression = $this->expressionForField($field, $query); $query->setSelect(array()); $query->selectField($fieldExpression, $field); $this->ensureSelectContainsOrderbyColumns($query, $originalSelect); @@ -692,17 +692,21 @@ class DataQuery { /** * @param String $field Select statement identifier, either the unquoted column name, * the full composite SQL statement, or the alias set through {@link SQLQuery->selectField()}. - * @param SQLQuery $query - * @return String + * @return String The expression used to query this field via this DataQuery */ - protected function expressionForField($field, $query) { - // Special case for ID - if($field == 'ID') { + protected function expressionForField($field) { + + // Prepare query object for selecting this field + $query = $this->getFinalisedQuery(array($field)); + + // Allow query to define the expression for this field + $expression = $query->expressionForField($field); + if(!empty($expression)) return $expression; + + // Special case for ID, if not provided + if($field === 'ID') { $baseClass = ClassInfo::baseDataClass($this->dataClass); - return "\"$baseClass\".\"ID\""; - - } else { - return $query->expressionForField($field); + return "\"$baseClass\".\"ID\""; } } diff --git a/model/Versioned.php b/model/Versioned.php index 560358475..7fab016e5 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -136,8 +136,8 @@ class Versioned extends DataExtension { * @todo Should this all go into VersionedDataQuery? */ public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { - $baseTable = ClassInfo::baseDataClass($dataQuery->dataClass()); - + $baseTable = ClassInfo::baseDataClass($dataQuery->dataClass()); + switch($dataQuery->getQueryParam('Versioned.mode')) { // Noop case '': @@ -232,8 +232,19 @@ class Versioned extends DataExtension { foreach(self::$db_for_versions_table as $name => $type) { $query->selectField(sprintf('"%s_versions"."%s"', $baseTable, $name), $name); } + + // Alias the record ID as the row ID $query->selectField(sprintf('"%s_versions"."%s"', $baseTable, 'RecordID'), "ID"); - $query->addOrderBy(sprintf('"%s_versions"."%s"', $baseTable, 'Version')); + + // Ensure that any sort order referring to this ID is correctly aliased + $orders = $query->getOrderBy(); + foreach($orders as $order => $dir) { + if($order === "\"$baseTable\".\"ID\"") { + unset($orders[$order]); + $orders["\"{$baseTable}_versions\".\"RecordID\""] = $dir; + } + } + $query->setOrderBy($orders); // latest_version has one more step // Return latest version instances, regardless of whether they are on a particular stage @@ -250,6 +261,9 @@ class Versioned extends DataExtension { ) AS \"{$alias}_versions_latest\" WHERE \"{$alias}_versions_latest\".\"RecordID\" = \"{$alias}_versions\".\"RecordID\" )"); + } else { + // If all versions are requested, ensure that records are sorted by this field + $query->addOrderBy(sprintf('"%s_versions"."%s"', $baseTable, 'Version')); } break; default: @@ -266,8 +280,8 @@ class Versioned extends DataExtension { */ function augmentLoadLazyFields(SQLQuery &$query, DataQuery &$dataQuery = null, $record) { $dataClass = $dataQuery->dataClass(); - if (isset($record['Version'])){ - $dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']); + if (isset($record['Version'])){ + $dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']); $dataQuery->where("\"$dataClass\".\"Version\" = " . $record['Version']); $dataQuery->setQueryParam('Versioned.mode', 'all_versions'); } diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 6c1aec3b9..a671adc77 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -74,8 +74,15 @@ class VersionedTest extends SapphireTest { * Test Versioned::get_including_deleted() */ public function testGetIncludingDeleted() { - // Delete a page - $this->objFromFixture('VersionedTest_DataObject', 'page3')->delete(); + // Get all ids of pages + $allPageIDs = DataObject::get('VersionedTest_DataObject', "\"ParentID\" = 0", "\"VersionedTest_DataObject\".\"ID\" ASC")->column('ID'); + + // Modify a page, ensuring that the Version ID and Record ID will differ, + // and then subsequently delete it + $targetPage = $this->objFromFixture('VersionedTest_DataObject', 'page3'); + $targetPage->Content = 'To be deleted'; + $targetPage->write(); + $targetPage->delete(); // Get all items, ignoring deleted $remainingPages = DataObject::get("VersionedTest_DataObject", "\"ParentID\" = 0", @@ -90,12 +97,17 @@ class VersionedTest extends SapphireTest { // Check that page 3 is still there $this->assertEquals(array("Page 1", "Page 2", "Page 3"), $allPages->column('Title')); + // Check that the returned pages have the correct IDs + $this->assertEquals($allPageIDs, $allPages->column('ID')); + // Check that this still works if we switch to reading the other stage 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')); + // Check that the returned pages still have the correct IDs + $this->assertEquals($allPageIDs, $allPages->column('ID')); } public function testVersionedFieldsAdded() {