API Enable linear-only restriction for DataList::applyRelation

API Remove DataList::getRelation

docs/en/04_Changelogs/4.0.0.md

@@ -36,6 +36,10 @@
 
 ## Deprecated classes/methods
 
+### ORM
+
+ * `DataList::getRelation` is removed, as it was mutable. Use `DataList::applyRelation` instead, which is immutable.
+
 ### ErrorPage
 
  * `ErrorPage::get_filepath_for_errorcode` has been removed

model/DataList.php

@@ -315,7 +315,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 				if(stristr($sort, ' asc') || stristr($sort, ' desc')) {
 					$query->sort($sort);
 				} else {
-					$list->applyRelation($sort, $column);
+					$list->applyRelation($sort, $column, true);
 					$query->sort($column, 'ASC');
 				}
 			}
@@ -327,7 +327,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 				foreach($sort as $column => $direction) {
 					// Convert column expressions to SQL fragment, while still allowing the passing of raw SQL
 					// fragments.
-					$list->applyRelation($column, $relationColumn);
+					$list->applyRelation($column, $relationColumn, true);
 					$query->sort($relationColumn, $direction, false);
 				}
 			}
@@ -480,9 +480,11 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 *
 	 * @param string $field Name of field or relation to apply
 	 * @param string &$columnName Quoted column name
+	 * @param bool $linearOnly Set to true to restrict to linear relations only. Set this
+	 * if this relation will be used for sorting, and should not include duplicate rows.
 	 * @return DataList DataList with this relation applied
 	 */
