From dfa44c055c620d16a74411f346d580be3332f549 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 27 May 2009 00:09:23 +0000 Subject: [PATCH] API CHANGE Changing DataObject::$changed to private visiblity. Please use getChangedFields() and isChanged() ENHANCEMENT Added DataObject->isChanged() to detect if a field has been changed in this object instance MINOR Changing call to CompositeDBField->compositeDatabaseFields() in DataObject->hasOwnDatabaseField() BUGFIX Unsettig "Version" property in DataObject->getChangedField() to allow versioned to write a new version after a call to forceChange() BUGFIX Introduced $markChanged in Money class BUGFIX Casting Money->__toString() return value as string MINOR Changing Member class to use new DataObject->isChanged() API BUGFIX Using new $markChanged API for CompositeDBFields in DBField::create() git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@77893 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- core/model/DataObject.php | 38 +++++++++++++++++++++++-------- core/model/fieldtypes/DBField.php | 2 +- core/model/fieldtypes/Money.php | 37 +++++++++++++++--------------- security/Member.php | 18 ++++++--------- tests/DataObjectTest.php | 31 +++++++++++++++++++++++++ tests/model/MoneyTest.php | 7 ++++++ tests/model/VersionedTest.php | 12 ++++++++++ 7 files changed, 106 insertions(+), 39 deletions(-) diff --git a/core/model/DataObject.php b/core/model/DataObject.php index bc6a59709..45b1df16b 100644 --- a/core/model/DataObject.php +++ b/core/model/DataObject.php @@ -105,9 +105,12 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity /** * An array indexed by fieldname, true if the field has been changed. + * Use {@link getChangedFields()} and {@link isChanged()} to inspect + * the changed state. + * * @var array */ - protected $changed; + private $changed; /** * The database record (in the same format as $record), before @@ -660,6 +663,9 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity foreach($this->record as $fieldName => $fieldVal) { if(!isset($this->changed[$fieldName])) $this->changed[$fieldName] = 1; } + + // @todo Find better way to allow versioned to write a new version after forceChange + if($this->isChanged('Version')) unset($this->changed['Version']); } /** @@ -848,7 +854,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity break; } } - + if($hasChanges || $forceWrite || !$this->record['ID']) { // New records have their insert into the base data table done first, so that they can pass the @@ -1806,7 +1812,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // write value only if either the field value exists, // or a valid record has been loaded from the database $value = (isset($this->record[$field])) ? $this->record[$field] : null; - if($value || $this->exists()) $fieldObj->setValue($value, $this->record); + if($value || $this->exists()) $fieldObj->setValue($value, $this->record, false); $this->record[$field] = $fieldObj; @@ -1827,9 +1833,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity /** * Return the fields that have changed. + * * The change level affects what the functions defines as "changed": - * Level 1 will return strict changes, even !== ones. - * Level 2 is more lenient, it will onlr return real data changes, for example a change from 0 to null + * - Level 1 will return strict changes, even !== ones. + * - Level 2 is more lenient, it will only return real data changes, for example a change from 0 to null * would not be included. * * Example return: @@ -1849,7 +1856,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // Update the changed array with references to changed obj-fields foreach($this->record as $k => $v) { if(is_object($v) && method_exists($v, 'isChanged') && $v->isChanged()) { - $this->changed[$k] = true; + $this->changed[$k] = 1; } } @@ -1862,7 +1869,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // Filter the list to those of a certain change level if($changeLevel > 1) { - foreach($fields as $name => $level) { + if($fields) foreach($fields as $name => $level) { if($level < $changeLevel) { unset($fields[$name]); } @@ -1879,6 +1886,19 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return $changedFields; } + + /** + * Uses {@link getChangedFields()} to determine if fields have been changed + * since loading them from the database. + * + * @param string $fieldName Name of the database field + * @param int $changeLevel See {@link getChangedFields()} + * @return boolean + */ + function isChanged($fieldName, $changeLevel = 1) { + $changed = $this->getChangedFields(false, $changeLevel); + return array_key_exists($fieldName, $changed); + } /** * Set the value of the field @@ -1998,7 +2018,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity $fieldMap = $this->uninherited('db', true); if($fieldMap) foreach($fieldMap as $fieldname => $fieldtype){ if(ClassInfo::classImplements($fieldtype, 'CompositeDBField')){ - $combined_db = singleton($fieldtype)->composite_db(); + $combined_db = singleton($fieldtype)->compositeDatabaseFields(); foreach($combined_db as $name => $type){ $fieldMap[$fieldname.$name] = $type; } @@ -2196,7 +2216,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity } else if($helperPair = $this->castingHelperPair($fieldName)) { $constructor = $helperPair['castingHelper']; $obj = eval($constructor); - $obj->setValue($this->$fieldName, $this->record); + $obj->setValue($this->$fieldName, $this->record, false); return $obj; // Special case for has_one relationships diff --git a/core/model/fieldtypes/DBField.php b/core/model/fieldtypes/DBField.php index 5a304f8fe..f4282015a 100644 --- a/core/model/fieldtypes/DBField.php +++ b/core/model/fieldtypes/DBField.php @@ -47,7 +47,7 @@ abstract class DBField extends ViewableData { */ static function create($className, $value, $name = null, $object = null) { $dbField = Object::create($className, $name, $object); - $dbField->setValue($value); + $dbField->setValue($value, null, false); return $dbField; } diff --git a/core/model/fieldtypes/Money.php b/core/model/fieldtypes/Money.php index ad1e8caeb..c46a5bdb6 100644 --- a/core/model/fieldtypes/Money.php +++ b/core/model/fieldtypes/Money.php @@ -68,14 +68,14 @@ class Money extends DBField implements CompositeDBField { parent::__construct($name); } - - public function composite_db(){ + + function compositeDatabaseFields() { return self::$composite_db; } function requireField() { - $composite_db = $this->composite_db(); - foreach($composite_db as $name => $type){ + $fields = $this->compositeDatabaseFields(); + if($fields) foreach($fields as $name => $type){ DB::requireField($this->tableName, $this->name.$name, $type); } } @@ -100,31 +100,32 @@ class Money extends DBField implements CompositeDBField { $query->select[] = sprintf('"%sCurrency"', $this->name); } - function setValue($value,$record=null) { + function setValue($value, $record = null, $markChanged = true) { // @todo Allow resetting value to NULL through Money $value field if ($value instanceof Money && $value->hasValue()) { - $this->setCurrency($value->getCurrency()); - $this->setAmount($value->getAmount()); + $this->setCurrency($value->getCurrency(), $markChanged); + $this->setAmount($value->getAmount(), $markChanged); + if($markChanged) $this->isChanged = true; } else if($record && isset($record[$this->name . 'Currency']) && isset($record[$this->name . 'Amount'])) { if($record[$this->name . 'Currency'] && $record[$this->name . 'Amount']) { - $this->setCurrency($record[$this->name . 'Currency']); - $this->setAmount($record[$this->name . 'Amount']); + $this->setCurrency($record[$this->name . 'Currency'], $markChanged); + $this->setAmount($record[$this->name . 'Amount'], $markChanged); } else { $this->value = $this->nullValue(); } + if($markChanged) $this->isChanged = true; } else if (is_array($value)) { if (array_key_exists('Currency', $value)) { - $this->setCurrency($value['Currency']); + $this->setCurrency($value['Currency'], $markChanged); } if (array_key_exists('Amount', $value)) { - $this->setAmount($value['Amount']); + $this->setAmount($value['Amount'], $markChanged); } + if($markChanged) $this->isChanged = true; } else { // @todo Allow to reset a money value by passing in NULL //user_error('Invalid value in Money->setValue()', E_USER_ERROR); } - - $this->isChanged = true; } /** @@ -164,9 +165,9 @@ class Money extends DBField implements CompositeDBField { /** * @param string */ - function setCurrency($currency) { + function setCurrency($currency, $markChanged = true) { $this->currency = $currency; - $this->isChanged = true; + if($markChanged) $this->isChanged = true; } /** @@ -181,9 +182,9 @@ class Money extends DBField implements CompositeDBField { /** * @param float $amount */ - function setAmount($amount) { + function setAmount($amount, $markChanged = true) { $this->amount = (float)$amount; - $this->isChanged = true; + if($markChanged) $this->isChanged = true; } /** @@ -281,7 +282,7 @@ class Money extends DBField implements CompositeDBField { * rather than a {@link Nice()} formatting. */ function __toString() { - return $this->getAmount(); + return (string)$this->getAmount(); } } ?> \ No newline at end of file diff --git a/security/Member.php b/security/Member.php index e633546bb..e006d309d 100644 --- a/security/Member.php +++ b/security/Member.php @@ -484,7 +484,7 @@ class Member extends DataObject { // Merge existing data into the local record foreach($existingRecord->getAllFields() as $k => $v) { - if(!isset($this->changed[$k]) || !$this->changed[$k]) $this->record[$k] = $v; + if(!$this->isChanged($k)) $this->record[$k] = $v; } $existingRecord->destroy(); } @@ -494,8 +494,7 @@ class Member extends DataObject { // However, if TestMailer is in use this isn't a risk. if( (Director::isLive() || Email::mailer() instanceof TestMailer) - && isset($this->changed['Password']) - && $this->changed['Password'] + && $this->isChanged('Password') && $this->record['Password'] && Member::$notify_password_change ) { @@ -503,18 +502,15 @@ class Member extends DataObject { } // The test on $this->ID is used for when records are initially created - if(!$this->ID || (isset($this->changed['Password']) && $this->changed['Password'])) { + if(!$this->ID || $this->isChanged('Password')) { // Password was changed: encrypt the password according the settings $encryption_details = Security::encrypt_password($this->Password); $this->Password = $encryption_details['password']; $this->Salt = $encryption_details['salt']; $this->PasswordEncryption = $encryption_details['algorithm']; - - $this->changed['Salt'] = true; - $this->changed['PasswordEncryption'] = true; // If we haven't manually set a password expiry - if(!isset($this->changed['PasswordExpiry']) || !$this->changed['PasswordExpiry']) { + if(!$this->isChanged('PasswordExpiry')) { // then set it for us if(self::$password_expiry_days) { $this->PasswordExpiry = date('Y-m-d', time() + 86400 * self::$password_expiry_days); @@ -535,7 +531,7 @@ class Member extends DataObject { function onAfterWrite() { parent::onAfterWrite(); - if(isset($this->changed['Password']) && $this->changed['Password']) { + if($this->isChanged('Password')) { MemberPassword::log($this); } } @@ -985,13 +981,13 @@ class Member extends DataObject { function validate() { $valid = parent::validate(); - if(!$this->ID || (isset($this->changed['Password']) && $this->changed['Password'])) { + if(!$this->ID || $this->isChanged('Password')) { if($this->Password && self::$password_validator) { $valid->combineAnd(self::$password_validator->validate($this->Password, $this)); } } - if((!$this->ID && $this->SetPassword) || (isset($this->changed['SetPassword']) && $this->changed['SetPassword'])) { + if((!$this->ID && $this->SetPassword) || $this->isChanged('SetPassword')) { if($this->SetPassword && self::$password_validator) { $valid->combineAnd(self::$password_validator->validate($this->SetPassword, $this)); } diff --git a/tests/DataObjectTest.php b/tests/DataObjectTest.php index 08ed53139..b55a82805 100644 --- a/tests/DataObjectTest.php +++ b/tests/DataObjectTest.php @@ -272,6 +272,37 @@ class DataObjectTest extends SapphireTest { ); } + function testIsChanged() { + $page = $this->fixture->objFromFixture('Page', 'home'); + $page->Title = 'Home-Changed'; + $page->ShowInMenus = true; // type change only, database stores "1" + + $this->assertTrue($page->isChanged('Title', 1)); + $this->assertTrue($page->isChanged('Title', 2)); + $this->assertTrue($page->isChanged('ShowInMenus', 1)); + $this->assertFalse($page->isChanged('ShowInMenus', 2)); + $this->assertFalse($page->isChanged('Content', 1)); + $this->assertFalse($page->isChanged('Content', 2)); + + $newPage = new Page(); + $newPage->Title = "New Page Title"; + $this->assertTrue($newPage->isChanged('Title', 1)); + $this->assertTrue($newPage->isChanged('Title', 2)); + $this->assertFalse($newPage->isChanged('Content', 1)); + $this->assertFalse($newPage->isChanged('Content', 2)); + + $newPage->write(); + $this->assertFalse($newPage->isChanged('Title', 1)); + $this->assertFalse($newPage->isChanged('Title', 2)); + $this->assertFalse($newPage->isChanged('Content', 1)); + $this->assertFalse($newPage->isChanged('Content', 2)); + + $page = $this->fixture->objFromFixture('Page', 'home'); + $page->Title = null; + $this->assertTrue($page->isChanged('Title', 1)); + $this->assertTrue($page->isChanged('Title', 2)); + } + function testRandomSort() { /* If we perforn the same regularly sorted query twice, it should return the same results */ $itemsA = DataObject::get("PageComment", "", "ID"); diff --git a/tests/model/MoneyTest.php b/tests/model/MoneyTest.php index bd64cefa4..617e59562 100644 --- a/tests/model/MoneyTest.php +++ b/tests/model/MoneyTest.php @@ -25,6 +25,13 @@ class MoneyTest extends SapphireTest { function testDataObjectChangedFields() { $obj = $this->objFromFixture('MoneyTest_DataObject', 'test1'); + + // Without changes + $curr = $obj->obj('MyMoney'); + $changed = $obj->getChangedFields(); + $this->assertNotContains('MyMoney', array_keys($changed)); + + // With changes $obj->MyMoney->setAmount(99); $changed = $obj->getChangedFields(); $this->assertContains('MyMoney', array_keys($changed)); diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 59a58515d..3e2e2c27d 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -3,6 +3,18 @@ class VersionedTest extends SapphireTest { static $fixture_file = 'sapphire/tests/model/VersionedTest.yml'; + function testForceChangeUpdatesVersion() { + $page = $this->objFromFixture('Page', 'page3'); + $oldVersion = $page->Version; + $page->forceChange(); + $page->write(); + + $this->assertTrue( + ($page->Version > $oldVersion), + "A object Version is increased when just calling forceChange() without any other changes" + ); + } + /** * Test Versioned::get_including_deleted() */