Merge pull request #8453 from sminnee/less-original

FIX: Update “original” DataObject data to be the content of the last write

src/ORM/DataObject.php

@@ -188,14 +188,23 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      *
      * @var array
      */
-    private $changed;
+    private $changed = [];
+
+    /**
+     * A flag to indicate that a "strict" change of the entire record been forced
+     * Use {@link getChangedFields()} and {@link isChanged()} to inspect
+     * the changed state.
+     *
+     * @var boolean
+     */
+    private $changeForced = false;
 
     /**
      * The database record (in the same format as $record), before
      * any changes.
      * @var array
      */
-    protected $original;
+    protected $original = [];
 
     /**
      * Used by onBeforeDelete() to ensure child classes call parent::onBeforeDelete()
@@ -387,7 +396,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         }
 
         // prevent populateDefaults() and setField() from marking overwritten defaults as changed
-        $this->changed = array();
+        $this->changed = [];
+        $this->changeForced = false;
     }
 
     /**
@@ -1107,8 +1117,9 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
     /**
      * Forces the record to think that all its data has changed.
-     * Doesn't write to the database. Only sets fields as changed
+     * Doesn't write to the database. Force-change preseved until
-     * if they are not already marked as changed.
+     * next write. Existing CHANGE_VALUE or CHANGE_STRICT values
+     * are preserved.
      *
      * @return $this
      */
