BUG Sort column order maintained correctly when using expressions in …

…SQLQuery and DataQuery
silverstripe · Oct 3, 2013 · afaf7f6 · afaf7f6
1 parent 6e72262
commit afaf7f6
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 10 deletions.
diff --git a/model/DataQuery.php b/model/DataQuery.php
@@ -256,7 +256,10 @@ protected function ensureSelectContainsOrderbyColumns($query, $originalSelect =
 		$baseClass = array_shift($tableClasses);
 
 		if($orderby = $query->getOrderBy()) {
+			$newOrderby = array();
 			foreach($orderby as $k => $dir) {
+				$newOrderby[$k] = $dir;
+
 				// don't touch functions in the ORDER BY or public function calls
 				// selected as fields
 				if(strpos($k, '(') !== false) continue;
@@ -283,10 +286,10 @@ protected function ensureSelectContainsOrderbyColumns($query, $originalSelect =
 						$qualCol = "\"$parts[0]\"";
 					}
 
-						// remove original sort
-						unset($orderby[$k]);
-						// add new columns sort
-						$orderby[$qualCol] = $dir;
+					// remove original sort
+					unset($newOrderby[$k]);
+					// add new columns sort
+					$newOrderby[$qualCol] = $dir;
 
 					// To-do: Remove this if block once SQLQuery::$select has been refactored to store getSelect()
 					// format internally; then this check can be part of selectField()
@@ -305,7 +308,7 @@ protected function ensureSelectContainsOrderbyColumns($query, $originalSelect =
 				}
 			}
 
-			$query->setOrderBy($orderby);
+			$query->setOrderBy($newOrderby);
 		}
 	}
 

diff --git a/model/SQLQuery.php b/model/SQLQuery.php
@@ -519,7 +519,7 @@ public function setOrderBy($clauses = null, $direction = null) {
 	 * @example $sql->orderby("Column", "DESC");
 	 * @example $sql->orderby(array("Column" => "ASC", "ColumnTwo" => "DESC"));
 	 *
-	 * @param string|array $orderby Clauses to add (escaped SQL statements)
+	 * @param string|array $clauses Clauses to add (escaped SQL statements)
 	 * @param string $dir Sort direction, ASC or DESC
 	 *
 	 * @return SQLQuery
@@ -566,21 +566,23 @@ public function addOrderBy($clauses = null, $direction = null) {
 		// directly in the ORDER BY
 		if($this->orderby) {
 			$i = 0;
+			$orderby = array();
 			foreach($this->orderby as $clause => $dir) {
 
 				// public function calls and multi-word columns like "CASE WHEN ..."
 				if(strpos($clause, '(') !== false || strpos($clause, " ") !== false ) {
-					// remove the old orderby
-					unset($this->orderby[$clause]);
 
+					// Move the clause to the select fragment, substituting a placeholder column in the sort fragment.
 					$clause = trim($clause);
 					$column = "_SortColumn{$i}";
-
 					$this->selectField($clause, $column);
-					$this->addOrderBy('"' . $column . '"', $dir);
+					$clause = '"' . $column . '"';
 					$i++;
 				}
+
+				$orderby[$clause] = $dir;
 			}
+			$this->orderby = $orderby;
 		}
 
 		return $this;

diff --git a/tests/model/DataQueryTest.php b/tests/model/DataQueryTest.php
@@ -115,6 +115,15 @@ public function testSubgroupHandoff() {
 
 		$this->assertEquals($dq->sql(), $orgDq->sql());
 	}
+
+	public function testOrderByMultiple() {
+		$dq = new DataQuery('SQLQueryTest_DO');
+		$dq = $dq->sort('"Name" ASC, MID("Name", 8, 1) DESC');
+		$this->assertContains(
+			'ORDER BY "Name" ASC, "_SortColumn0" DESC',
+			$dq->sql()
+		);
+	}
 }
 
 

diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php
@@ -479,6 +479,31 @@ public function testOrderByContainingAggregateAndLimitOffset() {
 		$this->assertEquals('Object 2', $records[0]['Name']);
 		$this->assertEquals('2012-05-01 09:00:00', $records['0']['_SortColumn0']);
 	}
+
+	/**
+	 * Test that multiple order elements are maintained in the given order
+	 */
+	public function testOrderByMultiple() {
+		if(DB::getConn() instanceof MySQLDatabase) {
+			$query = new SQLQuery();
+			$query->setSelect(array('"Name"', '"Meta"'));
+			$query->setFrom('"SQLQueryTest_DO"');
+			$query->setOrderBy(array('MID("Name", 8, 1) DESC', '"Name" ASC'));
+
+			$records = array();
+			foreach($query->execute() as $record) {
+				$records[] = $record;
+			}
+
+			$this->assertCount(2, $records);
+
+			$this->assertEquals('Object 2', $records[0]['Name']);
+			$this->assertEquals('2', $records[0]['_SortColumn0']);
+
+			$this->assertEquals('Object 1', $records[1]['Name']);
+			$this->assertEquals('1', $records[1]['_SortColumn0']);
+		}
+	}
 
 	/**
 	 * Test passing in a LIMIT with OFFSET clause string.