API: Allow for user-created objects to have values passed in the constructor (#8591)

src/Core/Injector/Injector.php

@@ -587,7 +587,7 @@ class Injector implements ContainerInterface
         if ((!$type || $type !== self::PROTOTYPE)
             && empty($constructorParams)
             && is_subclass_of($class, DataObject::class)) {
-            $constructorParams = [null, true];
+            $constructorParams = [null, DataObject::CREATE_SINGLETON];
         }
 
         $factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();

src/ORM/Connect/MySQLDatabase.php

@@ -293,7 +293,7 @@ class MySQLDatabase extends Database implements TransactionManager
         $objects = [];
 
         foreach ($records as $record) {
-            $objects[] = new $record['ClassName']($record);
+            $objects[] = new $record['ClassName']($record, DataObject::CREATE_HYDRATED);
         }
 
         $list = new PaginatedList(new ArrayList($objects));

src/ORM/DataList.php

@@ -819,6 +819,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
 
     /**
      * Create a DataObject from the given SQL row
+     * If called without $row['ID'] set, then a new object will be created rather than rehydrated.
      *
      * @param array $row
      * @return DataObject
@@ -841,7 +842,9 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
             $class = $row['RecordClassName'];
         }
 
-        $item = Injector::inst()->create($class, $row, false, $this->getQueryParams());
+        $creationType = empty($row['ID']) ? DataObject::CREATE_OBJECT : DataObject::CREATE_HYDRATED;
+
+        $item = Injector::inst()->create($class, $row, $creationType, $this->getQueryParams());
 
         return $item;
     }

src/ORM/DataObject.php

@@ -183,6 +183,25 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      */
     const CHANGE_VALUE = 2;
 
+    /**
+     * Value for 2nd argument to constructor, indicating that a new record is being created
+     * Setters will be called on fields passed, and defaults will be populated
+     */
+    const CREATE_OBJECT = 0;
+
+    /**
+     * Value for 2nd argument to constructor, indicating that a record is a singleton representing the whole type,
+     * e.g. to call requireTable() in dev/build
+     * Defaults will not be populated and data passed will be ignored
+     */
+    const CREATE_SINGLETON = 1;
+
+    /**
+     * Value for 2nd argument to constructor, indicating that a record is being hydrated from the database
+     * Neither setters and nor default population will be called
+     */
+    const CREATE_HYDRATED = 2;
+
     /**
      * An array indexed by fieldname, true if the field has been changed.
      * Use {@link getChangedFields()} and {@link isChanged()} to inspect
@@ -318,29 +337,63 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
     /**
      * Construct a new DataObject.
      *
-     * @param array|null $record Used internally for rehydrating an object from database content.
+     * @param array $record Initial record content, or rehydrated record content, depending on $creationType
-     *                           Bypasses setters on this class, and hence should not be used
+     * @param int|boolean $creationType Set to DataObject::CREATE_OBJECT, DataObject::CREATE_HYDRATED, or DataObject::CREATE_SINGLETON. Used by SilverStripe internals as best left as the default by regular users.
-     *                           for populating data on new records.
-     * @param boolean $isSingleton This this to true if this is a singleton() object, a stub for calling methods.
-     *                             Singletons don't have their defaults set.
      * @param array $queryParams List of DataQuery params necessary to lazy load, or load related objects.
      */
-    public function __construct($record = null, $isSingleton = false, $queryParams = [])
+    public function __construct($record = [], $creationType = self::CREATE_OBJECT, $queryParams = [])
     {
         parent::__construct();
 
+        // Legacy $record default
+        if ($record === null) {
+            $record = [];
+        }
+
+        // Legacy $isSingleton boolean
+        if (!is_int($creationType)) {
+            if (!is_bool($creationType)) {
+                user_error('Creation type is neither boolean (old isSingleton arg) nor integer (new arg), please review your code', E_USER_WARNING);
+            }
+            $creationType = $creationType ? self::CREATE_SINGLETON : self::CREATE_OBJECT;
+        }
+
         // Set query params on the DataObject to tell the lazy loading mechanism the context the object creation context
         $this->setSourceQueryParams($queryParams);
 
-        // Set the fields data.
+        // Set $this->record to $record, but ignore NULLs
-        if (!$record) {
+        $this->record = [];
-            $record = [
+
-                'ID' => 0,
+        switch ($creationType) {
-                'ClassName' => static::class,
+            // Hydrate a record from the database
-                'RecordClassName' => static::class
+            case self::CREATE_HYDRATED:
-            ];
+                if (!is_array($record) || empty($record['ID'])) {
+                    throw new \InvalidArgumentException("Hydrated records must be passed a record array including an ID");
                 }
 
+                $this->record = $record;
+
+                // Identify fields that should be lazy loaded, but only on existing records
+                // Get all field specs scoped to class for later lazy loading
+                $fields = static::getSchema()->fieldSpecs(
+                    static::class,
+                    DataObjectSchema::INCLUDE_CLASS | DataObjectSchema::DB_ONLY
+                );
+                foreach ($fields as $field => $fieldSpec) {
+                    $fieldClass = strtok($fieldSpec, ".");
+                    if (!array_key_exists($field, $record)) {
+                        $this->record[$field . '_Lazy'] = $fieldClass;
+                    }
+                }
+
+                $this->original = $this->record;
+                $this->changed = [];
+                $this->changeForced = false;
+
+                break;
+
+            // Create a new object, using the constructor argument as the initial content
+            case self::CREATE_OBJECT:
                 if ($record instanceof stdClass) {
                     $record = (array)$record;
                 }
@@ -357,49 +410,42 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
                         . " taken straight from the database.  Perhaps you should use DataList::create()->First(); instead?",
                         E_USER_WARNING
                     );
-            $record = null;
+                    $record = [];
-        }
-
-        // Set $this->record to $record, but ignore NULLs
-        $this->record = [];
-        foreach ($record as $k => $v) {
-            // Ensure that ID is stored as a number and not a string
-            // To do: this kind of clean-up should be done on all numeric fields, in some relatively
-            // performant manner
-            if ($v !== null) {
-                if ($k == 'ID' && is_numeric($v)) {
-                    $this->record[$k] = (int)$v;
-                } else {
-                    $this->record[$k] = $v;
-                }
-            }
-        }
-
-        // Identify fields that should be lazy loaded, but only on existing records
-        if (!empty($record['ID'])) {
-            // Get all field specs scoped to class for later lazy loading
-            $fields = static::getSchema()->fieldSpecs(
-                static::class,
-                DataObjectSchema::INCLUDE_CLASS | DataObjectSchema::DB_ONLY
-            );
-            foreach ($fields as $field => $fieldSpec) {
-                $fieldClass = strtok($fieldSpec, ".");
-                if (!array_key_exists($field, $record)) {
-                    $this->record[$field . '_Lazy'] = $fieldClass;
-                }
-            }
                 }
 
+                // Default columns
+                $this->record['ID'] = empty($record['ID']) ? 0 : $record['ID'];
+                $this->record['ClassName'] = static::class;
+                $this->record['RecordClassName'] = static::class;
+                unset($record['ID']);
                 $this->original = $this->record;
 
-        // Must be called after parent constructor
-        if (!$isSingleton && (!isset($this->record['ID']) || !$this->record['ID'])) {
                 $this->populateDefaults();
-        }
 
                 // prevent populateDefaults() and setField() from marking overwritten defaults as changed
                 $this->changed = [];
                 $this->changeForced = false;
+
+                // Set the data passed in the constructor, allowing for defaults and calling setters
+                // This will mark fields as changed
+                if ($record) {
+                    $this->update($record);
+                }
+                break;
+
+            case self::CREATE_SINGLETON:
+                // No setting happens for a singleton
+                $this->record['ID'] = 0;
+                $this->record['ClassName'] = static::class;
+                $this->record['RecordClassName'] = static::class;
+                $this->original = $this->record;
+                $this->changed = [];
+                $this->changeForced = false;
+                break;
+
+            default:
+                throw new \LogicException('Bad creationType ' . $this->creationType);
+        }
     }
 
     /**
@@ -703,7 +749,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         $originalClass = $this->ClassName;
 
         /** @var DataObject $newInstance */
