diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index 178b5e77a..6ece0b397 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/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 diff --git a/model/DataList.php b/model/DataList.php index 1f31586fc..fa6317f79 100644 --- a/model/DataList.php +++ b/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) { - $columnName = $list->getRelationName($field); - }); + return $this->alterDataQuery( + 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. * diff --git a/model/DataQuery.php b/model/DataQuery.php index 27fb05746..8f9b998b7 100644 --- a/model/DataQuery.php +++ b/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)) { - $foreignKey = $rel; - $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\""); - } - } - } - } + // Join via has_one + $this->joinHasOneRelation($modelClass, $rel, $component); $modelClass = $component; } elseif ($component = $model->hasManyComponent($rel)) { - if(!$this->query->isJoinedTo($component)) { - $ancestry = $model->getClassAncestry(); - $foreignKey = $model->getRemoteJoinField($rel); - $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\""); - } - } - } + // Fail on non-linear relations + if($linearOnly) { + throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass"); } + // Join via has_many + $this->joinHasManyRelation($modelClass, $rel, $component); $modelClass = $component; } elseif ($component = $model->manyManyComponent($rel)) { - list($parentClass, $componentClass, $parentField, $componentField, $relationTable) = $component; - $parentBaseClass = ClassInfo::baseDataClass($parentClass); - $componentBaseClass = ClassInfo::baseDataClass($componentClass); - $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\""); + // Fail on non-linear relations + if($linearOnly) { + throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass"); } + // 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. * diff --git a/model/UnsavedRelationList.php b/model/UnsavedRelationList.php index 05db48c92..634735892 100644 --- a/model/UnsavedRelationList.php +++ b/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."); } diff --git a/tests/forms/gridfield/GridFieldSortableHeaderTest.php b/tests/forms/gridfield/GridFieldSortableHeaderTest.php index 9229d3f1b..f74dcb594 100644 --- a/tests/forms/gridfield/GridFieldSortableHeaderTest.php +++ b/tests/forms/gridfield/GridFieldSortableHeaderTest.php @@ -162,15 +162,16 @@ class GridFieldSortableHeaderTest extends SapphireTest { . 'ON "GridFieldSortableHeaderTest_TeamGroup"."ID" = "GridFieldSortableHeaderTest_Team"."ID"', $relationListBsql ); - $this->assertContains( - '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 + // Joined tables are joined basetable first $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( diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index 9fab09ed4..f1ec620d4 100755 --- a/tests/model/DataListTest.php +++ b/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 */