From ba2c5b48f7f0308db7878989cd17b37af7ee59a0 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 9 Nov 2017 17:08:31 +1300 Subject: [PATCH] BUG Ensure relObject() safely bails on empty objects BUG Remove assignment of IDs to singletons API relation methods can take an optional $id parameter to get relations from specific parents API Added UnsavedRelationList::relation() method --- src/ORM/DataList.php | 5 +- src/ORM/DataObject.php | 115 ++++++++++++-------------- src/ORM/Relation.php | 6 ++ src/ORM/RelationList.php | 2 +- src/ORM/UnsavedRelationList.php | 24 ++++-- tests/php/ORM/DataObjectTest.php | 40 +++++++-- tests/php/ORM/DataObjectTest/Team.php | 5 ++ 7 files changed, 121 insertions(+), 76 deletions(-) diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index d55c77bc0..ee18097ef 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -1017,7 +1017,10 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li public function relation($relationName) { $ids = $this->column('ID'); - return singleton($this->dataClass)->$relationName()->forForeignID($ids); + $singleton = DataObject::singleton($this->dataClass); + /** @var HasManyList|ManyManyList $relation */ + $relation = $singleton->$relationName($ids); + return $relation; } public function dbObject($fieldName) diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index e59e9f11c..081382b28 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1584,10 +1584,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * Returns a one-to-many relation as a HasManyList * * @param string $componentName Name of the component + * @param int|array $id Optional ID(s) for parent of this relation, if not the current record * @return HasManyList|UnsavedRelationList The components of the one-to-many relationship. */ - public function getComponents($componentName) + public function getComponents($componentName, $id = null) { + if (!isset($id)) { + $id = $this->ID; + } $result = null; $schema = $this->getSchema(); @@ -1601,7 +1605,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } // If we haven't been written yet, we can't save these relations, so use a list that handles this case - if (!$this->ID) { + if (!$id) { if (!isset($this->unsavedRelations[$componentName])) { $this->unsavedRelations[$componentName] = new UnsavedRelationList(static::class, $componentName, $componentClass); @@ -1620,7 +1624,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $result ->setDataQueryParam($this->getInheritableQueryParams()) - ->forForeignID($this->ID); + ->forForeignID($id); } /** @@ -1799,10 +1803,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity /** * Returns a many-to-many component, as a ManyManyList. * @param string $componentName Name of the many-many component + * @param int|array $id Optional ID for parent of this relation, if not the current record * @return RelationList|UnsavedRelationList The set of components */ - public function getManyManyComponents($componentName) + public function getManyManyComponents($componentName, $id = null) { + if (!isset($id)) { + $id = $this->ID; + } $schema = static::getSchema(); $manyManyComponent = $schema->manyManyComponent(static::class, $componentName); if (!$manyManyComponent) { @@ -1814,7 +1822,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } // If we haven't been written yet, we can't save these relations, so use a list that handles this case - if (!$this->ID) { + if (!$id) { if (!isset($this->unsavedRelations[$componentName])) { $this->unsavedRelations[$componentName] = new UnsavedRelationList($manyManyComponent['parentClass'], $componentName, $manyManyComponent['childClass']); @@ -1846,7 +1854,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // foreignID set elsewhere. return $result ->setDataQueryParam($this->getInheritableQueryParams()) - ->forForeignID($this->ID); + ->forForeignID($id); } /** @@ -2013,8 +2021,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } // Otherwise, use the database field's scaffolder - } else { - $field = $this->relObject($fieldName)->scaffoldSearchField(); + } elseif ($object = $this->relObject($fieldName)) { + $field = $object->scaffoldSearchField(); } // Allow fields to opt out of search @@ -2679,82 +2687,69 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * The path to the related field is specified with dot separated syntax * (eg: Parent.Child.Child.FieldName). * - * @param string $fieldPath + * If a relation is blank, this will return null instead. + * If a relation name is invalid (e.g. non-relation on a parent) this + * can throw a LogicException. + * + * @param string $fieldPath List of paths on this object. All items in this path + * must be ViewableData implementors * * @return mixed DBField of the field on the object or a DataList instance. + * @throws LogicException If accessing invalid relations */ public function relObject($fieldPath) { $object = null; + $component = $this; - if (strpos($fieldPath, '.') !== false) { - $parts = explode('.', $fieldPath); - $fieldName = array_pop($parts); - - // Traverse dot syntax - $component = $this; - - foreach ($parts as $relation) { - if ($component instanceof SS_List) { - if (method_exists($component, $relation)) { - $component = $component->$relation(); - } else { - /** @var DataList $component */ - $component = $component->relation($relation); - } - } else { - $component = $component->$relation(); - } + // Parse all relations + foreach (explode('.', $fieldPath) as $relation) { + if (!$component) { + return null; } - $object = $component->dbObject($fieldName); - } else { - $object = $this->dbObject($fieldPath); + // Inspect relation type + if (ClassInfo::hasMethod($component, $relation)) { + $component = $component->$relation(); + } elseif ($component instanceof Relation || $component instanceof DataList) { + // $relation could either be a field (aggregate), or another relation + $singleton = DataObject::singleton($component->dataClass()); + $component = $singleton->dbObject($relation) ?: $component->relation($relation); + } elseif ($component instanceof DataObject && ($dbObject = $component->dbObject($relation))) { + $component = $dbObject; + } elseif ($component instanceof ViewableData && $component->hasField($relation)) { + $component = $component->obj($relation); + } else { + throw new LogicException( + "$relation is not a relation/field on ".get_class($component) + ); + } } - - return $object; + return $component; } /** * Traverses to a field referenced by relationships between data objects, returning the value * The path to the related field is specified with dot separated syntax (eg: Parent.Child.Child.FieldName) * - * @param $fieldName string - * @return string | null - will return null on a missing value + * @param string $fieldName string + * @return mixed Will return null on a missing value */ public function relField($fieldName) { + // Navigate to relative parent using relObject() if needed $component = $this; - - // We're dealing with relations here so we traverse the dot syntax - if (strpos($fieldName, '.') !== false) { - $relations = explode('.', $fieldName); - $fieldName = array_pop($relations); - foreach ($relations as $relation) { - // Inspect $component for element $relation - if ($component->hasMethod($relation)) { - // Check nested method - $component = $component->$relation(); - } elseif ($component instanceof SS_List) { - // Select adjacent relation from DataList - /** @var DataList $component */ - $component = $component->relation($relation); - } elseif ($component instanceof DataObject - && ($dbObject = $component->dbObject($relation)) - ) { - // Select db object - $component = $dbObject; - } else { - user_error("$relation is not a relation/field on ".get_class($component), E_USER_ERROR); - } - } + if (($pos = strrpos($fieldName, '.')) !== false) { + $relation = substr($fieldName, 0, $pos); + $fieldName = substr($fieldName, $pos + 1); + $component = $this->relObject($relation); } // Bail if the component is null if (!$component) { return null; } - if ($component->hasMethod($fieldName)) { + if (ClassInfo::hasMethod($component, $fieldName)) { return $component->$fieldName(); } return $component->$fieldName; @@ -3209,14 +3204,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity if (is_int($name)) { // Format: array('MyFieldName') $rewrite[$identifer] = array(); - } elseif (is_array($specOrName)) { + } elseif (is_array($specOrName) && ($relObject = $this->relObject($identifer))) { // Format: array('MyFieldName' => array( // 'filter => 'ExactMatchFilter', // 'field' => 'NumericField', // optional // 'title' => 'My Title', // optional // )) $rewrite[$identifer] = array_merge( - array('filter' => $this->relObject($identifer)->config()->get('default_search_filter_class')), + array('filter' => $relObject->config()->get('default_search_filter_class')), (array)$specOrName ); } else { diff --git a/src/ORM/Relation.php b/src/ORM/Relation.php index 34a404946..47c2ffa49 100644 --- a/src/ORM/Relation.php +++ b/src/ORM/Relation.php @@ -6,6 +6,12 @@ use SilverStripe\ORM\FieldType\DBField; /** * Abstract representation of a DB relation field, either saved or in memory + * + * Below methods will be added in 5.x + * + * @method Relation relation($relationName) + * @method Relation forForeignID($id) + * @method string dataClass() */ interface Relation extends SS_List, Filterable, Sortable, Limitable { diff --git a/src/ORM/RelationList.php b/src/ORM/RelationList.php index 0aca6fead..9a50be520 100644 --- a/src/ORM/RelationList.php +++ b/src/ORM/RelationList.php @@ -62,7 +62,7 @@ abstract class RelationList extends DataList implements Relation try { $query->removeFilterOn($currentFilter); } catch (Exception $e) { -/* NOP */ + /* NOP */ } } diff --git a/src/ORM/UnsavedRelationList.php b/src/ORM/UnsavedRelationList.php index 9677aeeee..ea11dddbd 100644 --- a/src/ORM/UnsavedRelationList.php +++ b/src/ORM/UnsavedRelationList.php @@ -282,13 +282,27 @@ class UnsavedRelationList extends ArrayList implements Relation /** * Returns a copy of this list with the relationship linked to the given foreign ID. * @param int|array $id An ID or an array of IDs. - * @return static + * @return Relation */ public function forForeignID($id) { - $class = singleton($this->baseClass); - $class->ID = 1; - return $class->{$this->relationName}()->forForeignID($id); + $singleton = DataObject::singleton($this->baseClass); + /** @var Relation $relation */ + $relation = $singleton->{$this->relationName}($id); + return $relation; + } + + /** + * @param string $relationName + * @return Relation + */ + public function relation($relationName) + { + $ids = $this->column('ID'); + $singleton = DataObject::singleton($this->dataClass); + /** @var Relation $relation */ + $relation = $singleton->$relationName($ids); + return $relation; } /** @@ -299,7 +313,7 @@ class UnsavedRelationList extends ArrayList implements Relation */ public function dbObject($fieldName) { - return singleton($this->dataClass)->dbObject($fieldName); + return DataObject::singleton($this->dataClass)->dbObject($fieldName); } protected function extractValue($item, $key) diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index dfbccaab8..cf6290094 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\Tests; +use LogicException; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\SapphireTest; use SilverStripe\i18n\i18n; @@ -1992,7 +1993,7 @@ class DataObjectTest extends SapphireTest } /** - * @expectedException \LogicException + * @expectedException LogicException */ public function testInvalidate() { @@ -2047,11 +2048,17 @@ class DataObjectTest extends SapphireTest public function testRelField() { - $captain = $this->objFromFixture(DataObjectTest\Player::class, 'captain1'); + $captain1 = $this->objFromFixture(DataObjectTest\Player::class, 'captain1'); // Test traversal of a single has_one - $this->assertEquals("Team 1", $captain->relField('FavouriteTeam.Title')); + $this->assertEquals("Team 1", $captain1->relField('FavouriteTeam.Title')); // Test direct field access - $this->assertEquals("Captain", $captain->relField('FirstName')); + $this->assertEquals("Captain", $captain1->relField('FirstName')); + + // Test empty link + $captain2 = $this->objFromFixture(DataObjectTest\Player::class, 'captain2'); + $this->assertEmpty($captain2->relField('FavouriteTeam.Title')); + $this->assertNull($captain2->relField('FavouriteTeam.ReturnsNull')); + $this->assertNull($captain2->relField('FavouriteTeam.ReturnsNull.Title')); $player = $this->objFromFixture(DataObjectTest\Player::class, 'player2'); // Test that we can traverse more than once, and that arbitrary methods are okay @@ -2063,24 +2070,39 @@ class DataObjectTest extends SapphireTest // Test that relField works on db field manipulations $comment = $this->objFromFixture(DataObjectTest\TeamComment::class, 'comment3'); $this->assertEquals("PHIL IS A UNIQUE GUY, AND COMMENTS ON TEAM2", $comment->relField('Comment.UpperCase')); + + // relField throws exception on invalid properties + $this->expectException(LogicException::class); + $this->expectExceptionMessage("Not is not a relation/field on " . DataObjectTest\TeamComment::class); + $comment->relField('Not.A.Field'); } public function testRelObject() { - $captain = $this->objFromFixture(DataObjectTest\Player::class, 'captain1'); + $captain1 = $this->objFromFixture(DataObjectTest\Player::class, 'captain1'); // Test traversal of a single has_one - $this->assertInstanceOf(DBVarchar::class, $captain->relObject('FavouriteTeam.Title')); - $this->assertEquals("Team 1", $captain->relObject('FavouriteTeam.Title')->getValue()); + $this->assertInstanceOf(DBVarchar::class, $captain1->relObject('FavouriteTeam.Title')); + $this->assertEquals("Team 1", $captain1->relObject('FavouriteTeam.Title')->getValue()); + + // Test empty link + $captain2 = $this->objFromFixture(DataObjectTest\Player::class, 'captain2'); + $this->assertEmpty($captain2->relObject('FavouriteTeam.Title')->getValue()); + $this->assertNull($captain2->relObject('FavouriteTeam.ReturnsNull.Title')); // Test direct field access - $this->assertInstanceOf(DBBoolean::class, $captain->relObject('IsRetired')); - $this->assertEquals(1, $captain->relObject('IsRetired')->getValue()); + $this->assertInstanceOf(DBBoolean::class, $captain1->relObject('IsRetired')); + $this->assertEquals(1, $captain1->relObject('IsRetired')->getValue()); $player = $this->objFromFixture(DataObjectTest\Player::class, 'player2'); // Test that we can traverse more than once, and that arbitrary methods are okay $this->assertInstanceOf(DBVarchar::class, $player->relObject('Teams.First.Title')); $this->assertEquals("Team 1", $player->relObject('Teams.First.Title')->getValue()); + + // relObject throws exception on invalid properties + $this->expectException(LogicException::class); + $this->expectExceptionMessage("Not is not a relation/field on " . DataObjectTest\Player::class); + $player->relObject('Not.A.Field'); } public function testLateStaticBindingStyle() diff --git a/tests/php/ORM/DataObjectTest/Team.php b/tests/php/ORM/DataObjectTest/Team.php index b44317d11..b7917c55b 100644 --- a/tests/php/ORM/DataObjectTest/Team.php +++ b/tests/php/ORM/DataObjectTest/Team.php @@ -80,4 +80,9 @@ class Team extends DataObject implements TestOnly { return 'dynamicfield'; } + + public function ReturnsNull() + { + return null; + } }