-        $newInstance = Injector::inst()->create($newClassName, $this->record, false);
+        $newInstance = Injector::inst()->create($newClassName, $this->record, self::CREATE_HYDRATED);
 
         // Modify ClassName
         if ($newClassName != $originalClass) {
@@ -920,13 +966,18 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
     /**
      * Convert this object to a map.
+     * Note that it has the following quirks:
+     *  - custom getters, including those that adjust the result of database fields, won't be executed
+     *  - NULL values won't be returned.
      *
      * @return array The data as a map.
      */
     public function toMap()
     {
         $this->loadLazyFields();
-        return $this->record;
+        return array_filter($this->record, function ($val) {
+            return $val !== null;
+        });
     }
 
     /**

tests/php/Dev/SSListExporterTest.php

@@ -57,12 +57,13 @@ class SSListExporterTest extends SapphireTest
     public function testToMapReturnsDataOfDataObjects()
     {
         $data = [
+            'ID' => 5,
             'Foo' => 'Bar',
             'Baz' => 'Boom',
             'One' => 'Two'
         ];
 
-        $map = $this->exporter->toMap(DataObject::create($data));
+        $map = $this->exporter->toMap(DataObject::create($data, DataObject::CREATE_HYDRATED));
 
         $this->assertEquals($data, $map, 'Map should match data passed to DataObject');
     }

tests/php/ORM/DataObjectTest.php

@@ -206,8 +206,8 @@ class DataObjectTest extends SapphireTest
         $this->assertEquals('John', $player->FirstName);
         $this->assertEquals('Doe', $player->Surname);
 
-        // IDs should be stored as integers, not strings
+        // Note that automatic conversion of IDs to integer no longer happens as the DB layer does that for us now
-        $player = new DataObjectTest\Player(['ID' => '5']);
+        $player = new DataObjectTest\Player(['ID' => 5]);
         $this->assertSame(5, $player->ID);
     }
 
@@ -1306,6 +1306,7 @@ class DataObjectTest extends SapphireTest
             'HasOneRelationshipID',
             'ExtendedHasOneRelationshipID',
             'SubclassDatabaseField',
+            'SubclassFieldWithOverride',
             'ParentTeamID',
         ];
         $actual = array_keys($subteamSpecifications);
@@ -1321,6 +1322,7 @@ class DataObjectTest extends SapphireTest
         $expected = [
             'ID',
             'SubclassDatabaseField',
+            'SubclassFieldWithOverride',
             'ParentTeamID',
         ];
         $actual = array_keys($subteamFields);
@@ -2255,9 +2257,15 @@ class DataObjectTest extends SapphireTest
 
         $map = $obj->toMap();
 
-        $this->assertArrayHasKey('ID', $map, 'Contains base fields');
+        $this->assertArrayHasKey('ID', $map, 'Should contain ID');
-        $this->assertArrayHasKey('Title', $map, 'Contains fields from parent class');
+        $this->assertArrayHasKey('ClassName', $map, 'Should contain ClassName');
-        $this->assertArrayHasKey('SubclassDatabaseField', $map, 'Contains fields from concrete class');
+        $this->assertArrayHasKey('Created', $map, 'Should contain base Created');
+        $this->assertArrayHasKey('LastEdited', $map, 'Should contain base LastEdited');
+        $this->assertArrayHasKey('Title', $map, 'Should contain fields from parent class');
+        $this->assertArrayHasKey('SubclassDatabaseField', $map, 'Should contain fields from concrete class');
+
+        $this->assertEquals('DB value of SubclassFieldWithOverride (override)', $obj->SubclassFieldWithOverride, 'Object uses custom field getter');
+        $this->assertEquals('DB value of SubclassFieldWithOverride', $map['SubclassFieldWithOverride'], 'toMap does not use custom field getter');
 
         $this->assertEquals(
             $obj->ID,
@@ -2275,8 +2283,17 @@ class DataObjectTest extends SapphireTest
             'Contains values from concrete class fields'
         );
 
-        $newObj = new DataObjectTest\SubTeam();
+        $newObj = new DataObjectTest\SubTeam(['Title' => null]);
-        $this->assertArrayHasKey('Title', $map, 'Contains null fields');
+        $this->assertArrayNotHasKey('Title', $newObj->toMap(), 'Should not contain new null fields');
+
+        $newObj->Title = '';
+        $this->assertArrayHasKey('Title', $newObj->toMap(), 'Should contain fields once they are set, even if falsey');
+
+        $newObj->Title = null;
+        $this->assertArrayNotHasKey('Title', $newObj->toMap(), 'Should not contain reset-to-null fields');
+
+        $this->objFromFixture(DataObjectTest\SubTeam::class, 'subteam3_with_empty_fields');
+        $this->assertArrayNotHasKey('SubclassDatabaseField', $newObj->toMap(), 'Should not contain null re-hydrated fields');
     }
 
     public function testIsEmpty()
@@ -2520,4 +2537,39 @@ class DataObjectTest extends SapphireTest
             'Root is 2 step remove from grand children. It was not written on a shallow recursive write.'
         );
     }
+
+    /**
+     * Test the different methods for creating DataObjects.
+     * Note that using anything other than the default option should generally be left to ORM interanls.
+     */
+    public function testDataObjectCreationTypes()
+    {
+
+        // Test the default (DataObject::CREATE_OBJECT)
+        // Defaults are used, changes of non-default fields are tracked
+        $staff = new DataObjectTest\Staff([
+            'Salary' => 50,
+        ]);
+        $this->assertEquals('Staff', $staff->EmploymentType);
+        $this->assertEquals(['Salary'], array_keys($staff->getChangedFields()));
+
+
+        // Test hydration (DataObject::CREATE_HYDRATED)
+        // Defaults are not used, changes are not tracked
+        $staff = new DataObjectTest\Staff([
+            'ID' => 5,
+            'Salary' => 50,
+        ], DataObject::CREATE_HYDRATED);
+        $this->assertEquals(null, $staff->EmploymentType);
+        $this->assertEquals([], $staff->getChangedFields());
+
+        // Test singleton (DataObject::CREATE_SINGLETON)
+        // Values are ingored
+        $staff = new DataObjectTest\Staff([
+            'Salary' => 50,
+        ], DataObject::CREATE_SINGLETON);
+        $this->assertEquals(null, $staff->EmploymentType);
+        $this->assertEquals(null, $staff->Salary);
+        $this->assertEquals([], $staff->getChangedFields());
+    }
 }

