ENHANCEMENT: Removed circular references from extensions to improve garbage collection.

API CHANGE: The result of any extension returned by Object::extInstance() should have setOwner() called on it before calling a method, and clearOwner() after.

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@78414 467b73ca-7a2a-4603-9d3b-597d59a354a9

core/Extension.php

@@ -29,6 +29,12 @@ abstract class Extension extends Object {
 	 * @var DataObject
 	 */
 	protected $ownerBaseClass;
+	
+	/**
+	 * Reference counter to ensure that the owner isn't cleared until clearOwner() has
+	 * been called as many times as setOwner()
+	 */
+	private $ownerRefs = 0;
 
 	/**
 	 * Set the owner of this decorator.
@@ -38,9 +44,18 @@ abstract class Extension extends Object {
 	 * and then a Page object was instantiated, $owner would be a Page object, but $ownerBaseClass
 	 * would be 'SiteTree'.
 	 */
-	function setOwner(Object $owner, $ownerBaseClass = null) {
-		$this->ownerBaseClass = $ownerBaseClass ? $ownerBaseClass : $owner->class;
+	function setOwner($owner, $ownerBaseClass = null) {
+		if($owner) $this->ownerRefs++;
 		$this->owner = $owner;
+
+		if($ownerBaseClass) $this->ownerBaseClass = $ownerBaseClass;
+		else if(!$this->ownerBaseClass && $owner) $this->ownerBaseClass = $owner->class;
+	}
+	
+	function clearOwner() {
+		if($this->ownerRefs <= 0) user_error("clearOwner() called more than setOwner()", E_USER_WARNING);
+		$this->ownerRefs--;
+		if($this->ownerRefs == 0) $this->owner = null;
 	}
 	
 	/**

core/Object.php

@@ -453,7 +453,7 @@ abstract class Object {
 				// an $extension value can contain parameters as a string,
 				// e.g. "Versioned('Stage','Live')"
 				$instance = eval("return new $extension;");
-				$instance->setOwner($this, $class);
+				$instance->setOwner(null, $class);
 				$this->extension_instances[$instance->class] = $instance;
 			}
 		}
@@ -464,6 +464,7 @@ abstract class Object {
 		}
 	}
 	
+	
 	/**
 	 * Attemps to locate and call a method dynamically added to a class at runtime if a default cannot be located
 	 *
@@ -485,8 +486,13 @@ abstract class Object {
 					$obj = $config['index'] !== null ?
 						$this->{$config['property']}[$config['index']] :
 						$this->{$config['property']};
-					
-					if($obj) return call_user_func_array(array($obj, $method), $arguments);
+						
+					if($obj) {
+						if(!empty($config['callSetOwnerFirst'])) $obj->setOwner($this);
+						$retVal = call_user_func_array(array($obj, $method), $arguments);
+						if(!empty($config['callSetOwnerFirst'])) $obj->clearOwner();
+						return $retVal;
+					}
 					
 					if($this->destroyed) {
 						throw new Exception (
@@ -597,7 +603,8 @@ abstract class Object {
 			foreach($extension->allMethodNames(true) as $method) {
 				self::$extra_methods[$this->class][$method] = array (
 					'property' => $property,
-					'index'    => $index
+					'index'    => $index,
+					'callSetOwnerFirst' => $extension instanceof Extension,
 				);
 			}
 		}
@@ -738,8 +745,10 @@ abstract class Object {
 		
 		if($this->extension_instances) foreach($this->extension_instances as $instance) {
 			if($instance->hasMethod($method)) {
+				$instance->setOwner($this);
 				$value = $instance->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7);
 				if($value !== null) $values[] = $value;
+				$instance->clearOwner();
 			}
 		}
 		

core/model/DataObject.php

@@ -362,7 +362,9 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
 		// Define the extra db fields
 		if($this->extension_instances) foreach($this->extension_instances as $i => $instance) {
+			$instance->setOwner($this);
 			$instance->loadExtraStatics();
+			$instance->clearOwner();
 		}
 
 		// Set up accessors for joined items
@@ -2507,6 +2509,13 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 			if(class_exists($record['RecordClassName'])) {
 				$results[] = new $record['RecordClassName']($record);
 			} else {
+				if(!$baseClass) {
+					user_error("Bad RecordClassName '{$record['RecordClassName']}' and "
+						. "\$baseClass not set", E_USER_ERROR);
+				} else if(!is_string($baseClass) || !class_exists($baseClass)) {
+					user_error("Bad RecordClassName '{$record['RecordClassName']}' and bad "
+						. "\$baseClass '$baseClass not set", E_USER_ERROR);
+				}
 				$results[] = new $baseClass($record);
 			}
 		}

core/model/DataObjectDecorator.php

@@ -37,8 +37,9 @@ abstract class DataObjectDecorator extends Extension {
 	 * Set the owner of this decorator.
 	 * @param DataObject $owner
 	 */
-	function setOwner(Object $owner, $ownerBaseClass = null) {
-		if(!($owner instanceof DataObject)) {
+	/*
+	function setOwner($owner, $ownerBaseClass = null) {
+		if($owner && !($owner instanceof DataObject)) {
 			user_error(sprintf(
 				"DataObjectDecorator->setOwner(): Trying to decorate an object of class '%s' with '%s', 
 				only Dataobject subclasses are supported.",
@@ -50,6 +51,7 @@ abstract class DataObjectDecorator extends Extension {
 		
 		parent::setOwner($owner, $ownerBaseClass);
 	}
+	*/
 	
 	/**
 	 * Load the extra database fields defined in extraStatics.

core/model/Hierarchy.php

@@ -416,6 +416,8 @@ class Hierarchy extends DataObjectDecorator {
 	 * @return DataObjectSet
 	 */
 	public function doAllChildrenIncludingDeleted($context = null) {
+		if(!$this->owner) user_error('Hierarchy::doAllChildrenIncludingDeleted() called without $this->owner');
+		
 		$idxStageChildren = array();
 		$idxLiveChildren = array();
 		

core/model/Translatable.php

@@ -172,7 +172,7 @@ class Translatable extends DataObjectDecorator {
 	 * An array of fields that can be translated.
 	 * @var array
 	 */
-	protected $translatableFields;
+	protected $translatableFields = null;
 
 	/**
 	 * A map of the field values of the original (untranslated) DataObject record
@@ -424,11 +424,13 @@ class Translatable extends DataObjectDecorator {
 	
 	}
 	
-	function setOwner(Object $owner) {
-		parent::setOwner($owner);
+	function setOwner($owner, $ownerBaseClass = null) {
+		parent::setOwner($owner, $ownerBaseClass);
 
 		// setting translatable fields by inspecting owner - this should really be done in the constructor
-		$this->translatableFields = array_keys($this->owner->inheritedDatabaseFields());
+		if($this->owner && $this->translatableFields === null) {
+			$this->translatableFields = array_keys($this->owner->inheritedDatabaseFields());
+		}
 	}
 	
 	function extraStatics() {

core/model/Versioned.php

@@ -223,7 +223,9 @@ class Versioned extends DataObjectDecorator {
 				}
 				if ($suffix && ($ext = $this->owner->extInstance($allSuffixes[$suffix]))) {
 					if (!$ext->isVersionedTable($table)) continue;
+					$ext->setOwner($this->owner);
 					$fields = $ext->fieldsInExtraTables($suffix);
+					$ext->clearOwner();
 					$indexes = $fields['indexes'];
 					$fields = $fields['db'];
 				}
@@ -387,7 +389,10 @@ class Versioned extends DataObjectDecorator {
 	function extendWithSuffix($table) {
 		foreach (Versioned::$versionableExtensions as $versionableExtension => $suffixes) {
 			if ($this->owner->hasExtension($versionableExtension)) {
-				$table = $this->owner->extInstance($versionableExtension)->extendWithSuffix($table);
+				$ext = $this->owner->extInstance($versionableExtension);
+				$ext->setOwner($this->owner);
+				$table = $ext->extendWithSuffix($table);
+				$ext->clearOwner();
 			}
 		}
 		return $table;
@@ -796,7 +801,7 @@ class Versioned extends DataObjectDecorator {
 			AND \"$archiveTable\".\"Version\" = \"{$baseTable}_versions\".\"Version\"";
 		
 		// Process into a DataObjectSet
-		$result = $SNG->buildDataObjectSet($query->execute());
+		$result = $SNG->buildDataObjectSet($query->execute(), 'DataObjectSet', null, $class);
 
 		Versioned::$reading_stage = $oldStage;
 		return $result;

security/Group.php

@@ -351,7 +351,11 @@ class Group extends DataObject {
 	 * Filters to only those groups that the current user can edit
 	 */
 	function AllChildrenIncludingDeleted() {
-		$children = $this->extInstance('Hierarchy')->AllChildrenIncludingDeleted();
+		$extInstance = $this->extInstance('Hierarchy');
+		$extInstance->setOwner($this);
+		$children = $extInstance->AllChildrenIncludingDeleted();
+		$extInstance->clearOwner();
+		
 		$filteredChildren = new DataObjectSet();
 		
 		if($children) foreach($children as $child) {

tests/model/TranslatableTest.php

@@ -36,12 +36,16 @@ class TranslatableTest extends FunctionalTest {
 		// @todo Hack to refresh statics on the newly decorated classes
 		$newSiteTree = new SiteTree();
 		foreach($newSiteTree->getExtensionInstances() as $extInstance) {
+			$extInstance->setOwner($newSiteTree);
 			$extInstance->loadExtraStatics();
+			$extInstance->clearOwner();
 		}
 		// @todo Hack to refresh statics on the newly decorated classes
 		$TranslatableTest_DataObject = new TranslatableTest_DataObject();
 		foreach($TranslatableTest_DataObject->getExtensionInstances() as $extInstance) {
+			$extInstance->setOwner($TranslatableTest_DataObject);
 			$extInstance->loadExtraStatics();
+			$extInstance->clearOwner();
 		}
 
 		// recreate database with new settings