From 5bb2d9484a203345964f75e6480bbfd5388aa824 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 5 Oct 2018 11:09:00 +1300 Subject: [PATCH 1/2] =?UTF-8?q?FIX:=20Update=20=E2=80=9Coriginal=E2=80=9D?= =?UTF-8?q?=20DataObject=20data=20to=20be=20the=20content=20of=20the=20las?= =?UTF-8?q?t=20write=20FIX:=20Compare=20to=20original=20when=20determining?= =?UTF-8?q?=20fields=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a number of edge-case issues relating to change detection. Fixes #8443 Fixes #3821 Fixes #4561 --- src/ORM/DataObject.php | 30 ++++++++++++----- src/ORM/FieldType/DBComposite.php | 10 ++++++ tests/php/ORM/DataObjectTest.php | 56 +++++++++++++++++++++++++++++++ tests/php/Security/MemberTest.php | 4 +++ 4 files changed, 91 insertions(+), 9 deletions(-) diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index a47034381..028025551 100644 --- a/src/ORM/DataObject.php +++ b/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) - || (isset($this->record[$fieldName]) && $this->record[$fieldName] != $val) + if ((!isset($this->original[$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 is always saved back when strict check succeeds. - $this->record[$fieldName] = $val; + // Value has been restored to its original, remove any record of the change + } elseif (isset($this->changed[$fieldName]) && $this->changed[$fieldName] !== self::CHANGE_FORCED) { + unset($this->changed[$fieldName]); } + + // Value is saved regardless, since the change detection relates to the last write + $this->record[$fieldName] = $val; } return $this; } diff --git a/src/ORM/FieldType/DBComposite.php b/src/ORM/FieldType/DBComposite.php index 781cd200c..844b00afa 100644 --- a/src/ORM/FieldType/DBComposite.php +++ b/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 diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index aee0b8d1c..d34373b63 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/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 */ diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php index bdf664c1f..4b5341622 100644 --- a/tests/php/Security/MemberTest.php +++ b/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()); } From 67fe41d00b3f58358a4bed7b8925bd21dc88726e Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Mon, 8 Oct 2018 14:55:02 +1300 Subject: [PATCH 2/2] =?UTF-8?q?FIX:=20Ensure=20that=20repeated=20setting/u?= =?UTF-8?q?nsetting=20doesn=E2=80=99t=20corrode=20forceChange()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ORM/DataObject.php | 71 ++++++++++++++++++-------------- tests/php/ORM/DataObjectTest.php | 13 ++++++ 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 028025551..3fbf3afa4 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -166,11 +166,6 @@ 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) @@ -193,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() @@ -392,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; } /** @@ -1112,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 - * if they are not already marked as changed. + * Doesn't write to the database. Force-change preseved until + * next write. Existing CHANGE_VALUE or CHANGE_STRICT values + * are preserved. * * @return $this */ @@ -1121,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 - $fieldNames = array_unique(array_merge( - array_keys($this->record), - array_keys($fields) - )); - - foreach ($fieldNames as $fieldName) { - if (!isset($this->changed[$fieldName])) { - $this->changed[$fieldName] = self::CHANGE_FORCED; - } - // Populate the null values in record so that they actually get written + // Populate the null values in record so that they actually get written + foreach (array_keys(static::getSchema()->fieldSpecs(static::class)) as $fieldName) { if (!isset($this->record[$fieldName])) { $this->record[$fieldName] = null; } } - // @todo Find better way to allow versioned to write a new version after forceChange - if ($this->isChanged('Version')) { - unset($this->changed['Version']); - } + $this->changeForced = true; + return $this; } @@ -1379,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; } @@ -1529,6 +1525,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // 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) { @@ -2530,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 @@ -2650,7 +2659,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $this->changed[$fieldName] = self::CHANGE_VALUE; } // Value has been restored to its original, remove any record of the change - } elseif (isset($this->changed[$fieldName]) && $this->changed[$fieldName] !== self::CHANGE_FORCED) { + } elseif (isset($this->changed[$fieldName])) { unset($this->changed[$fieldName]); } diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index d34373b63..1ab89ee32 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -802,17 +802,30 @@ class DataObjectTest extends SapphireTest $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'));