From 6394679b29ec60a7374676b67e2d33868dc37683 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Mon, 13 Jul 2009 05:47:45 +0000 Subject: [PATCH] BUGFIX #4285: Fixed static application bug that appeared in 2.3.2 git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.3@81698 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- core/Object.php | 48 +++++++++-- core/model/DataObject.php | 7 +- core/model/DataObjectDecorator.php | 95 ++++++++++++--------- core/model/SiteTree.php | 3 +- core/model/Translatable.php | 22 ++--- tests/DataObjectDecoratorTest.php | 23 ++++- tests/model/TranslatableTest.php | 11 --- tests/search/TranslatableSearchFormTest.php | 11 --- 8 files changed, 134 insertions(+), 86 deletions(-) diff --git a/core/Object.php b/core/Object.php index 2fe95f3dd..1a4dc0ddd 100755 --- a/core/Object.php +++ b/core/Object.php @@ -41,10 +41,11 @@ abstract class Object { */ private static - $statics = array(), - $cached_statics = array(), - $extra_statics = array(), - $replaced_statics = array(); + $statics = array(), + $cached_statics = array(), + $extra_statics = array(), + $replaced_statics = array(), + $_cache_statics_prepared = array(); private static $classes_constructed = array(), @@ -170,7 +171,12 @@ abstract class Object { * @return mixed */ public static function get_static($class, $name, $uncached = false) { + if(!isset(self::$_cache_statics_prepared[$class])) { + Object::prepare_statics($class); + } + if(!isset(self::$cached_statics[$class][$name]) || $uncached) { + //if($class == 'DataObjectDecoratorTest_MyObject') Debug::message("$class - $name"); $extra = $builtIn = $break = $replacedAt = false; $ancestry = array_reverse(ClassInfo::ancestry($class)); @@ -234,6 +240,10 @@ abstract class Object { * @param mixed $value */ public static function set_static($class, $name, $value) { + if(!isset(self::$_cache_statics_prepared[$class])) { + Object::prepare_statics($class); + } + self::$statics[$class][$name] = $value; self::$cached_statics[$class][$name] = true; } @@ -368,7 +378,7 @@ abstract class Object { * as a string, e.g. "Versioned" or "Translatable('Param')" */ public static function add_extension($class, $extension) { - if(!preg_match('/([^(]*)/', $extension, $matches)) { + if(!preg_match('/^([^(]*)/', $extension, $matches)) { return false; } $extensionClass = $matches[1]; @@ -388,10 +398,36 @@ abstract class Object { unset(self::$classes_constructed[$subclass]); } - // merge with existing static vars self::add_static_var($class, 'extensions', array($extension)); + + // load statics now for DataObject classes + if(is_subclass_of($class, 'DataObject')) { + DataObjectDecorator::load_extra_statics($class, $extensionClass); + } } + + /** + * Prepare static variables before processing a {@link get_static} or {@link set_static} + * call. + */ + private static function prepare_statics($class) { + // _cache_statics_prepared setting must come first to prevent infinite loops when we call + // get_static below + self::$_cache_statics_prepared[$class] = true; + + // load statics now for DataObject classes + if(is_subclass_of($class, 'DataObject')) { + $extensions = Object::get_static($class, 'extensions'); + if($extensions) foreach($extensions as $extension) { + if(preg_match('/^([^(]*)/', $extension, $matches)) { + $extensionClass = $matches[1]; + DataObjectDecorator::load_extra_statics($class, $extensionClass); + } + } + } + } + /** * Remove an extension from a class. diff --git a/core/model/DataObject.php b/core/model/DataObject.php index 2e43f3042..369030ce9 100644 --- a/core/model/DataObject.php +++ b/core/model/DataObject.php @@ -297,9 +297,12 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity parent::defineMethods(); - // Define the extra db fields + // Define the extra db fields - this is only necessary for extensions added in the + // class definition. Object::add_extension() will call this at definition time for + // those objects, which is a better mechanism. Perhaps extensions defined inside the + // class def can somehow be applied at definiton time also? if($this->extension_instances) foreach($this->extension_instances as $i => $instance) { - $instance->loadExtraStatics(); + DataObjectDecorator::load_extra_statics($this->class, $instance->class); } // Set up accessors for joined items diff --git a/core/model/DataObjectDecorator.php b/core/model/DataObjectDecorator.php index ae33ad7f3..dbbff1b0b 100755 --- a/core/model/DataObjectDecorator.php +++ b/core/model/DataObjectDecorator.php @@ -13,26 +13,27 @@ abstract class DataObjectDecorator extends Extension { * which can be decorated onto. This list is * limited for security and performance reasons. * + * Keys are the static names, and the values are whether or not the value is an array that should + * be merged. + * * @var array */ protected static $decoratable_statics = array( - 'db', - 'has_one', - 'indexes', - 'defaults', - 'has_many', - 'many_many', - 'belongs_many_many', - 'many_many_extraFields', - 'searchable_fields', + 'db' => true, + 'has_one' => true, + 'indexes' => true, + 'defaults' => true, + 'has_many' => true, + 'many_many' => true, + 'belongs_many_many' => true, + 'many_many_extraFields' => true, + 'searchable_fields' => true, + 'api_access' => false, ); + private static $extra_statics_loaded = array(); - /** - * Set the owner of this decorator. - * @param DataObject $owner - */ function setOwner(Object $owner, $ownerBaseClass = null) { if(!($owner instanceof DataObject)) { user_error(sprintf( @@ -43,34 +44,55 @@ abstract class DataObjectDecorator extends Extension { ); return false; } - parent::setOwner($owner, $ownerBaseClass); } - + /** - * Load the extra database fields defined in extraStatics. + * Load the extra static definitions for the given extension + * class name, called by {@link Object::add_extension()} + * + * @param string $class Class name of the owner class (or owner base class) + * @param string $extension Class name of the extension class */ - function loadExtraStatics() { - if(!empty(self::$extra_statics_loaded[$this->ownerBaseClass][$this->class])) return; - self::$extra_statics_loaded[$this->ownerBaseClass][$this->class] = true; - + public static function load_extra_statics($class, $extension) { + if(!empty(self::$extra_statics_loaded[$class][$extension])) return; + self::$extra_statics_loaded[$class][$extension] = true; + + // @deprecated 2.4 - use extraStatics() now, not extraDBFields() + if(method_exists($extension, 'extraDBFields')) { + $extraStaticsMethod = 'extraDBFields'; + } else { + $extraStaticsMethod = 'extraStatics'; + } + // If the extension has been manually applied to a subclass, we should ignore that. - if(Object::has_extension(get_parent_class($this->owner), $this->class)) return; - - if($fields = $this->extraStatics()) { - foreach($fields as $relation => $fields) { - if(in_array($relation, self::$decoratable_statics)) { - // Can't use add_static_var() here as it would merge the array rather than replacing - Object::set_static ( - $this->ownerBaseClass, - $relation, - array_merge((array) Object::get_static($this->ownerBaseClass, $relation), $fields) - ); + if(Object::has_extension(get_parent_class($class), $extension)) return; + + $statics = call_user_func(array($extension, $extraStaticsMethod)); + + if($statics) { + foreach($statics as $name => $newVal) { + if(isset(self::$decoratable_statics[$name])) { + $origVal = self::get_static($class, $name); + if($class == 'VersionedTest_DataObject') { + Debug::message($name); + Debug::dump($newVal); + } + + // Array to be merged + if(self::$decoratable_statics[$name]) { + // Can't use add_static_var() here as it would merge the array rather than replacing + self::set_static($class, $name, array_merge((array)$origVal, $newVal)); + + // Value to be overwritten + } else { + Object::set_static($class, $name, $newVal); + } } } - DataObject::$cache_has_own_table[$this->ownerBaseClass] = null; - DataObject::$cache_has_own_table_field[$this->ownerBaseClass] = null; + DataObject::$cache_has_own_table[$class] = null; + DataObject::$cache_has_own_table_field[$class] = null; } } @@ -149,13 +171,6 @@ abstract class DataObjectDecorator extends Extension { * the values are additional fields/relations to be defined. */ function extraStatics() { - return $this->extraDBFields(); - } - - /** - * @deprecated 2.3 Use extraStatics() - */ - function extraDBFields() { return array(); } diff --git a/core/model/SiteTree.php b/core/model/SiteTree.php index d0707a2b5..593218dd0 100644 --- a/core/model/SiteTree.php +++ b/core/model/SiteTree.php @@ -1935,4 +1935,5 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid } } -?> + +?> \ No newline at end of file diff --git a/core/model/Translatable.php b/core/model/Translatable.php index 6f00b1d8e..fc63eb75f 100755 --- a/core/model/Translatable.php +++ b/core/model/Translatable.php @@ -436,19 +436,15 @@ class Translatable extends DataObjectDecorator { } function extraStatics() { - if(get_class($this->owner) == ClassInfo::baseDataClass(get_class($this->owner))) { - return array( - "db" => array( - "Locale" => "DBLocale", - //"TranslationMasterID" => "Int" // optional relation to a "translation master" - ), - "defaults" => array( - "Locale" => Translatable::default_locale() // as an overloaded getter as well: getLang() - ) - ); - } else { - return array(); - } + return array( + "db" => array( + "Locale" => "DBLocale", + //"TranslationMasterID" => "Int" // optional relation to a "translation master" + ), + "defaults" => array( + "Locale" => Translatable::default_locale() // as an overloaded getter as well: getLang() + ) + ); } /** diff --git a/tests/DataObjectDecoratorTest.php b/tests/DataObjectDecoratorTest.php index 8743debeb..30d348921 100644 --- a/tests/DataObjectDecoratorTest.php +++ b/tests/DataObjectDecoratorTest.php @@ -31,6 +31,13 @@ class DataObjectDecoratorTest extends SapphireTest { $contact->delete(); } + /** + * Test that DataObject::$api_access can be set to true via a decorator + */ + function testApiAccessCanBeDecorated() { + $this->assertTrue(Object::get_static('DataObjectDecoratorTest_Member', 'api_access')); + } + function testPermissionDecoration() { // testing behaviour in isolation, too many sideeffects and other checks // in SiteTree->can*() methods to test one single feature reliably with them @@ -85,7 +92,8 @@ class DataObjectDecoratorTest_ContactRole extends DataObjectDecorator implements ), 'has_many' => array( 'RelatedObjects' => 'DataObjectDecoratorTest_RelatedObject' - ) + ), + 'api_access' => true, ); } @@ -167,6 +175,17 @@ class DataObjectDecoratorTest_Ext2 extends DataObjectDecorator implements TestOn } +class DataObjectDecoratorTest_Faves extends DataObjectDecorator implements TestOnly { + public function extraStatics() { + return array( + 'many_many' => array( + 'Faves' => 'Page' + ) + ); + } +} + DataObject::add_extension('DataObjectDecoratorTest_MyObject', 'DataObjectDecoratorTest_Ext1'); DataObject::add_extension('DataObjectDecoratorTest_MyObject', 'DataObjectDecoratorTest_Ext2'); -?> +DataObject::add_extension('DataObjectDecoratorTest_MyObject', 'DataObjectDecoratorTest_Faves'); +?> \ No newline at end of file diff --git a/tests/model/TranslatableTest.php b/tests/model/TranslatableTest.php index 9fa488692..3f34ab285 100644 --- a/tests/model/TranslatableTest.php +++ b/tests/model/TranslatableTest.php @@ -33,17 +33,6 @@ class TranslatableTest extends FunctionalTest { global $_SINGLETONS; $_SINGLETONS = array(); - // @todo Hack to refresh statics on the newly decorated classes - $newSiteTree = new SiteTree(); - foreach($newSiteTree->getExtensionInstances() as $extInstance) { - $extInstance->loadExtraStatics(); - } - // @todo Hack to refresh statics on the newly decorated classes - $TranslatableTest_DataObject = new TranslatableTest_DataObject(); - foreach($TranslatableTest_DataObject->getExtensionInstances() as $extInstance) { - $extInstance->loadExtraStatics(); - } - // recreate database with new settings $dbname = self::create_temp_db(); DB::set_alternative_database_name($dbname); diff --git a/tests/search/TranslatableSearchFormTest.php b/tests/search/TranslatableSearchFormTest.php index 0894535d8..c69e74727 100644 --- a/tests/search/TranslatableSearchFormTest.php +++ b/tests/search/TranslatableSearchFormTest.php @@ -32,17 +32,6 @@ class TranslatableSearchFormTest extends FunctionalTest { // clear singletons, they're caching old extension info which is used in DatabaseAdmin->doBuild() global $_SINGLETONS; $_SINGLETONS = array(); - - // @todo Hack to refresh statics on the newly decorated classes - $newSiteTree = new SiteTree(); - foreach($newSiteTree->getExtensionInstances() as $extInstance) { - $extInstance->loadExtraStatics(); - } - // @todo Hack to refresh statics on the newly decorated classes - $TranslatableTest_DataObject = new TranslatableTest_DataObject(); - foreach($TranslatableTest_DataObject->getExtensionInstances() as $extInstance) { - $extInstance->loadExtraStatics(); - } // recreate database with new settings $dbname = self::create_temp_db();