diff --git a/model/DataList.php b/model/DataList.php index 0715a8176..1c6f16bab 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -189,7 +189,13 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab $this->dataQuery->sort(null, null); // wipe the sort foreach(func_get_arg(0) as $col => $dir) { - $this->dataQuery->sort($this->getRelationName($col), $dir, false); + // Convert column expressions to SQL fragment, while still allowing the passing of raw SQL fragments. + try { + $relCol = $this->getRelationName($col); + } catch(InvalidArgumentException $e) { + $relCol = $col; + } + $this->dataQuery->sort($relCol, $dir, false); } } @@ -250,12 +256,16 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab /** * Translates a Object relation name to a Database name and apply the relation join to - * the query + * the query. Throws an InvalidArgumentException if the $field doesn't correspond to a relation * * @param string $field * @return string */ public function getRelationName($field) { + if(!preg_match('/^[A-Z0-9._]+$/i', $field)) { + throw new InvalidArgumentException("Bad field expression $field"); + } + if(strpos($field,'.') === false) { return '"'.$field.'"'; } diff --git a/model/DataQuery.php b/model/DataQuery.php index f607a7840..1759c60e4 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -229,7 +229,7 @@ class DataQuery { * @param SQLQuery $query * @return null */ - protected function ensureSelectContainsOrderbyColumns($query) { + protected function ensureSelectContainsOrderbyColumns($query, $originalSelect = array()) { $tableClasses = ClassInfo::dataClassesFor($this->dataClass); $baseClass = array_shift($tableClasses); @@ -239,8 +239,13 @@ class DataQuery { foreach($orderby as $k => $dir) { // don't touch functions in the ORDER BY or function calls // selected as fields - if(strpos($k, '(') !== false || preg_match('/_SortColumn/', $k)) + if(strpos($k, '(') !== false) continue; + + // Pull through SortColumn references from the originalSelect variables + if(preg_match('/_SortColumn/', $k)) { + if(isset($originalSelect[$k])) $query->selectField($originalSelect[$k], $k); continue; + } $col = str_replace('"', '', trim($k)); $parts = explode('.', $col); @@ -616,8 +621,11 @@ class DataQuery { */ public function column($field = 'ID') { $query = $this->getFinalisedQuery(array($field)); - $query->select($this->expressionForField($field, $query)); - $this->ensureSelectContainsOrderbyColumns($query); + $originalSelect = $query->itemisedSelect(); + $fieldExpression = $this->expressionForField($field, $query); + $query->clearSelect(); + $query->selectField($fieldExpression); + $this->ensureSelectContainsOrderbyColumns($query, $originalSelect); return $query->execute()->column($field); } diff --git a/model/SQLQuery.php b/model/SQLQuery.php index 47dc9a660..6ce1ad459 100644 --- a/model/SQLQuery.php +++ b/model/SQLQuery.php @@ -343,13 +343,12 @@ class SQLQuery { else { $sort = explode(",", $clauses); } - + $clauses = array(); foreach($sort as $clause) { - $clause = explode(" ", trim($clause)); - $dir = (isset($clause[1])) ? $clause[1] : $direction; - $clauses[$clause[0]] = $dir; + list($column, $direction) = $this->getDirectionFromString($clause, $direction); + $clauses[$column] = $direction; } } @@ -357,16 +356,13 @@ class SQLQuery { foreach($clauses as $key => $value) { if(!is_numeric($key)) { $column = trim($key); - $direction = strtoupper(trim($value)); + $columnDir = strtoupper(trim($value)); } else { - $clause = explode(" ", trim($value)); - - $column = trim($clause[0]); - $direction = (isset($clause[1])) ? strtoupper(trim($clause[1])) : ""; + list($column, $columnDir) = $this->getDirectionFromString($value); } - $this->orderby[$column] = $direction; + $this->orderby[$column] = $columnDir; } } else { @@ -380,9 +376,9 @@ class SQLQuery { // directly in the ORDER BY if($this->orderby) { $i = 0; - foreach($this->orderby as $clause => $dir) { - if(strpos($clause, '(') !== false) { + // Function calls and multi-word columns like "CASE WHEN ..." + if(strpos($clause, '(') !== false || strpos($clause, " ") !== false ) { // remove the old orderby unset($this->orderby[$clause]); @@ -399,6 +395,22 @@ class SQLQuery { return $this; } + /** + * Extract the direction part of a single-column order by clause. + * + * Return a 2 element array: array($column, $direction) + */ + private function getDirectionFromString($value, $defaultDirection = null) { + if(preg_match('/^(.*)(asc|desc)$/i', $value, $matches)) { + $column = trim($matches[1]); + $direction = strtoupper($matches[2]); + } else { + $column = $value; + $direction = $defaultDirection ? $defaultDirection : "ASC"; + } + return array($column, $direction); + } + /** * Returns the current order by as array if not already. To handle legacy * statements which are stored as strings. Without clauses and directions, diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index 802b883e2..45ce64531 100755 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -515,4 +515,18 @@ class DataListTest extends SapphireTest { $this->assertEquals('Bob', $list->last()->Name, 'Last comment should be from Bob'); $this->assertEquals('Phil', $list->first()->Name, 'First comment should be from Phil'); } + + public function testSortByComplexExpression() { + // Test an expression with both spaces and commas + // This test also tests that column() can be called with a complex sort expression, so keep using column() below + $list = DataObjectTest_Team::get()->sort('CASE WHEN "DataObjectTest_Team"."ClassName" = \'DataObjectTest_SubTeam\' THEN 0 ELSE 1 END, "Title" DESC'); + $this->assertEquals(array( + 'Subteam 3', + 'Subteam 2', + 'Subteam 1', + 'Team 3', + 'Team 2', + 'Team 1', + ), $list->column("Title")); + } } diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php index 3591f95af..3c4018b15 100755 --- a/tests/model/SQLQueryTest.php +++ b/tests/model/SQLQueryTest.php @@ -102,7 +102,7 @@ class SQLQueryTest extends SapphireTest { $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby('MyName'); - $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName', $query->sql()); + $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC', $query->sql()); $query = new SQLQuery(); $query->from[] = "MyTable"; @@ -117,7 +117,7 @@ class SQLQueryTest extends SapphireTest { $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby('MyName ASC, Color'); - $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC, Color', $query->sql()); + $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC, Color ASC', $query->sql()); $query = new SQLQuery(); $query->from[] = "MyTable"; @@ -127,23 +127,23 @@ class SQLQueryTest extends SapphireTest { $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby(array('MyName' => 'desc', 'Color')); - $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName DESC, Color', $query->sql()); + $this->assertEquals('SELECT * FROM MyTable ORDER BY MyName DESC, Color ASC', $query->sql()); $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby('implode("MyName","Color")'); - $this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0', $query->sql()); + $this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC', $query->sql()); $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby('implode("MyName","Color") DESC'); - $this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 DESC', $query->sql()); + $this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 DESC', $query->sql()); $query = new SQLQuery(); $query->from[] = "MyTable"; $query->orderby('RAND()'); - $this->assertEquals('SELECT RAND() AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0', $query->sql()); + $this->assertEquals('SELECT *, RAND() AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC', $query->sql()); } public function testReverseOrderBy() { @@ -174,7 +174,7 @@ class SQLQueryTest extends SapphireTest { $query->orderby('implode("MyName","Color") DESC'); $query->reverseOrderBy(); - $this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC',$query->sql()); + $this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC',$query->sql()); } function testFiltersOnID() {