API: Queries added by DataList::addInnerJoin() and DataList::leftJoin() come after the base joins, not before.

This bug will surface when using the ORM and adding an join to DataList
where a DataObject inherits another DataObject.

If you for example want to restrict the number of pages that only have a
related Staff object:

    $list = DataList::create('Page')
		->InnerJoin('Staff', '"Staff"."ID" = "Page"."StaffID");

This will create a SQL query where the INNER JOIN is before the
LEFT JOIN of Page and SiteTree in the resulting SQL string. In MySQL
and PostgreSQL this will create an invalid query.

This patch solves the problem by sorting the joins.

model/DataQuery.php

@@ -190,7 +190,7 @@ class DataQuery {
 			}
 
 			if ($joinTable) {
-				$query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"") ;
+				$query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"", $tableClass, 10) ;
 			}
 		}
 		

model/SQLQuery.php

@@ -276,18 +276,28 @@ class SQLQuery {
 	 * Add a LEFT JOIN criteria to the FROM clause.
 	 *
 	 * @param string $table Unquoted table name
-	 * @param string $onPredicate The "ON" SQL fragment in a "LEFT JOIN ... AS ... ON ..." statement,
+	 * @param string $onPredicate The "ON" SQL fragment in a "LEFT JOIN ... AS ... ON ..." statement, Needs to be valid 
-	 *  Needs to be valid (quoted) SQL.
+	 *                            (quoted) SQL.
 	 * @param string $tableAlias Optional alias which makes it easier to identify and replace joins later on
+	 * @param int $order A numerical index to control the order that joins are added to the query; lower order values 
+	 *                   will cause the query to appear first. The default is 20, and joins created automatically by the
+	 *                   ORM have a value of 10.
 	 * @return SQLQuery
 	 */
-	public function addLeftJoin($table, $onPredicate, $tableAlias = null) {
+	public function addLeftJoin($table, $onPredicate, $tableAlias = '', $order = 20) {
-		if(!$tableAlias) $tableAlias = $table;
+		if(!$tableAlias) {
-		$this->from[$tableAlias] = array('type' => 'LEFT', 'table' => $table, 'filter' => array($onPredicate));
+			$tableAlias = $table;
+		}
+		$this->from[$tableAlias] = array(
+			'type' => 'LEFT',
+			'table' => $table,
+			'filter' => array($onPredicate), 
+			'order' => $order
+		);
 		return $this;
 	}
 
-	public function leftjoin($table, $onPredicate, $tableAlias = null) {
+	public function leftjoin($table, $onPredicate, $tableAlias = null, $order = 20) {
 		Deprecation::notice('3.0', 'Please use addLeftJoin() instead!');
 		$this->addLeftJoin($table, $onPredicate, $tableAlias);
 	}
@@ -296,20 +306,28 @@ class SQLQuery {
 	 * Add an INNER JOIN criteria to the FROM clause.
 	 *
 	 * @param string $table Unquoted table name
-	 * @param string $onPredicate The "ON" SQL fragment in an "INNER JOIN ... AS ... ON ..." statement.
+	 * @param string $onPredicate The "ON" SQL fragment in an "INNER JOIN ... AS ... ON ..." statement. Needs to be 
-	 *  Needs to be valid (quoted) SQL.
+	 *                            valid (quoted) SQL.
 	 * @param string $tableAlias Optional alias which makes it easier to identify and replace joins later on
+	 * @param int $order A numerical index to control the order that joins are added to the query; lower order values 
+	 *                   will cause the query to appear first. The default is 20, and joins created automatically by the
+	 *                   ORM have a value of 10.
 	 * @return SQLQuery
 	 */
-	public function addInnerJoin($table, $onPredicate, $tableAlias = null) {
+	public function addInnerJoin($table, $onPredicate, $tableAlias = null, $order = 20) {
 		if(!$tableAlias) $tableAlias = $table;
-		$this->from[$tableAlias] = array('type' => 'INNER', 'table' => $table, 'filter' => array($onPredicate));
+		$this->from[$tableAlias] = array(
+			'type' => 'INNER',
+			'table' => $table,
+			'filter' => array($onPredicate),
+			'order' => $order
+		);
 		return $this;
 	}
 
-	public function innerjoin($table, $onPredicate, $tableAlias = null) {
+	public function innerjoin($table, $onPredicate, $tableAlias = null, $order = 20) {
 		Deprecation::notice('3.0', 'Please use addInnerJoin() instead!');
-		return $this->addInnerJoin($table, $onPredicate, $tableAlias);
+		return $this->addInnerJoin($table, $onPredicate, $tableAlias, $order);
 	}
 
 	/**
@@ -876,12 +894,13 @@ class SQLQuery {
 		// TODO: Don't require this internal-state manipulate-and-preserve - let sqlQueryToString() handle the new
 		// syntax
 		$origFrom = $this->from;
-
+		// Sort the joins
+		$this->from = $this->getOrderedJoins($this->from);
 		// Build from clauses
 		foreach($this->from as $alias => $join) {
 			// $join can be something like this array structure
 			// array('type' => 'inner', 'table' => 'SiteTree', 'filter' => array("SiteTree.ID = 1", 
-			// "Status = 'approved'"))
+			// "Status = 'approved'", 'order' => 20))
 			if(is_array($join)) {
 				if(is_string($join['filter'])) $filter = $join['filter'];
 				else if(sizeof($join['filter']) == 1) $filter = $join['filter'][0];
@@ -902,10 +921,9 @@ class SQLQuery {
 		$this->from = $origFrom;
 
 		// The query was most likely just created and then exectued.
-		if($sql === 'SELECT *') {
+		if(trim($sql) === 'SELECT * FROM') {
 			return '';
 		}
-
 		return $sql;
 	}
 	
@@ -1087,5 +1105,80 @@ class SQLQuery {
 		return $query;
 	}
 	
+	/**
+	 * Ensure that framework "auto-generated" table JOINs are first in the finalised SQL query.
+	 * This prevents issues where developer-initiated JOINs attempt to JOIN using relations that haven't actually 
+	 * yet been scaffolded by the framework. Demonstrated by PostGres in errors like: 
+	 *"...ERROR: missing FROM-clause..."
+	 * 
+	 * @param $from array - in the format of $this->select
+	 * @return array - and reorderded list of selects
+	 */
+	protected function getOrderedJoins($from) {
+		// shift the first FROM table out from so we only deal with the JOINs
+		$baseFrom = array_shift($from);
+		$this->mergesort($from, function($firstJoin, $secondJoin) {
+			if($firstJoin['order'] == $secondJoin['order']) {
+				return 0;
+			}
+			return ($firstJoin['order'] < $secondJoin['order']) ?  -1 : 1;
+		});
+		
+		// Put the first FROM table back into the results 
+		array_unshift($from, $baseFrom);
+		return $from;
+	}
+
+	/**
+	 * Since uasort don't preserve the order of an array if the comparison is equal
+	 * we have to resort to a merge sort. It's quick and stable: O(n*log(n)).
+	 *
+	 * @see http://stackoverflow.com/q/4353739/139301
+	 * 
+	 * @param array &$array - the array to sort
+	 * @param callable $cmpFunction - the function to use for comparison
+	 */ 
+	protected function mergesort(&$array, $cmpFunction = 'strcmp') {
+		// Arrays of size < 2 require no action.
+		if (count($array) < 2) {
+			return;
+		}
+		// Split the array in half
+		$halfway = count($array) / 2;
+		$array1 = array_slice($array, 0, $halfway);
+		$array2 = array_slice($array, $halfway);
+		// Recurse to sort the two halves
+		$this->mergesort($array1, $cmpFunction);
+		$this->mergesort($array2, $cmpFunction);
+		// If all of $array1 is <= all of $array2, just append them.
+		if(call_user_func($cmpFunction, end($array1), reset($array2)) < 1) {
+			$array = array_merge($array1, $array2);
+			return;
+		}
+		// Merge the two sorted arrays into a single sorted array
+		$array = array();
+		$val1 = reset($array1);
+		$val2 = reset($array2);
+		do {
+			if (call_user_func($cmpFunction, $val1, $val2) < 1) {
+				$array[key($array1)] = $val1;
+				$val1 = next($array1);
+			} else {
+				$array[key($array2)] = $val2;
+				$val2 = next($array2);
+			}
+		} while($val1 && $val2);
+		
+		// Merge the remainder
+		while($val1) {
+			$array[key($array1)] = $val1;
+			$val1 = next($array1);
+		}
+		while($val2) {
+			$array[key($array2)] = $val2;
+			$val2 = next($array2);
+		}
+		return;
+	}
 }
 

tests/model/DataQueryTest.php

@@ -1,6 +1,13 @@
 <?php
 
 class DataQueryTest extends SapphireTest {
+
+	protected $extraDataObjects = array(
+		'DataQueryTest_A',
+		'DataQueryTest_B',
+		'DataQueryTest_D',
+	);
+
 	/**
 	 * Test the leftJoin() and innerJoin method of the DataQuery object
 	 */
@@ -32,6 +39,12 @@ class DataQueryTest extends SapphireTest {
 		$this->assertEquals('DataQueryTest_B', $dq->applyRelation('ManyTestBs'),
 			'DataQuery::applyRelation should return the name of the related object.');
 	}
+
+	public function testRelationOrderWithCustomJoin() {
+		$dataQuery = new DataQuery('DataQueryTest_B');
+		$dataQuery->innerJoin('DataQueryTest_D', '"DataQueryTest_D"."RelationID" = "DataQueryTest_B"."ID"');
+		$dataQuery->execute();
+	}
 }
 
 
@@ -56,6 +69,7 @@ class DataQueryTest_B extends DataQueryTest_A {
 }
 
 class DataQueryTest_C extends DataObject implements TestOnly {
+
 	public static $has_one = array(
 		'TestA' => 'DataQueryTest_A',
 		'TestB' => 'DataQueryTest_B',
@@ -71,3 +85,10 @@ class DataQueryTest_C extends DataObject implements TestOnly {
 		'ManyTestBs' => 'DataQueryTest_B',
 	);
 }
+
+class DataQueryTest_D extends DataObject implements TestOnly {
+
+	public static $has_one = array(
+		'Relation' => 'DataQueryTest_B',
+	);
+}

tests/model/SQLQueryTest.php

@@ -293,6 +293,7 @@ class SQLQueryTest extends SapphireTest {
 		);
 	}
 	
+
 	public function testSetWhereAny() {
 		$query = new SQLQuery();
 		$query->setFrom('MyTable');
@@ -376,4 +377,17 @@ class SQLQueryTest_DO extends DataObject implements TestOnly {
 	);
 }
 
+class SQLQueryTestBase extends DataObject implements TestOnly {
+	static $db = array(
+		"Title" => "Varchar",
+	);
+}
 
+class SQLQueryTestChild extends SQLQueryTestBase {
+	static $db = array(
+		"Name" => "Varchar",
+	);
+
+	static $has_one = array(
+	);
+}