From e55f3126fa6ec167e0ff67aaa927e601d35e0b46 Mon Sep 17 00:00:00 2001
From: Ingo Schommer <ingo@silverstripe.com>
Date: Mon, 27 Apr 2009 05:55:25 +0000
Subject: [PATCH] BUGFIX Changed DataObject->newClassInstance() to only force
 changes if a differing classname was set. Setting ClassName property on new
 instance to trigger change detection (required for
 Translatable->onBeforeWrite()) BUGFIX Only force DataObject->forceChange() on
 fields which aren't already marked as changed

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@75248 467b73ca-7a2a-4603-9d3b-597d59a354a9
---
 core/model/DataObject.php | 37 +++++++++++++++++++++++++------------
 tests/DataObjectTest.php  | 19 +++++++++++++++++++
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/core/model/DataObject.php b/core/model/DataObject.php
index c1d23d926..aa3e7b7d1 100644
--- a/core/model/DataObject.php
+++ b/core/model/DataObject.php
@@ -269,9 +269,13 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 	}
 
 	/**
-	 * Set the ClassName attribute; $this->class is also updated.
+	 * Set the ClassName attribute. {@link $class} is also updated.
+	 * Warning: This will produce an inconsistent record, as the object
+	 * instance will not automatically switch to the new subclass.
+	 * Please use {@link newClassInstance()} for this purpose,
+	 * or destroy and reinstanciate the record.
 	 *
-	 * @param string $className The new ClassName attribute
+	 * @param string $className The new ClassName attribute (a subclass of {@link DataObject})
 	 */
 	function setClassName($className) {
 		$this->class = trim($className);
@@ -282,19 +286,26 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 	 * Create a new instance of a different class from this object's record
 	 * This is useful when dynamically changing the type of an instance. Specifically,
 	 * it ensures that the instance of the class is a match for the className of the
-	 * record.
+	 * record. Don't set the {@link DataObject->class} or {@link DataObject->ClassName}
+	 * property manually before calling this method, as it will confuse change detection.
 	 *
 	 * @param string $newClassName The name of the new class
 	 *
 	 * @return DataObject The new instance of the new class, The exact type will be of the class name provided.
 	 */
 	function newClassInstance($newClassName) {
-		$newRecord = $this->record;
-		//$newRecord['RecordClassName'] = $newRecord['ClassName'] = $newClassName;
-
-		$newInstance = new $newClassName($newRecord);
-		$newInstance->setClassName($newClassName);
-		$newInstance->forceChange();
+		$originalClass = $this->ClassName;
+		$newInstance = new $newClassName(array_merge(
+			$this->record,
+			array(
+				'ClassName'=>$originalClass,
+				'RecordClassName'=>$originalClass,
+			)
+		));
+		if($newClassName != $this->ClassName) {
+			$newInstance->setClassName($newClassName);
+			$newInstance->forceChange();
+		}
 
 		return $newInstance;
 	}
@@ -622,11 +633,13 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
 	/**
 	 * Forces the record to think that all its data has changed.
-	 * Doesn't write to the database.
+	 * Doesn't write to the database. Only sets fields as changed
+	 * if they are not already marked as changed.
 	 */
 	public function forceChange() {
-		foreach($this->record as $fieldName => $fieldVal)
-		$this->changed[$fieldName] = 1;
+		foreach($this->record as $fieldName => $fieldVal) {
+			if(!isset($this->changed[$fieldName])) $this->changed[$fieldName] = 1;
+		}
 	}
 	
 	/**
diff --git a/tests/DataObjectTest.php b/tests/DataObjectTest.php
index fe6e3d539..fd6650128 100644
--- a/tests/DataObjectTest.php
+++ b/tests/DataObjectTest.php
@@ -604,6 +604,25 @@ class DataObjectTest extends SapphireTest {
 			"Defaults are populated for in-memory object from \$defaults array"
 		);
 	}
+	
+	function testNewClassInstance() {
+		$page = $this->fixture->objFromFixture('Page', 'page1');
+		$changedPage = $page->newClassInstance('RedirectorPage');
+		$changedFields = $changedPage->getChangedFields();
+		
+		// Don't write the record, it will reset changed fields
+		
+		$this->assertType('RedirectorPage', $changedPage);
+		$this->assertEquals($changedPage->ClassName, 'RedirectorPage');
+		//$this->assertEquals($changedPage->RecordClassName, 'RedirectorPage');
+		$this->assertContains('ClassName', array_keys($changedFields));
+		$this->assertEquals($changedFields['ClassName']['before'], 'Page');
+		$this->assertEquals($changedFields['ClassName']['after'], 'RedirectorPage');
+		
+		$changedPage->write();
+		$this->assertType('RedirectorPage', $changedPage);
+		$this->assertEquals($changedPage->ClassName, 'RedirectorPage');
+	}
 
 }