@@ -1116,28 +1127,16 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
     {
         // Ensure lazy fields loaded
         $this->loadLazyFields();
-        $fields = static::getSchema()->fieldSpecs(static::class);
 
-        // $this->record might not contain the blank values so we loop on $this->inheritedDatabaseFields() as well
+        // Populate the null values in record so that they actually get written
-        $fieldNames = array_unique(array_merge(
+        foreach (array_keys(static::getSchema()->fieldSpecs(static::class)) as $fieldName) {
-            array_keys($this->record),
-            array_keys($fields)
-        ));
-
-        foreach ($fieldNames as $fieldName) {
-            if (!isset($this->changed[$fieldName])) {
-                $this->changed[$fieldName] = self::CHANGE_STRICT;
-            }
-            // Populate the null values in record so that they actually get written
             if (!isset($this->record[$fieldName])) {
                 $this->record[$fieldName] = null;
             }
         }
 
-        // @todo Find better way to allow versioned to write a new version after forceChange
+        $this->changeForced = true;
-        if ($this->isChanged('Version')) {
+
-            unset($this->changed['Version']);
-        }
         return $this;
     }
 
@@ -1374,11 +1373,13 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         $table = $schema->tableName($class);
         $manipulation[$table] = array();
 
+        $changed = $this->getChangedFields();
+
         // Extract records for this table
         foreach ($this->record as $fieldName => $fieldValue) {
             // we're not attempting to reset the BaseTable->ID
             // Ignore unchanged fields or attempts to reset the BaseTable->ID
-            if (empty($this->changed[$fieldName]) || ($table === $baseTable && $fieldName === 'ID')) {
+            if (empty($changed[$fieldName]) || ($table === $baseTable && $fieldName === 'ID')) {
                 continue;
             }
 
@@ -1520,7 +1521,12 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             // If there's any relations that couldn't be saved before, save them now (we have an ID here)
             $this->writeRelations();
             $this->onAfterWrite();
-            $this->changed = array();
+
+            // Reset isChanged data
+            // DBComposites properly bound to the parent record will also have their isChanged value reset
+            $this->changed = [];
+            $this->changeForced = false;
+            $this->original = $this->record;
         } else {
             if ($showDebug) {
                 Debug::message("no changes for DataObject");
@@ -2487,7 +2493,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
     }
 
     /**
-     * Return the fields that have changed.
+     * Return the fields that have changed since the last write.
      *
      * The change level affects what the functions defines as "changed":
      * - Level CHANGE_STRICT (integer 1) will return strict changes, even !== ones.
@@ -2521,13 +2527,25 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             }
         }
 
+        // If change was forced, then derive change data from $this->record
+        if ($this->changeForced && $changeLevel <= self::CHANGE_STRICT) {
+            $changed = array_combine(
+                array_keys($this->record),
+                array_fill(0, count($this->record), self::CHANGE_STRICT)
+            );
+            // @todo Find better way to allow versioned to write a new version after forceChange
+            unset($changed['Version']);
+        } else {
+            $changed = $this->changed;
+        }
+
         if (is_array($databaseFieldsOnly)) {
-            $fields = array_intersect_key((array)$this->changed, array_flip($databaseFieldsOnly));
+            $fields = array_intersect_key($changed, array_flip($databaseFieldsOnly));
         } elseif ($databaseFieldsOnly) {
             $fieldsSpecs = static::getSchema()->fieldSpecs(static::class);
-            $fields = array_intersect_key((array)$this->changed, $fieldsSpecs);
+            $fields = array_intersect_key($changed, $fieldsSpecs);
         } else {
-            $fields = $this->changed;
+            $fields = $changed;
         }
 
         // Filter the list to those of a certain change level
@@ -2628,22 +2646,25 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             }
 
             // if a field is not existing or has strictly changed
-            if (!isset($this->record[$fieldName]) || $this->record[$fieldName] !== $val) {
+            if (!isset($this->original[$fieldName]) || $this->original[$fieldName] !== $val) {
                 // TODO Add check for php-level defaults which are not set in the db
                 // TODO Add check for hidden input-fields (readonly) which are not set in the db
                 // At the very least, the type has changed
                 $this->changed[$fieldName] = self::CHANGE_STRICT;
 
-                if ((!isset($this->record[$fieldName]) && $val)
+                if ((!isset($this->original[$fieldName]) && $val)
-                    || (isset($this->record[$fieldName]) && $this->record[$fieldName] != $val)
+                    || (isset($this->original[$fieldName]) && $this->original[$fieldName] != $val)
                 ) {
                     // Value has changed as well, not just the type
                     $this->changed[$fieldName] = self::CHANGE_VALUE;
                 }
-
+            // Value has been restored to its original, remove any record of the change
-                // Value is always saved back when strict check succeeds.
+            } elseif (isset($this->changed[$fieldName])) {
-                $this->record[$fieldName] = $val;
+                unset($this->changed[$fieldName]);
             }
+
+            // Value is saved regardless, since the change detection relates to the last write
+            $this->record[$fieldName] = $val;
         }
         return $this;
     }

src/ORM/FieldType/DBComposite.php

@@ -36,6 +36,12 @@ abstract class DBComposite extends DBField
      */
     private static $composite_db = array();
 
+    /**
+     * Marker as to whether this record has changed
+     * Only used when deference to the parent object isn't possible
+     */
+    protected $isChanged = false;
+
     /**
      * Either the parent dataobject link, or a record of saved values for each field
      *
@@ -113,6 +119,10 @@ abstract class DBComposite extends DBField
     }
 
 
+    /**
+     * Returns true if this composite field has changed.
+     * For fields bound to a DataObject, this will be cleared when the DataObject is written.
+     */
     public function isChanged()
     {
         // When unbound, use the local changed flag

tests/php/ORM/DataObjectTest.php

@@ -765,6 +765,75 @@ class DataObjectTest extends SapphireTest
         );
     }
 
+    public function testChangedFieldsWhenRestoringData()
+    {
+        $obj = $this->objFromFixture(DataObjectTest\Player::class, 'captain1');
+        $obj->FirstName = 'Captain-changed';
+        $obj->FirstName = 'Captain';
+
+        $this->assertEquals(
+            [],
+            $obj->getChangedFields(true, DataObject::CHANGE_STRICT)
+        );
+    }
+
+    public function testChangedFieldsAfterWrite()
+    {
+        $obj = $this->objFromFixture(DataObjectTest\Player::class, 'captain1');
+        $obj->FirstName = 'Captain-changed';
+        $obj->write();
+        $obj->FirstName = 'Captain';
+
+        $this->assertEquals(
+            array(
+                'FirstName' => array(
+                    'before' => 'Captain-changed',
+                    'after' => 'Captain',
+                    'level' => DataObject::CHANGE_VALUE,
+                ),
+            ),
+            $obj->getChangedFields(true, DataObject::CHANGE_VALUE)
+        );
+    }
+
+    public function testForceChangeCantBeCancelledUntilWrite()
+    {
+        $obj = $this->objFromFixture(DataObjectTest\Player::class, 'captain1');
+        $this->assertFalse($obj->isChanged('FirstName'));
+        $this->assertFalse($obj->isChanged('Surname'));
+
+        // Force change marks the records as changed
+        $obj->forceChange();
+        $this->assertTrue($obj->isChanged('FirstName'));
+        $this->assertTrue($obj->isChanged('Surname'));
+
+        // ...but not if we explicitly ask if the value has changed
+        $this->assertFalse($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
+        $this->assertFalse($obj->isChanged('Surname', DataObject::CHANGE_VALUE));
+
+        // Not overwritten by setting the value to is original value
+        $obj->FirstName = 'Captain';
+        $this->assertTrue($obj->isChanged('FirstName'));
+        $this->assertTrue($obj->isChanged('Surname'));
+
+        // Not overwritten by changing it to something else and back again
+        $obj->FirstName = 'Captain-changed';
+        $this->assertTrue($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
+
+        $obj->FirstName = 'Captain';
+        $this->assertFalse($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
+        $this->assertTrue($obj->isChanged('FirstName'));
+        $this->assertTrue($obj->isChanged('Surname'));
+
+        // Cleared after write
+        $obj->write();
+        $this->assertFalse($obj->isChanged('FirstName'));
+        $this->assertFalse($obj->isChanged('Surname'));
+
+        $obj->FirstName = 'Captain';
+        $this->assertFalse($obj->isChanged('FirstName'));
+    }
+
     /**
      * @skipUpgrade
      */

tests/php/Security/MemberTest.php

@@ -655,10 +655,14 @@ class MemberTest extends FunctionalTest
         $member = $this->objFromFixture(Member::class, 'test');
         $member->setName('Test Some User');
         $this->assertEquals('Test Some User', $member->getName());
+        $this->assertEquals('Test Some', $member->FirstName);
+        $this->assertEquals('User', $member->Surname);
         $member->setName('Test');
         $this->assertEquals('Test', $member->getName());
         $member->FirstName = 'Test';
         $member->Surname = '';
+        $this->assertEquals('Test', $member->FirstName);
+        $this->assertEquals('', $member->Surname);
         $this->assertEquals('Test', $member->getName());
     }