Merge pull request #1043 from tractorcow/3.1-deleted-pages-fixes

Fix for issue #7975 - Issue with retrieving deleted pages

model/DataQuery.php

@@ -647,12 +647,12 @@ class DataQuery {
 	 * @param string $field 
 	 */
 	public function subtract(DataQuery $subtractQuery, $field='ID') {
-		$subSelect= $subtractQuery->getFinalisedQuery();
+		$fieldExpression = $subtractQuery->expressionForField($field);
-		$fieldExpression = $this->expressionForField($field, $subSelect);
+		$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 The expression used to query this field via this DataQuery
-	 * @return String
 	 */
-	protected function expressionForField($field, $query) {
+	protected function expressionForField($field) {
-		// Special case for ID
+		
-		if($field == 'ID') {
+		// 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);
 		}
 	}
 

model/Versioned.php

@@ -136,7 +136,7 @@ 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
@@ -203,6 +203,7 @@ class Versioned extends DataExtension {
 			// below)
 			$dataQuery->setQueryParam('Versioned.mode', 'stage');
 			$this->augmentSQL($query, $dataQuery);
+			$dataQuery->setQueryParam('Versioned.mode', 'stage_unique');
 
 			// Now exclude any ID from any other stage. Note that we double rename to avoid the regular stage rename
 			// renaming all subquery references to be Versioned.stage
@@ -232,8 +233,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 +262,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 +281,8 @@ class Versioned extends DataExtension {
 	 */
 	function augmentLoadLazyFields(SQLQuery &$query, DataQuery &$dataQuery = null, $record) {
 		$dataClass = $dataQuery->dataClass();
-		if (isset($record['Version'])){
+	    if (isset($record['Version'])){
-			$dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']);
+	    	$dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']);
 			$dataQuery->where("\"$dataClass\".\"Version\" = " . $record['Version']);
 			$dataQuery->setQueryParam('Versioned.mode', 'all_versions');
 		}

tests/model/VersionedTest.php

@@ -74,8 +74,15 @@ class VersionedTest extends SapphireTest {
 	 * Test Versioned::get_including_deleted()
 	 */
 	public function testGetIncludingDeleted() {
-		// Delete a page
+		// Get all ids of pages
-		$this->objFromFixture('VersionedTest_DataObject', 'page3')->delete();
+		$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() {