Skip to content

Commit

Permalink
Merge pull request #8453 from sminnee/less-original
Browse files Browse the repository at this point in the history 
FIX: Update “original” DataObject data to be the content of the last write
  • Loading branch information
@kinglozzer
kinglozzer authored Nov 6, 2018
2 parents b835b77 + 67fe41d commit 0e492b0
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 33 deletions.
87 changes: 54 additions & 33 deletions src/ORM/DataObject.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -387,7 +396,8 @@ public function __construct($record = null, $isSingleton = false, $queryParams =
}

// prevent populateDefaults() and setField() from marking overwritten defaults as changed
$this->changed = array();
$this->changed = [];
$this->changeForced = false;
}

/**
Expand Down Expand Up @@ -1107,37 +1117,26 @@ public function merge($rightObj, $priority = 'right', $includeRelations = true,

/**
* 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
*/
public function forceChange()
{
// 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_STRICT;
}
// 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;
}

Expand Down Expand Up @@ -1374,11 +1373,13 @@ protected function prepareManipulationTable($baseTable, $now, $isNewRecord, &$ma
$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;
}

Expand Down Expand Up @@ -1520,7 +1521,12 @@ public function write($showDebug = false, $forceInsert = false, $forceWrite = fa
// 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");
Expand Down Expand Up @@ -2487,7 +2493,7 @@ protected function loadLazyFields($class = null)
}

/**
* 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.
Expand Down Expand Up @@ -2521,13 +2527,25 @@ public function getChangedFields($databaseFieldsOnly = false, $changeLevel = sel
}
}

// 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
Expand Down Expand Up @@ -2628,22 +2646,25 @@ public function setField($fieldName, $val)
}

// 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])) {
unset($this->changed[$fieldName]);
}

// Value is saved regardless, since the change detection relates to the last write
$this->record[$fieldName] = $val;
}
return $this;
}
Expand Down
10 changes: 10 additions & 0 deletions src/ORM/FieldType/DBComposite.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -113,6 +119,10 @@ public function compositeDatabaseFields()
}


/**
* 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
Expand Down
69 changes: 69 additions & 0 deletions tests/php/ORM/DataObjectTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,75 @@ public function testChangedFields()
);
}

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
*/
Expand Down
4 changes: 4 additions & 0 deletions tests/php/Security/MemberTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,10 +655,14 @@ public function testName()
$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());
}

Expand Down

0 comments on commit 0e492b0

Please sign in to comment.