Merge pull request #6731 from kinglozzer/manymanyextrafields-order

FIX: many_many_extraFields breaks _SortColumn0 ordering (fixes #6730)

model/DataQuery.php

@@ -763,14 +763,14 @@ class DataQuery {
 	}
 
 	/**
-	 * Select the given fields from the given table.
+	 * Select only the given fields from the given table.
 	 *
 	 * @param String $table Unquoted table name (will be escaped automatically)
 	 * @param Array $fields Database column names (will be escaped automatically)
 	 */
 	public function selectFromTable($table, $fields) {
 		$fieldExpressions = array_map(function($item) use($table) {
-			return "\"$table\".\"$item\"";
+			return Convert::symbol2sql("{$table}.{$item}");
 		}, $fields);
 
 		$this->query->setSelect($fieldExpressions);
@@ -778,6 +778,22 @@ class DataQuery {
 		return $this;
 	}
 
+	/**
+	 * Add the given fields from the given table to the select statement.
+	 *
+	 * @param String $table Unquoted table name (will be escaped automatically)
+	 * @param Array $fields Database column names (will be escaped automatically)
+	 */
+	public function addSelectFromTable($table, $fields) {
+		$fieldExpressions = array_map(function($item) use($table) {
+			return Convert::symbol2sql("{$table}.{$item}");
+		}, $fields);
+
+		$this->query->addSelect($fieldExpressions);
+
+		return $this;
+	}
+
 	/**
 	 * Query the given field column from the database and return as an array.
 	 *

model/ManyManyList.php

@@ -104,7 +104,7 @@ class ManyManyList extends RelationList {
 			}
 		}
 
-		$this->dataQuery->selectFromTable($this->joinTable, $finalized);
+		$this->dataQuery->addSelectFromTable($this->joinTable, $finalized);
 	}
 
 	/**
@@ -352,7 +352,7 @@ class ManyManyList extends RelationList {
 	 */
 	public function getExtraData($componentName, $itemID) {
 		$result = array();
-		
+
 		// Skip if no extrafields or unsaved record
 		if(empty($this->extraFields) || empty($itemID)) {
 			return $result;

tests/model/ManyManyListTest.php

@@ -11,6 +11,10 @@ class ManyManyListTest extends SapphireTest {
 	protected $extraDataObjects = array(
 		'DataObjectTest_Team',
 		'DataObjectTest_SubTeam',
+		'DataObjectTest_Fan',
+		'DataObjectTest_Sortable',
+		'DataObjectTest_EquipmentCompany',
+		'DataObjectTest_SubEquipmentCompany',
 		'DataObjectTest_Player',
 		'DataObjectTest_Company',
 		'DataObjectTest_TeamComment',
@@ -41,6 +45,28 @@ class ManyManyListTest extends SapphireTest {
 		$this->assertEquals(100, $check->Worth->getAmount());
 	}
 
+	/**
+	 * This test targets a bug where appending many_many_extraFields to a query would
+	 * result in erroneous queries for sort orders that rely on _SortColumn0
+	 */
+	public function testAddCompositedExtraFieldsWithSortColumn0() {
+		$obj = new ManyManyListTest_ExtraFields();
+		$obj->write();
+
+		$product = new ManyManyListTest_Product();
+		$product->Title = 'Test Product';
+		$product->write();
+
+		// the actual test is that this does not generate an error in the sql.
+		$obj->Products()->add($product, array(
+			'Reference' => 'Foo'
+		));
+
+		$result = $obj->Products()->First();
+		$this->assertEquals('Foo', $result->Reference, 'Basic scalar fields should exist');
+		$this->assertEquals('Test Product', $result->Title);
+	}
+
 	public function testCreateList() {
 		$list = ManyManyList::create('DataObjectTest_Team','DataObjectTest_Team_Players', 'DataObjectTest_TeamID',
 			'DataObjectTest_PlayerID');
@@ -290,7 +316,8 @@ class ManyManyListTest extends SapphireTest {
 class ManyManyListTest_ExtraFields extends DataObject implements TestOnly {
 
 	private static $many_many = array(
-		'Clients' => 'ManyManyListTest_ExtraFields'
+		'Clients' => 'ManyManyListTest_ExtraFields',
+		'Products' => 'ManyManyListTest_Product'
 	);
 
 	private static $belongs_many_many = array(
@@ -301,6 +328,9 @@ class ManyManyListTest_ExtraFields extends DataObject implements TestOnly {
 		'Clients' => array(
 			'Reference' => 'Varchar',
 			'Worth' => 'Money'
+		),
+		'Products' => array(
+			'Reference' => 'Varchar'
 		)
 	);
 }
@@ -320,6 +350,8 @@ class ManyManyListTest_Product extends DataObject implements TestOnly {
 		'Categories' => 'ManyManyListTest_Category'
 	);
 
+	private static $default_sort = '"Title" IS NOT NULL ASC, "Title" ASC';
+
 }
 
 class ManyManyListTest_Category extends DataObject implements TestOnly {