From 2b1e5ee071e72494c02a1c04a508b54e401afe35 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 17 Sep 2015 10:19:03 +1200 Subject: [PATCH 1/2] API Enable DataList::sort to support composite field names (similar to filter) --- forms/gridfield/GridFieldSortableHeader.php | 49 +------- model/DataList.php | 82 +++++++++---- model/DataQuery.php | 4 +- .../gridfield/GridFieldSortableHeaderTest.php | 110 +++++++++++++++++- .../gridfield/GridFieldSortableHeaderTest.yml | 109 +++++++++++------ tests/model/DataListTest.php | 9 ++ tests/model/DataObjectTest.php | 16 ++- tests/model/DataObjectTest.yml | 4 + 8 files changed, 268 insertions(+), 115 deletions(-) diff --git a/forms/gridfield/GridFieldSortableHeader.php b/forms/gridfield/GridFieldSortableHeader.php index cc9ec49e3..d4aa80abf 100644 --- a/forms/gridfield/GridFieldSortableHeader.php +++ b/forms/gridfield/GridFieldSortableHeader.php @@ -208,53 +208,6 @@ class GridFieldSortableHeader implements GridField_HTMLProvider, GridField_DataM return $dataList; } - $column = $state->SortColumn; - - // if we have a relation column with dot notation - if(strpos($column, '.') !== false) { - $lastAlias = $dataList->dataClass(); - $tmpItem = singleton($lastAlias); - $parts = explode('.', $state->SortColumn); - - for($idx = 0; $idx < sizeof($parts); $idx++) { - $methodName = $parts[$idx]; - - // If we're not on the last item, we're looking at a relation - if($idx !== sizeof($parts) - 1) { - // Traverse to the relational list - $tmpItem = $tmpItem->$methodName(); - - $joinClass = ClassInfo::table_for_object_field( - $lastAlias, - $methodName . "ID" - ); - - // if the field isn't in the object tree then it is likely - // been aliased. In that event, assume what the user has - // provided is the correct value - if(!$joinClass) $joinClass = $lastAlias; - - $dataList = $dataList->leftJoin( - $tmpItem->class, - '"' . $methodName . '"."ID" = "' . $joinClass . '"."' . $methodName . 'ID"', - $methodName - ); - - // Store the last 'alias' name as it'll be used for the next - // join, or the 'sort' column - $lastAlias = $methodName; - } else { - // Change relation.relation.fieldname to alias.fieldname - $column = $lastAlias . '.' . $methodName; - } - } - } - - // We need to manually create our ORDER BY "Foo"."Bar" string for relations, - // as ->sort() won't do it by itself. Blame PostgreSQL for making this necessary - $pieces = explode('.', $column); - $column = '"' . implode('"."', $pieces) . '"'; - - return $dataList->sort($column, $state->SortDirection('asc')); + return $dataList->sort($state->SortColumn, $state->SortDirection('asc')); } } diff --git a/model/DataList.php b/model/DataList.php index 772febd1a..1f31586fc 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -297,28 +297,26 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab if ($count == 2) { list($col, $dir) = func_get_args(); + + // Validate direction + if(!in_array(strtolower($dir),array('desc','asc'))){ + user_error('Second argument to sort must be either ASC or DESC'); + } + + $sort = array($col => $dir); } else { $sort = func_get_arg(0); } - return $this->alterDataQuery(function($query, $list) use ($sort, $col, $dir){ + return $this->alterDataQuery(function(DataQuery $query, DataList $list) use ($sort){ - if ($col) { - // sort('Name','Desc') - if(!in_array(strtolower($dir),array('desc','asc'))){ - user_error('Second argument to sort must be either ASC or DESC'); - } - - $query->sort($col, $dir); - } - - else if(is_string($sort) && $sort){ - // sort('Name ASC') + if(is_string($sort) && $sort){ if(stristr($sort, ' asc') || stristr($sort, ' desc')) { $query->sort($sort); } else { - $query->sort($sort, 'ASC'); + $list->applyRelation($sort, $column); + $query->sort($column, 'ASC'); } } @@ -326,15 +324,11 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab // sort(array('Name'=>'desc')); $query->sort(null, null); // wipe the sort - foreach($sort as $col => $dir) { + foreach($sort as $column => $direction) { // Convert column expressions to SQL fragment, while still allowing the passing of raw SQL // fragments. - try { - $relCol = $list->getRelationName($col); - } catch(InvalidArgumentException $e) { - $relCol = $col; - } - $query->sort($relCol, $dir, false); + $list->applyRelation($column, $relationColumn); + $query->sort($relationColumn, $direction, false); } } }); @@ -478,6 +472,44 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab return $output; } + /** + * Given a field or relation name, apply it safely to this datalist. + * + * Unlike getRelationName, this is immutable and will fallback to the quoted field + * name if not a relation. + * + * @param string $field Name of field or relation to apply + * @param string &$columnName Quoted column name + * @return DataList DataList with this relation applied + */ + public function applyRelation($field, &$columnName = null) { + // If field is invalid, return it without modification + if(!$this->isValidRelationName($field)) { + $columnName = $field; + return $this; + } + + // Simple fields without relations are mapped directly + if(strpos($field,'.') === false) { + $columnName = '"'.$field.'"'; + return $this; + } + + return $this->alterDataQuery(function(DataQuery $query, DataList $list) use ($field, &$columnName) { + $columnName = $list->getRelationName($field); + }); + } + + /** + * Check if the given field specification could be interpreted as an unquoted relation name + * + * @param string $field + * @return bool + */ + protected function isValidRelationName($field) { + 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 @@ -489,7 +521,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab * @return string */ public function getRelationName($field) { - if(!preg_match('/^[A-Z0-9._]+$/i', $field)) { + if(!$this->isValidRelationName($field)) { throw new InvalidArgumentException("Bad field expression $field"); } @@ -507,7 +539,13 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab $fieldName = array_pop($relations); $relationModelName = $this->dataQuery->applyRelation($field); - return '"'.$relationModelName.'"."'.$fieldName.'"'; + // Find the db field the relation belongs to + $className = ClassInfo::table_for_object_field($relationModelName, $fieldName); + if(empty($className)) { + $className = $relationModelName; + } + + return '"'.$className.'"."'.$fieldName.'"'; } /** diff --git a/model/DataQuery.php b/model/DataQuery.php index 1fe90ed6c..27fb05746 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -670,7 +670,7 @@ class DataQuery { $ancestry = array_reverse($ancestry); foreach($ancestry as $ancestor){ if($ancestor != $component){ - $this->query->addInnerJoin($ancestor, "\"$component\".\"ID\" = \"$ancestor\".\"ID\""); + $this->query->addLeftJoin($ancestor, "\"$component\".\"ID\" = \"$ancestor\".\"ID\""); } } } @@ -703,7 +703,7 @@ class DataQuery { list($parentClass, $componentClass, $parentField, $componentField, $relationTable) = $component; $parentBaseClass = ClassInfo::baseDataClass($parentClass); $componentBaseClass = ClassInfo::baseDataClass($componentClass); - $this->query->addInnerJoin($relationTable, + $this->query->addLeftJoin($relationTable, "\"$relationTable\".\"$parentField\" = \"$parentBaseClass\".\"ID\""); $this->query->addLeftJoin($componentBaseClass, "\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\""); diff --git a/tests/forms/gridfield/GridFieldSortableHeaderTest.php b/tests/forms/gridfield/GridFieldSortableHeaderTest.php index e1c2eb4a1..9229d3f1b 100644 --- a/tests/forms/gridfield/GridFieldSortableHeaderTest.php +++ b/tests/forms/gridfield/GridFieldSortableHeaderTest.php @@ -10,8 +10,10 @@ class GridFieldSortableHeaderTest extends SapphireTest { protected $extraDataObjects = array( 'GridFieldSortableHeaderTest_Team', + 'GridFieldSortableHeaderTest_TeamGroup', 'GridFieldSortableHeaderTest_Cheerleader', - 'GridFieldSortableHeaderTest_CheerleaderHat' + 'GridFieldSortableHeaderTest_CheerleaderHat', + 'GridFieldSortableHeaderTest_Mom' ); /** @@ -103,6 +105,94 @@ class GridFieldSortableHeaderTest extends SapphireTest { ); } + /** + * Test getManipulatedData on subclassed dataobjects + */ + public function testInheritedGetManiplatedData() { + $list = GridFieldSortableHeaderTest_TeamGroup::get(); + $config = new GridFieldConfig_RecordEditor(); + $gridField = new GridField('testfield', 'testfield', $list, $config); + $state = $gridField->State->GridFieldSortableHeader; + $compontent = $gridField->getConfig()->getComponentByType('GridFieldSortableHeader'); + + // Test that inherited dataobjects will work correctly + $state->SortColumn = 'Cheerleader.Hat.Colour'; + $state->SortDirection = 'asc'; + $relationListA = $compontent->getManipulatedData($gridField, $list); + $relationListAsql = Convert::nl2os($relationListA->sql(), ' '); + + // Assert that all tables are joined properly + $this->assertContains('FROM "GridFieldSortableHeaderTest_Team"', $relationListAsql); + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_TeamGroup" ' + . 'ON "GridFieldSortableHeaderTest_TeamGroup"."ID" = "GridFieldSortableHeaderTest_Team"."ID"', + $relationListAsql + ); + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_Cheerleader" ' + . 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Team"."CheerleaderID"', + $relationListAsql + ); + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_CheerleaderHat" ' + . 'ON "GridFieldSortableHeaderTest_CheerleaderHat"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."HatID"', $relationListAsql); + + // Test sorting is correct + $this->assertEquals( + array('Cologne', 'Auckland', 'Wellington', 'Melbourne'), + $relationListA->column('City') + ); + $state->SortDirection = 'desc'; + $relationListAdesc = $compontent->getManipulatedData($gridField, $list); + $this->assertEquals( + array('Melbourne', 'Wellington', 'Auckland', 'Cologne'), + $relationListAdesc->column('City') + ); + + // Test subclasses of tables + $state->SortColumn = 'CheerleadersMom.Hat.Colour'; + $state->SortDirection = 'asc'; + $relationListB = $compontent->getManipulatedData($gridField, $list); + $relationListBsql = $relationListB->sql(); + + // Assert that subclasses are included in the query + $this->assertContains('FROM "GridFieldSortableHeaderTest_Team"', $relationListBsql); + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_TeamGroup" ' + . '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 + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_Cheerleader" ' + . 'ON "GridFieldSortableHeaderTest_Mom"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."ID"', + $relationListBsql + ); + $this->assertContains( + 'LEFT JOIN "GridFieldSortableHeaderTest_CheerleaderHat" ' + . 'ON "GridFieldSortableHeaderTest_CheerleaderHat"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."HatID"', + $relationListBsql + ); + + + // Test sorting is correct + $this->assertEquals( + array('Cologne', 'Auckland', 'Wellington', 'Melbourne'), + $relationListB->column('City') + ); + $state->SortDirection = 'desc'; + $relationListBdesc = $compontent->getManipulatedData($gridField, $list); + $this->assertEquals( + array('Melbourne', 'Wellington', 'Auckland', 'Cologne'), + $relationListBdesc->column('City') + ); + } + } class GridFieldSortableHeaderTest_Team extends DataObject implements TestOnly { @@ -119,11 +209,18 @@ class GridFieldSortableHeaderTest_Team extends DataObject implements TestOnly { ); private static $has_one = array( - 'Cheerleader' => 'GridFieldSortableHeaderTest_Cheerleader' + 'Cheerleader' => 'GridFieldSortableHeaderTest_Cheerleader', + 'CheerleadersMom' => 'GridFieldSortableHeaderTest_Mom' ); } +class GridFieldSortableHeaderTest_TeamGroup extends GridFieldSortableHeaderTest_Team implements TestOnly { + private static $db = array( + 'GroupName' => 'Varchar' + ); +} + class GridFieldSortableHeaderTest_Cheerleader extends DataObject implements TestOnly { private static $db = array( @@ -137,6 +234,15 @@ class GridFieldSortableHeaderTest_Cheerleader extends DataObject implements Test } +/** + * Should have access to same properties as cheerleader + */ +class GridFieldSortableHeaderTest_Mom extends GridFieldSortableHeaderTest_Cheerleader implements TestOnly { + private static $db = array( + 'NumberOfCookiesBaked' => 'Int' + ); +} + class GridFieldSortableHeaderTest_CheerleaderHat extends DataObject implements TestOnly { private static $db = array( diff --git a/tests/forms/gridfield/GridFieldSortableHeaderTest.yml b/tests/forms/gridfield/GridFieldSortableHeaderTest.yml index bf0eb4557..88e34f093 100644 --- a/tests/forms/gridfield/GridFieldSortableHeaderTest.yml +++ b/tests/forms/gridfield/GridFieldSortableHeaderTest.yml @@ -1,39 +1,76 @@ GridFieldSortableHeaderTest_CheerleaderHat: - hat1: - Colour: Blue - hat2: - Colour: Red - hat3: - Colour: Green - hat4: - Colour: Pink + hat1: + Colour: Blue + hat2: + Colour: Red + hat3: + Colour: Green + hat4: + Colour: Pink GridFieldSortableHeaderTest_Cheerleader: - cheerleader1: - Name: Heather - Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat2 - cheerleader2: - Name: Bob - Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat4 - cheerleader3: - Name: Jenny - Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat1 - cheerleader4: - Name: Sam - Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat3 + cheerleader1: + Name: Heather + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat2 + cheerleader2: + Name: Bob + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat4 + cheerleader3: + Name: Jenny + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat1 + cheerleader4: + Name: Sam + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat3 + +GridFieldSortableHeaderTest_Mom: + mom1: + Name: Ethel + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat2 + mom2: + Name: Eileene + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat4 + mom3: + Name: Gertrude + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat1 + mom4: + Name: Andrew + Hat: =>GridFieldSortableHeaderTest_CheerleaderHat.hat3 + GridFieldSortableHeaderTest_Team: - team1: - Name: Team 1 - City: Cologne - Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader3 - team2: - Name: Team 2 - City: Wellington - Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader2 - team3: - Name: Team 3 - City: Auckland - Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader4 - team4: - Name: Team 4 - City: Melbourne - Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader1 + team1: + Name: Team 1 + City: Cologne + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader3 + team2: + Name: Team 2 + City: Wellington + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader2 + team3: + Name: Team 3 + City: Auckland + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader4 + team4: + Name: Team 4 + City: Melbourne + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader1 + +GridFieldSortableHeaderTest_TeamGroup: + group1: + Name: Group 1 + City: Cologne + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader3 + CheerleadersMom: =>GridFieldSortableHeaderTest_Mom.mom3 + group2: + Name: Group 2 + City: Wellington + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader2 + CheerleadersMom: =>GridFieldSortableHeaderTest_Mom.mom2 + group3: + Name: Group 3 + City: Auckland + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader4 + CheerleadersMom: =>GridFieldSortableHeaderTest_Mom.mom4 + group4: + Name: Group 4 + City: Melbourne + Cheerleader: =>GridFieldSortableHeaderTest_Cheerleader.cheerleader1 + CheerleadersMom: =>GridFieldSortableHeaderTest_Mom.mom1 diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index 57b204a2f..9fab09ed4 100755 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -521,6 +521,15 @@ class DataListTest extends SapphireTest { $this->assertEquals('Phil', $list->last()->Name, 'Last comment should be from Phil'); } + public function testSortWithCompositeSyntax() { + // Phil commented on team with founder surname "Aaron" + $list = DataObjectTest_TeamComment::get(); + $list = $list->sort('Team.Founder.Surname', 'asc'); + $this->assertEquals('Phil', $list->first()->Name); + $list = $list->sort('Team.Founder.Surname', 'desc'); + $this->assertEquals('Phil', $list->last()->Name); + } + /** * $list->filter('Name', 'bob'); // only bob in the list */ diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 4f1c35e13..f058b3d0a 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -830,11 +830,12 @@ class DataObjectTest extends SapphireTest { 'DatabaseField', 'ExtendedDatabaseField', 'CaptainID', + 'FounderID', 'HasOneRelationshipID', 'ExtendedHasOneRelationshipID' ), - array_keys($teamInstance->db()), - 'db() contains all fields defined on instance: base, extended and foreign keys' + array_keys($teamInstance->inheritedDatabaseFields()), + 'inheritedDatabaseFields() contains all fields defined on instance: base, extended and foreign keys' ); $this->assertEquals( @@ -847,11 +848,12 @@ class DataObjectTest extends SapphireTest { 'DatabaseField', 'ExtendedDatabaseField', 'CaptainID', + 'FounderID', 'HasOneRelationshipID', 'ExtendedHasOneRelationshipID' ), - array_keys(DataObjectTest_Team::database_fields()), - 'database_fields() contains only fields defined on instance, including base, extended and foreign keys' + array_keys(DataObject::database_fields('DataObjectTest_Team', false)), + 'databaseFields() contains only fields defined on instance, including base, extended and foreign keys' ); $this->assertEquals( @@ -864,6 +866,7 @@ class DataObjectTest extends SapphireTest { 'DatabaseField', 'ExtendedDatabaseField', 'CaptainID', + 'FounderID', 'HasOneRelationshipID', 'ExtendedHasOneRelationshipID', 'SubclassDatabaseField', @@ -1706,7 +1709,9 @@ class DataObjectTest_Player extends Member implements TestOnly { ); private static $has_many = array( - 'Fans' => 'DataObjectTest_Fan.Favourite' // Polymorphic - Player fans + 'Fans' => 'DataObjectTest_Fan.Favourite', // Polymorphic - Player fans + 'CaptainTeams' => 'DataObjectTest_Team.Captain', + 'FoundingTeams' => 'DataObjectTest_Team.Founder' ); private static $belongs_to = array ( @@ -1728,6 +1733,7 @@ class DataObjectTest_Team extends DataObject implements TestOnly { private static $has_one = array( "Captain" => 'DataObjectTest_Player', + "Founder" => 'DataObjectTest_Player', 'HasOneRelationship' => 'DataObjectTest_Player', ); diff --git a/tests/model/DataObjectTest.yml b/tests/model/DataObjectTest.yml index a559f744e..70292f8c7 100644 --- a/tests/model/DataObjectTest.yml +++ b/tests/model/DataObjectTest.yml @@ -20,13 +20,17 @@ DataObjectTest_Team: DataObjectTest_Player: captain1: FirstName: Captain + Surname: Zookeeper ShirtNumber: 007 FavouriteTeam: =>DataObjectTest_Team.team1 Teams: =>DataObjectTest_Team.team1 + FoundingTeams: =>DataObjectTest_Team.team1 IsRetired: 1 captain2: FirstName: Captain 2 + Surname: Aaron Teams: =>DataObjectTest_Team.team2 + FoundingTeams: =>DataObjectTest_Team.team2 player1: FirstName: Player 1 player2: From 641c26299ce9bce5c63f36277d8626aa3c1fe6e8 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 29 Oct 2015 18:37:25 +1300 Subject: [PATCH 2/2] API Enable linear-only restriction for DataList::applyRelation API Remove DataList::getRelation --- docs/en/04_Changelogs/4.0.0.md | 4 + model/DataList.php | 66 ++---- model/DataQuery.php | 196 +++++++++++++----- model/UnsavedRelationList.php | 4 - .../gridfield/GridFieldSortableHeaderTest.php | 15 +- tests/model/DataListTest.php | 11 + 6 files changed, 192 insertions(+), 104 deletions(-) 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 */