tests/php/ORM/DataObjectTest.yml

@@ -70,6 +70,7 @@ SilverStripe\ORM\Tests\DataObjectTest\SubTeam:
   subteam1:
     Title: Subteam 1
     SubclassDatabaseField: Subclassed 1
+    SubclassFieldWithOverride: DB value of SubclassFieldWithOverride
     ExtendedDatabaseField: Extended 1
     ParentTeam: =>SilverStripe\ORM\Tests\DataObjectTest\Team.team1
     Sponsors:

tests/php/ORM/DataObjectTest/SubTeam.php

@@ -9,7 +9,8 @@ class SubTeam extends Team implements TestOnly
     private static $table_name = 'DataObjectTest_SubTeam';
 
     private static $db = [
-        'SubclassDatabaseField' => 'Varchar'
+        'SubclassDatabaseField' => 'Varchar',
+        'SubclassFieldWithOverride' => 'Varchar',
     ];
 
     private static $has_one = [
@@ -25,4 +26,13 @@ class SubTeam extends Team implements TestOnly
             'Position' => 'Varchar(100)'
         ]
     ];
+
+    /**
+     * Override the value of SubclassFieldWithOverride
+     * @return string Suffixes " (override)" to SubclassFieldWithOverride value
+     */
+    public function getSubclassFieldWithOverride()
+    {
+        return $this->getField('SubclassFieldWithOverride') . ' (override)';
+    }
 }