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

FIX: Compare to original when determining fields changes

This fixes a number of edge-case issues relating to change detection.

Fixes #8443
Fixes #3821
Fixes #4561

src/ORM/DataObject.php

@@ -166,6 +166,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      */
     const CHANGE_NONE = 0;
 
+    /**
+     * Represents a field that has had a change forced
+     */
+    const CHANGE_FORCED = 0.5;
+
     /**
      * Represents a field that has changed type, although not the loosely defined value.
      * (before !== after && before == after)
@@ -1126,7 +1131,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
         foreach ($fieldNames as $fieldName) {
             if (!isset($this->changed[$fieldName])) {
-                $this->changed[$fieldName] = self::CHANGE_STRICT;
+                $this->changed[$fieldName] = self::CHANGE_FORCED;
             }
             // Populate the null values in record so that they actually get written
             if (!isset($this->record[$fieldName])) {
@@ -1520,7 +1525,11 @@ 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->original = $this->record;
         } else {
             if ($showDebug) {
                 Debug::message("no changes for DataObject");
@@ -2487,7 +2496,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.
@@ -2628,22 +2637,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->changed[$fieldName] !== self::CHANGE_FORCED) {
-                $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,62 @@ 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'));
+
+        $obj->forceChange();
+        $this->assertTrue($obj->isChanged('FirstName'));
+        $this->assertTrue($obj->isChanged('Surname'));
+
+        $this->assertFalse($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
+        $this->assertFalse($obj->isChanged('Surname', DataObject::CHANGE_VALUE));
+
+        $obj->FirstName = 'Captain';
+        $this->assertTrue($obj->isChanged('FirstName'));
+        $this->assertTrue($obj->isChanged('Surname'));
+
+        $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());
     }