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

@@ -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']);
 	}
 	
 	/**
@@ -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]);
 				}
@@ -1880,6 +1887,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
 	 * Called by {@link __set()} and any setFieldName() methods you might create.
@@ -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

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;
 	}
 	

core/model/fieldtypes/Money.php

@@ -69,13 +69,13 @@ 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();
 	}
 }
 ?>

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));
 			}

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");

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));

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()
 	 */