tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 12:05:37 +00:00
Code Issues Projects Releases Wiki Activity

BUG Fix DataObject::isChanged() detecting non saveable changes (#5545)

Browse Source
This commit is contained in:
Damian Mooyman 2016-05-18 11:00:04 +12:00 committed by Sam Minnée
parent 8947bb0245
commit 8ed25ae482
5 changed files with 114 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/en/04_Changelogs/3.4.0.md
View File

@ -14,3 +14,5 @@ was affected by these:
* When FormFields are rendered, leading & trailing whitespace is now stripped. The resulting HTML for form fields is
the same for the default fields, but if you have a custom form field that is relying on trailing whitespace being
outputted.
* DataObject::isChanged() now defaults to only checking database fields. If you rely on this method
for checking changes to non-db field properties, use getChangedFields() instead.

68
model/DataObject.php
View File

@ -1033,7 +1033,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
* Doesn't write to the database. Only sets fields as changed
* if they are not already marked as changed.
*
* @return DataObject $this
* @return $this
*/
public function forceChange() {
// Ensure lazy fields loaded
@ -1042,7 +1042,8 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
// $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($this->inheritedDatabaseFields())));
array_keys($this->inheritedDatabaseFields())
));
foreach($fieldNames as $fieldName) {
if(!isset($this->changed[$fieldName])) $this->changed[$fieldName] = self::CHANGE_STRICT;
@ -1227,22 +1228,16 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
* @param bool $forceChanges If set to true, force all fields to be treated as changed
* @return bool True if any changes are detected
*/
protected function updateChanges($forceChanges = false) {
// Update the changed array with references to changed obj-fields
foreach($this->record as $field => $value) {
// Only mark ID as changed if $forceChanges
if($field === 'ID' && !$forceChanges) continue;
// Determine if this field should be forced, or can mark itself, changed
if($forceChanges
|| !$this->isInDB()
|| (is_object($value) && method_exists($value, 'isChanged') && $value->isChanged())
) {
$this->changed[$field] = self::CHANGE_VALUE;
protected function updateChanges($forceChanges = false)
{
if($forceChanges) {
// Force changes, but only for loaded fields
foreach($this->record as $field => $value) {
$this->changed[$field] = static::CHANGE_VALUE;
}
return true;
}
// Check changes exist, abort if there are no changes
return $this->changed && (bool)array_filter($this->changed);
return $this->isChanged();
}
/**
@ -1378,7 +1373,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
$isNewRecord = !$this->isInDB() || $forceInsert;
// Check changes exist, abort if there are none
$hasChanges = $this->updateChanges($forceInsert);
$hasChanges = $this->updateChanges($isNewRecord);
if($hasChanges || $forceWrite || $isNewRecord) {
// New records have their insert into the base data table done first, so that they can pass the
// generated primary key on to the rest of the manipulation
@ -2451,10 +2446,15 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
/**
* Loads all the stub fields that an initial lazy load didn't load fully.
*
* @param tableClass Base table to load the values from. Others are joined as required.
* Not specifying a tableClass will load all lazy fields from all tables.
* @param string $tableClass Base table to load the values from. Others are joined as required.
* Not specifying a tableClass will load all lazy fields from all tables.
* @return bool Flag if lazy loading succeeded
*/
protected function loadLazyFields($tableClass = null) {
if(!$this->isInDB() || !is_numeric($this->ID)) {
return false;
}
if (!$tableClass) {
$loaded = array();
@ -2465,7 +2465,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
}
}
return;
return false;
}
$dataQuery = new DataQuery($tableClass);
@ -2475,9 +2475,6 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
foreach($params as $key => $value) $dataQuery->setQueryParam($key, $value);
}
// TableField sets the record ID to "new" on new row data, so don't try doing anything in that case
if(!is_numeric($this->record['ID'])) return false;
// Limit query to the current record, unless it has the Versioned extension,
// in which case it requires special handling through augmentLoadLazyFields()
if(!$this->hasExtension('Versioned')) {
@ -2522,6 +2519,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
}
}
}
return true;
}
/**
@ -2554,11 +2552,15 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
}
if($databaseFieldsOnly) {
$databaseFields = $this->inheritedDatabaseFields();
$databaseFields['ID'] = true;
$databaseFields['LastEdited'] = true;
$databaseFields['Created'] = true;
$databaseFields['ClassName'] = true;
// Merge all DB fields together
$inheritedFields = $this->inheritedDatabaseFields();
$compositeFields = static::composite_fields(get_class($this));
$fixedFields = $this->config()->fixed_fields;
$databaseFields = array_merge(
$inheritedFields,
$fixedFields,
$compositeFields
);
$fields = array_intersect_key((array)$this->changed, $databaseFields);
} else {
$fields = $this->changed;
@ -2593,11 +2595,13 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
* @return boolean
*/
public function isChanged($fieldName = null, $changeLevel = self::CHANGE_STRICT) {
$changed = $this->getChangedFields(false, $changeLevel);
if(!isset($fieldName)) {
if (!$fieldName) {
// Limit "any changes" to db fields only
$changed = $this->getChangedFields(true, $changeLevel);
return !empty($changed);
}
else {
} else {
// Given a field name, check all fields
$changed = $this->getChangedFields(false, $changeLevel);
return array_key_exists($fieldName, $changed);
}
}

62
tests/model/DataObjectTest.php
View File

@ -533,29 +533,29 @@ class DataObjectTest extends SapphireTest {
$obj->IsRetired = true;
$this->assertEquals(
$obj->getChangedFields(false, 1),
$obj->getChangedFields(true, DataObject::CHANGE_STRICT),
array(
'FirstName' => array(
'before' => 'Captain',
'after' => 'Captain-changed',
'level' => 2
'level' => DataObject::CHANGE_VALUE
),
'IsRetired' => array(
'before' => 1,
'after' => true,
'level' => 1
'level' => DataObject::CHANGE_STRICT
)
),
'Changed fields are correctly detected with strict type changes (level=1)'
);
$this->assertEquals(
$obj->getChangedFields(false, 2),
$obj->getChangedFields(true, DataObject::CHANGE_VALUE),
array(
'FirstName' => array(
'before'=>'Captain',
'after'=>'Captain-changed',
'level' => 2
'level' => DataObject::CHANGE_VALUE
)
),
'Changed fields are correctly detected while ignoring type changes (level=2)'
@ -564,50 +564,58 @@ class DataObjectTest extends SapphireTest {
$newObj = new DataObjectTest_Player();
$newObj->FirstName = "New Player";
$this->assertEquals(
$newObj->getChangedFields(false, 2),
array(
'FirstName' => array(
'before' => null,
'after' => 'New Player',
'level' => 2
'level' => DataObject::CHANGE_VALUE
)
),
$newObj->getChangedFields(true, DataObject::CHANGE_VALUE),
'Initialised fields are correctly detected as full changes'
);
}
public function testIsChanged() {
$obj = $this->objFromFixture('DataObjectTest_Player', 'captain1');
$obj->NonDBField = 'bob';
$obj->FirstName = 'Captain-changed';
$obj->IsRetired = true; // type change only, database stores "1"
$this->assertTrue($obj->isChanged('FirstName', 1));
$this->assertTrue($obj->isChanged('FirstName', 2));
$this->assertTrue($obj->isChanged('IsRetired', 1));
$this->assertFalse($obj->isChanged('IsRetired', 2));
// Now that DB fields are changed, isChanged is true
$this->assertTrue($obj->isChanged('NonDBField'));
$this->assertFalse($obj->isChanged('NonField'));
$this->assertTrue($obj->isChanged('FirstName', DataObject::CHANGE_STRICT));
$this->assertTrue($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
$this->assertTrue($obj->isChanged('IsRetired', DataObject::CHANGE_STRICT));
$this->assertFalse($obj->isChanged('IsRetired', DataObject::CHANGE_VALUE));
$this->assertFalse($obj->isChanged('Email', 1), 'Doesnt change mark unchanged property');
$this->assertFalse($obj->isChanged('Email', 2), 'Doesnt change mark unchanged property');
$newObj = new DataObjectTest_Player();
$newObj->FirstName = "New Player";
$this->assertTrue($newObj->isChanged('FirstName', 1));
$this->assertTrue($newObj->isChanged('FirstName', 2));
$this->assertFalse($newObj->isChanged('Email', 1));
$this->assertFalse($newObj->isChanged('Email', 2));
$this->assertTrue($newObj->isChanged('FirstName', DataObject::CHANGE_STRICT));
$this->assertTrue($newObj->isChanged('FirstName', DataObject::CHANGE_VALUE));
$this->assertFalse($newObj->isChanged('Email', DataObject::CHANGE_STRICT));
$this->assertFalse($newObj->isChanged('Email', DataObject::CHANGE_VALUE));
$newObj->write();
$this->assertFalse($newObj->isChanged('FirstName', 1));
$this->assertFalse($newObj->isChanged('FirstName', 2));
$this->assertFalse($newObj->isChanged('Email', 1));
$this->assertFalse($newObj->isChanged('Email', 2));
$this->assertFalse($newObj->ischanged());
$this->assertFalse($newObj->isChanged('FirstName', DataObject::CHANGE_STRICT));
$this->assertFalse($newObj->isChanged('FirstName', DataObject::CHANGE_VALUE));
$this->assertFalse($newObj->isChanged('Email', DataObject::CHANGE_STRICT));
$this->assertFalse($newObj->isChanged('Email', DataObject::CHANGE_VALUE));
$obj = $this->objFromFixture('DataObjectTest_Player', 'captain1');
$obj->FirstName = null;
$this->assertTrue($obj->isChanged('FirstName', 1));
$this->assertTrue($obj->isChanged('FirstName', 2));
$this->assertTrue($obj->isChanged('FirstName', DataObject::CHANGE_STRICT));
$this->assertTrue($obj->isChanged('FirstName', DataObject::CHANGE_VALUE));
/* Test when there's not field provided */
$obj = $this->objFromFixture('DataObjectTest_Player', 'captain1');
$obj = $this->objFromFixture('DataObjectTest_Player', 'captain2');
$this->assertFalse($obj->isChanged());
$obj->NonDBField = 'new value';
$this->assertFalse($obj->isChanged());
$obj->FirstName = "New Player";
$this->assertTrue($obj->isChanged());
@ -1109,7 +1117,7 @@ class DataObjectTest extends SapphireTest {
public function testValidateModelDefinitionsFailsWithArray() {
Config::nest();
$object = new DataObjectTest_Team;
$method = $this->makeAccessible($object, 'validateModelDefinitions');
@ -1126,7 +1134,7 @@ class DataObjectTest extends SapphireTest {
public function testValidateModelDefinitionsFailsWithIntKey() {
Config::nest();
$object = new DataObjectTest_Team;
$method = $this->makeAccessible($object, 'validateModelDefinitions');
@ -1143,7 +1151,7 @@ class DataObjectTest extends SapphireTest {
public function testValidateModelDefinitionsFailsWithIntValue() {
Config::nest();
$object = new DataObjectTest_Team;
$method = $this->makeAccessible($object, 'validateModelDefinitions');
@ -1163,7 +1171,7 @@ class DataObjectTest extends SapphireTest {
*/
public function testValidateModelDefinitionsPassesWithExtraFields() {
Config::nest();
$object = new DataObjectTest_Team;
$method = $this->makeAccessible($object, 'validateModelDefinitions');
@ -1810,7 +1818,7 @@ class DataObjectTest_SubTeam extends DataObjectTest_Team implements TestOnly {
private static $many_many = array(
'FormerPlayers' => 'DataObjectTest_Player'
);
private static $many_many_extraFields = array(
'FormerPlayers' => array(
'Position' => 'Varchar(100)'

29
tests/model/MoneyTest.php
View File

@ -71,6 +71,35 @@ class MoneyTest extends SapphireTest {
$this->assertEquals(0.0000, $moneyTest->MyMoneyAmount);
}
public function testIsChanged() {
$obj1 = $this->objFromFixture('MoneyTest_DataObject', 'test1');
$this->assertFalse($obj1->isChanged());
$this->assertFalse($obj1->isChanged('MyMoney'));
// modify non-db field
$m1 = new Money();
$m1->setAmount(500);
$m1->setCurrency('NZD');
$obj1->NonDBMoneyField = $m1;
$this->assertFalse($obj1->isChanged()); // Because only detects DB fields
$this->assertTrue($obj1->isChanged('NonDBMoneyField')); // Allow change detection to non-db fields explicitly named
// Modify db field
$obj2 = $this->objFromFixture('MoneyTest_DataObject', 'test2');
$m2 = new Money();
$m2->setAmount(500);
$m2->setCurrency('NZD');
$obj2->MyMoney = $m2;
$this->assertTrue($obj2->isChanged()); // Detects change to DB field
$this->assertTrue($obj2->ischanged('MyMoney'));
// Modify sub-fields
$obj3 = $this->objFromFixture('MoneyTest_DataObject', 'test3');
$obj3->MyMoneyCurrency = 'USD';
$this->assertTrue($obj3->isChanged()); // Detects change to DB field
$this->assertTrue($obj3->ischanged('MyMoneyCurrency'));
}
/**
* Write a Money object to the database, then re-read it to ensure it
* is re-read properly.

18
tests/model/MoneyTest.yml
View File

@ -1,8 +1,14 @@
MoneyTest_DataObject:
test1:
MyMoneyCurrency: EUR
MyMoneyAmount: 1.23
test1:
MyMoneyCurrency: EUR
MyMoneyAmount: 1.23
test2:
MyMoneyCurrency: USD
MyMoneyAmount: 4.45
test3:
MyMoneyCurrency: NZD
MyMoneyAmount: 7.66
MoneyTest_SubClass:
test2:
MyOtherMoneyCurrency: GBP
MyOtherMoneyAmount: 2.46
test2:
MyOtherMoneyCurrency: GBP
MyOtherMoneyAmount: 2.46