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

@@ -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)

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 {

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
 {

src/ORM/RelationList.php

@@ -62,7 +62,7 @@ abstract class RelationList extends DataList implements Relation
                 try {
                     $query->removeFilterOn($currentFilter);
                 } catch (Exception $e) {
-/* NOP */
+                    /* NOP */
                 }
             }
 

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)

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()

tests/php/ORM/DataObjectTest/Team.php

@@ -80,4 +80,9 @@ class Team extends DataObject implements TestOnly
     {
         return 'dynamicfield';
     }
+
+    public function ReturnsNull()
+    {
+        return null;
+    }
 }