-	public function applyRelation($field, &$columnName = null) {
+	public function applyRelation($field, &$columnName = null, $linearOnly = false) {
 		// If field is invalid, return it without modification
 		if(!$this->isValidRelationName($field)) {
 			$columnName = $field;
@@ -495,9 +497,23 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			return $this;
 		}
 
-		return $this->alterDataQuery(function(DataQuery $query, DataList $list) use ($field, &$columnName) {
+		return $this->alterDataQuery(
-			$columnName = $list->getRelationName($field);
+			function(DataQuery $query, DataList $list) use ($field, &$columnName, $linearOnly) {
-		});
+				$relations = explode('.', $field);
+				$fieldName = array_pop($relations);
+
+				// Apply
+				$relationModelName = $query->applyRelation($field, $linearOnly);
+
+				// Find the db field the relation belongs to
+				$className = ClassInfo::table_for_object_field($relationModelName, $fieldName);
+				if(empty($className)) {
+					$className = $relationModelName;
+				}
+
+				$columnName = '"'.$className.'"."'.$fieldName.'"';
+			}
+		);
 	}
 
 	/**
@@ -510,44 +526,6 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 		return preg_match('/^[A-Z0-9._]+$/i', $field);
 	}
 
-	/**
-	 * Translates a {@link Object} relation name to a Database name and apply
-	 * the relation join to the query.  Throws an InvalidArgumentException if
-	 * the $field doesn't correspond to a relation.
-	 *
-	 * @throws InvalidArgumentException
-	 * @param string $field
-	 *
-	 * @return string
-	 */
-	public function getRelationName($field) {
-		if(!$this->isValidRelationName($field)) {
-			throw new InvalidArgumentException("Bad field expression $field");
-		}
-
-		if (!$this->inAlterDataQueryCall) {
-			throw new BadMethodCallException(
-				'getRelationName is mutating, and must be called inside an alterDataQuery block'
-			);
-		}
-
-		if(strpos($field,'.') === false) {
-			return '"'.$field.'"';
-		}
-
-		$relations = explode('.', $field);
-		$fieldName = array_pop($relations);
-		$relationModelName = $this->dataQuery->applyRelation($field);
-
-		// Find the db field the relation belongs to
-		$className = ClassInfo::table_for_object_field($relationModelName, $fieldName);
-		if(empty($className)) {
-			$className = $relationModelName;
-		}
-
-		return '"'.$className.'"."'.$fieldName.'"';
-	}
-
 	/**
 	 * Translates a filter type to a SQL query.
 	 *

model/DataQuery.php

@@ -642,9 +642,11 @@ class DataQuery {
 	 * in any overloaded {@link SearchFilter->apply()} methods manually.
 	 *
 	 * @param String|array $relation The array/dot-syntax relation to follow
+	 * @param bool $linearOnly Set to true to restrict to linear relations only. Set this
+	 * if this relation will be used for sorting, and should not include duplicate rows.
 	 * @return The model class of the related item
 	 */
-	public function applyRelation($relation) {
+	public function applyRelation($relation, $linearOnly = false) {
 		// NO-OP
 		if(!$relation) return $this->dataClass;
 
@@ -655,62 +657,29 @@ class DataQuery {
 		foreach($relation as $rel) {
 			$model = singleton($modelClass);
 			if ($component = $model->hasOneComponent($rel)) {
-				if(!$this->query->isJoinedTo($component)) {
+				// Join via has_one
-					$foreignKey = $rel;
+				$this->joinHasOneRelation($modelClass, $rel, $component);
-					$realModelClass = ClassInfo::table_for_object_field($modelClass, "{$foreignKey}ID");
-					$this->query->addLeftJoin($component,
-						"\"$component\".\"ID\" = \"{$realModelClass}\".\"{$foreignKey}ID\"");
-
-					/**
-					 * add join clause to the component's ancestry classes so that the search filter could search on
-					 * its ancestor fields.
-					 */
-					$ancestry = ClassInfo::ancestry($component, true);
-					if(!empty($ancestry)){
-						$ancestry = array_reverse($ancestry);
-						foreach($ancestry as $ancestor){
-							if($ancestor != $component){
-								$this->query->addLeftJoin($ancestor, "\"$component\".\"ID\" = \"$ancestor\".\"ID\"");
-							}
-						}
-					}
-				}
 				$modelClass = $component;
 
 			} elseif ($component = $model->hasManyComponent($rel)) {
-				if(!$this->query->isJoinedTo($component)) {
+				// Fail on non-linear relations
-					$ancestry = $model->getClassAncestry();
+				if($linearOnly) {
-					$foreignKey = $model->getRemoteJoinField($rel);
+					throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass");
-					$this->query->addLeftJoin($component,
-						"\"$component\".\"{$foreignKey}\" = \"{$ancestry[0]}\".\"ID\"");
-					/**
-					 * add join clause to the component's ancestry classes so that the search filter could search on
-					 * its ancestor fields.
-					 */
-					$ancestry = ClassInfo::ancestry($component, true);
-					if(!empty($ancestry)){
-						$ancestry = array_reverse($ancestry);
-						foreach($ancestry as $ancestor){
-							if($ancestor != $component){
-								$this->query->addInnerJoin($ancestor, "\"$component\".\"ID\" = \"$ancestor\".\"ID\"");
-							}
-						}
-					}
 				}
+				// Join via has_many
+				$this->joinHasManyRelation($modelClass, $rel, $component);
 				$modelClass = $component;
 
 			} elseif ($component = $model->manyManyComponent($rel)) {
-				list($parentClass, $componentClass, $parentField, $componentField, $relationTable) = $component;
+				// Fail on non-linear relations
-				$parentBaseClass = ClassInfo::baseDataClass($parentClass);
+				if($linearOnly) {
-				$componentBaseClass = ClassInfo::baseDataClass($componentClass);
+					throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass");
-				$this->query->addLeftJoin($relationTable,
-					"\"$relationTable\".\"$parentField\" = \"$parentBaseClass\".\"ID\"");
-				$this->query->addLeftJoin($componentBaseClass,
-					"\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\"");
-				if(ClassInfo::hasTable($componentClass)) {
-					$this->query->addLeftJoin($componentClass,
-						"\"$relationTable\".\"$componentField\" = \"$componentClass\".\"ID\"");
 				}
+				// Join via many_many
+				list($parentClass, $componentClass, $parentField, $componentField, $relationTable) = $component;
+				$this->joinManyManyRelationship(
+					$parentClass, $componentClass, $parentField, $componentField, $relationTable
+				);
 				$modelClass = $componentClass;
 
 			}
@@ -719,6 +688,135 @@ class DataQuery {
 		return $modelClass;
 	}
 
+	/**
+	 * Join the given class to this query with the given key
+	 *
+	 * @param string $localClass Name of class that has the has_one to the joined class
+	 * @param string $localField Name of the has_one relationship to joi
+	 * @param string $foreignClass Class to join
+	 */
+	protected function joinHasOneRelation($localClass, $localField, $foreignClass)
+	{
+		if (!$foreignClass) {
+			throw new InvalidArgumentException("Could not find a has_one relationship {$localField} on {$localClass}");
+		}
+
+		if ($foreignClass === 'DataObject') {
+			throw new InvalidArgumentException(
+				"Could not join polymorphic has_one relationship {$localField} on {$localClass}"
+			);
+		}
+
+		// Skip if already joined
+		if($this->query->isJoinedTo($foreignClass)) {
+			return;
+		}
+
+		$realModelClass = ClassInfo::table_for_object_field($localClass, "{$localField}ID");
+		$foreignBase = ClassInfo::baseDataClass($foreignClass);
+		$this->query->addLeftJoin(
+			$foreignBase,
+			"\"$foreignBase\".\"ID\" = \"{$realModelClass}\".\"{$localField}ID\""
+		);
+
+		/**
+		 * add join clause to the component's ancestry classes so that the search filter could search on
+		 * its ancestor fields.
+		 */
+		$ancestry = ClassInfo::ancestry($foreignClass, true);
+		if(!empty($ancestry)){
+			$ancestry = array_reverse($ancestry);
+			foreach($ancestry as $ancestor){
+				if($ancestor != $foreignBase) {
+					$this->query->addLeftJoin($ancestor, "\"$foreignBase\".\"ID\" = \"$ancestor\".\"ID\"");
+				}
+			}
+		}
+	}
+
+	/**
+	 * Join the given has_many relation to this query.
+	 *
+	 * Doesn't work with polymorphic relationships
+	 *
+	 * @param string $localClass Name of class that has the has_many to the joined class
+	 * @param string $localField Name of the has_many relationship to join
+	 * @param string $foreignClass Class to join
+	 */
+	protected function joinHasManyRelation($localClass, $localField, $foreignClass) {
+		if(!$foreignClass || $foreignClass === 'DataObject') {
+			throw new InvalidArgumentException("Could not find a has_many relationship {$localField} on {$localClass}");
+		}
+		
+		// Skip if already joined
+		if($this->query->isJoinedTo($foreignClass)) {
+			return;
+		}
+
+		// Join table with associated has_one
+		$model = singleton($localClass);
+		$ancestry = $model->getClassAncestry();
+		$foreignKey = $model->getRemoteJoinField($localField, 'has_many', $polymorphic);
+		if($polymorphic) {
+			$this->query->addLeftJoin(
+				$foreignClass,
+				"\"$foreignClass\".\"{$foreignKey}ID\" = \"{$ancestry[0]}\".\"ID\" AND "
+					. "\"$foreignClass\".\"{$foreignKey}Class\" = \"{$ancestry[0]}\".\"ClassName\""
+			);
+		} else {
+			$this->query->addLeftJoin(
+				$foreignClass,
+				"\"$foreignClass\".\"{$foreignKey}\" = \"{$ancestry[0]}\".\"ID\""
+			);
+		}
+		
+		/**
+		 * add join clause to the component's ancestry classes so that the search filter could search on
+		 * its ancestor fields.
+		 */
+		$ancestry = ClassInfo::ancestry($foreignClass, true);
+		$ancestry = array_reverse($ancestry);
+		foreach($ancestry as $ancestor){
+			if($ancestor != $foreignClass){
+				$this->query->addInnerJoin($ancestor, "\"$foreignClass\".\"ID\" = \"$ancestor\".\"ID\"");
+			}
+		}
+	}
+
+	/**
+	 * Join table via many_many relationship
+	 *
+	 * @param string $parentClass
+	 * @param string $componentClass
+	 * @param string $parentField
+	 * @param string $componentField
+	 * @param string $relationTable Name of relation table
+	 */
+	protected function joinManyManyRelationship($parentClass, $componentClass, $parentField, $componentField, $relationTable) {
+		$parentBaseClass = ClassInfo::baseDataClass($parentClass);
+		$componentBaseClass = ClassInfo::baseDataClass($componentClass);
+		$this->query->addLeftJoin(
+			$relationTable,
+			"\"$relationTable\".\"$parentField\" = \"$parentBaseClass\".\"ID\""
+		);
+		$this->query->addLeftJoin(
+			$componentBaseClass,
+			"\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\""
+		);
+
+		/**
+		 * add join clause to the component's ancestry classes so that the search filter could search on
+		 * its ancestor fields.
+		 */
+		$ancestry = ClassInfo::ancestry($componentClass, true);
+		$ancestry = array_reverse($ancestry);
+		foreach($ancestry as $ancestor){
+			if($ancestor != $componentBaseClass){
+				$this->query->addInnerJoin($ancestor, "\"$componentBaseClass\".\"ID\" = \"$ancestor\".\"ID\"");
+			}
+		}
+	}
+
 	/**
 	 * Removes the result of query from this query.
 	 *

model/UnsavedRelationList.php

@@ -323,10 +323,6 @@ class UnsavedRelationList extends ArrayList {
 		throw new LogicException(__FUNCTION__ . " can't be called on an UnsavedRelationList.");
 	}
 
-	public function getRelationName() {
-		throw new LogicException(__FUNCTION__ . " can't be called on an UnsavedRelationList.");
-	}
-
 	public function innerJoin() {
 		throw new LogicException(__FUNCTION__ . " can't be called on an UnsavedRelationList.");
 	}

tests/forms/gridfield/GridFieldSortableHeaderTest.php

@@ -162,15 +162,16 @@ class GridFieldSortableHeaderTest extends SapphireTest {
 			. 'ON "GridFieldSortableHeaderTest_TeamGroup"."ID" = "GridFieldSortableHeaderTest_Team"."ID"',
 			$relationListBsql
 		);
-		$this->assertContains(
+		// Joined tables are joined basetable first
-			'LEFT JOIN "GridFieldSortableHeaderTest_Mom" '
-			. 'ON "GridFieldSortableHeaderTest_Mom"."ID" = "GridFieldSortableHeaderTest_Team"."CheerleadersMomID"',
-			$relationListBsql
-		);
-		// Note that cheerleader is no longer aliased, as it is an implicit join
 		$this->assertContains(
 			'LEFT JOIN "GridFieldSortableHeaderTest_Cheerleader" '
-			. 'ON "GridFieldSortableHeaderTest_Mom"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."ID"',
+				. 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Team"."CheerleadersMomID"',
+			$relationListBsql
+		);
+		// Then the basetable of the joined record is joined to the specific subtable
+		$this->assertContains(
+			'LEFT JOIN "GridFieldSortableHeaderTest_Mom" '
+			. 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Mom"."ID"',
 			$relationListBsql
 		);
 		$this->assertContains(

tests/model/DataListTest.php

@@ -530,6 +530,17 @@ class DataListTest extends SapphireTest {
 		$this->assertEquals('Phil', $list->last()->Name);
 	}
 
+	public function testSortInvalidParameters() {
+		$this->setExpectedException(
+			'InvalidArgumentException',
+			'Fans is not a linear relation on model DataObjectTest_Player'
+		);
+		$list = DataObjectTest_Team::get();
+		$list = $list->sort('Founder.Fans.Surname'); // Can't sort on has_many
+	}
+
+
+
 	/**
 	 * $list->filter('Name', 'bob'); // only bob in the list
 	 */