From b996e2c22c47eb8eecd1656e1dd69160b97fc68c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 5 Oct 2017 17:23:02 +1300 Subject: [PATCH] API Extensions are now stateless ENHANCEMENT Injector now lazy-loads services more intelligently --- docs/en/04_Changelogs/4.0.0.md | 75 +++++++- src/Core/ClassInfo.php | 14 +- .../Config/Middleware/ExtensionMiddleware.php | 2 + src/Core/CustomMethods.php | 48 ++--- src/Core/Extensible.php | 9 +- src/Core/Extension.php | 54 +++--- src/Core/Injector/Injector.php | 167 +++++++++++------- src/Dev/State/ExtensionTestState.php | 6 +- src/ORM/Hierarchy/Hierarchy.php | 68 +++---- src/Security/Group.php | 6 +- tests/php/Core/ObjectTest.php | 61 +++++-- tests/php/ORM/DataExtensionTest.php | 6 +- 12 files changed, 332 insertions(+), 184 deletions(-) diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index a9b21db12..c28eb73d7 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -578,6 +578,69 @@ In some places it may still be necessary to access the session object where no r In rare cases it is still possible to access the request of the current controller via `Controller::curr()->getRequest()` to gain access to the current session. +#### Upgrade extensions to work as singletons + +In SilverStripe 4.0 extensions are now all singletons, meaning that state stored as protected vars +within extensions are now shared across all object instances that use this extension. + +As a result, you must take care that all protected vars are either refactored to be stored against +the owner object, or are keyed in a fashion distinct to the parent. + +E.g. + +```diff +class MyExtension extends Extension { + public function getContent() { +- if (!$this->contentCache) { +- $this->contentCache = $this->generateContent(); +- } +- return $this->contentCache; ++ $contentCache = $this->owner->getField('contentCache'); ++ if (!$contentCache) { ++ $contentCache = $this->generateContent(); ++ $this->owner->setField('contentCache', $contentCache); ++ } ++ return $contentCache; + } +} +``` + +In addition when using extensions with distinct constructor arguments it's advisable to +use `yml` to register you those constructor arguments and use a service name or alias +in `private static $extensions`. + +E.g. this is the service definition for the various versioned extension variants + +```yaml +--- +Name: versionedextension +--- +SilverStripe\Core\Injector\Injector: + # Versioning only + SilverStripe\Versioned\Versioned.versioned: + class: SilverStripe\Versioned\Versioned + constructor: + mode: Versioned + # Staging and Versioning + SilverStripe\Versioned\Versioned.stagedversioned: + class: SilverStripe\Versioned\Versioned + constructor: + mode: StagedVersioned + # Default is alias for .stagedversioned + SilverStripe\Versioned\Versioned: '%$SilverStripe\Versioned\Versioned.stagedversioned' +``` + +Upgrade your extension references as below: + +```diff +class MyClass extends DataObject { + private static $extensions = [ +- Versioned::class . '(Versioned)', ++ Versioned::class . '.versioned', + ]; +} +``` + #### Compatibility with the new front-end building tools If you are using Requirements from 3.x then your scripts should continue to work as they did before. @@ -1297,8 +1360,12 @@ Rather than declaring the list of stages a model has, the constructor for `Versi parameter, which declares whether or not the model is versioned and has a draft and live stage, or alternatively if it only has versioning without staging. +Each form of this extension is registered under the appropriate service identifier, which you should use in your +model as below: + ```php use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\Versioning\Versioned; /** * This model has staging and versioning. Stages will be "Stage" and "Live" @@ -1306,7 +1373,7 @@ use SilverStripe\ORM\DataObject; class MyStagedModel extends SilverStripe\ORM\DataObject { private static $extensions = [ - "SilverStripe\\ORM\\Versioning\\Versioned('StagedVersioned')" + Versioned::class . '.stagedversioned', ]; } @@ -1316,7 +1383,7 @@ class MyStagedModel extends SilverStripe\ORM\DataObject class MyVersionedModel extends DataObject { private static $extensions = [ - "SilverStripe\\ORM\\Versioning\\Versioned('Versioned')" + Versioned::class . '.versioned', ]; } ``` @@ -1860,6 +1927,8 @@ that it belongs to, e.g. `themes/mytheme/templates/MyVendor/Foobar/Model/MyModel * `RequestFilter` has been deprecated in favour of [HTTPMiddleware](/developer_guides/controllers/middlewares). Also the legacy RequestFilter API has changed: $session and $dataModel variables removed from preRequest / postRequest. +* `Extension` instances are now singletons and no longer are constructed once per extended object. + See the 'Upgrade extensions to work as singletons' section on this page for more information. #### General and Core Removed API @@ -1945,6 +2014,8 @@ that it belongs to, e.g. `themes/mytheme/templates/MyVendor/Foobar/Model/MyModel Accordingly, `hasService()` has been renamed to `has()`, and `get()` will throw `SilverStripe\Core\Injector\InjectorNotFoundException` when the service can't be found. * Removed `CustomMethods::createMethod()`. Use closures instead. +* Removed `Extension::$ownerBaseClass` property. You should use $this->owner->baseClass() instead. + The second argument of `Extension::setOwner()` has also been removed. #### General and Core Deprecated API diff --git a/src/Core/ClassInfo.php b/src/Core/ClassInfo.php index bc3eaf30a..12e707d82 100644 --- a/src/Core/ClassInfo.php +++ b/src/Core/ClassInfo.php @@ -197,7 +197,7 @@ class ClassInfo // Merge with descendants $descendants = ClassLoader::inst()->getManifest()->getDescendantsOf($className); return array_merge( - [ $lowerClassName => $className ], + [$lowerClassName => $className], $descendants ); } @@ -441,6 +441,10 @@ class ClassInfo } elseif (is_array($token) && $token[0] === T_NS_SEPARATOR) { $class .= $token[1]; $hadNamespace = true; + } elseif ($token === '.') { + // Treat service name separator as NS separator + $class .= '.'; + $hadNamespace = true; } elseif ($hadNamespace && is_array($token) && $token[0] === T_STRING) { $class .= $token[1]; $hadNamespace = false; @@ -454,7 +458,11 @@ class ClassInfo $result = stripcslashes(substr($argString, 1, -1)); break; case "'": - $result = str_replace(array("\\\\", "\\'"), array("\\", "'"), substr($argString, 1, -1)); + $result = str_replace( + ["\\\\", "\\'"], + ["\\", "'"], + substr($argString, 1, -1) + ); break; default: throw new Exception("Bad T_CONSTANT_ENCAPSED_STRING arg $argString"); @@ -506,7 +514,7 @@ class ClassInfo } else { if ($tokenName === '[') { $result = array(); - } elseif (($tokenName === ')' || $tokenName === ']') && ! empty($bucketStack)) { + } elseif (($tokenName === ')' || $tokenName === ']') && !empty($bucketStack)) { // Store the bucket we're currently working on $oldBucket = $bucket; // Fetch the key for the bucket at the top of the stack diff --git a/src/Core/Config/Middleware/ExtensionMiddleware.php b/src/Core/Config/Middleware/ExtensionMiddleware.php index ef64c2994..3edbc5b0d 100644 --- a/src/Core/Config/Middleware/ExtensionMiddleware.php +++ b/src/Core/Config/Middleware/ExtensionMiddleware.php @@ -64,6 +64,8 @@ class ExtensionMiddleware implements Middleware $extensions = $extensionSourceConfig['extensions']; foreach ($extensions as $extension) { list($extensionClass, $extensionArgs) = ClassInfo::parse_class_spec($extension); + // Strip service name specifier + $extensionClass = strtok($extensionClass, '.'); if (!class_exists($extensionClass)) { throw new InvalidArgumentException("$class references nonexistent $extensionClass in 'extensions'"); } diff --git a/src/Core/CustomMethods.php b/src/Core/CustomMethods.php index f0f3f45fc..91b3038ea 100644 --- a/src/Core/CustomMethods.php +++ b/src/Core/CustomMethods.php @@ -67,27 +67,30 @@ trait CustomMethods $this->{$property}[$index] : $this->{$property}; - if ($obj) { - if (!empty($config['callSetOwnerFirst'])) { - /** @var Extension $obj */ - $obj->setOwner($this); - } - $retVal = call_user_func_array(array($obj, $method), $arguments); - if (!empty($config['callSetOwnerFirst'])) { - /** @var Extension $obj */ - $obj->clearOwner(); - } - return $retVal; + if (!$obj) { + throw new BadMethodCallException( + "Object->__call(): {$class} cannot pass control to {$property}({$index})." + . ' Perhaps this object was mistakenly destroyed?' + ); } - throw new BadMethodCallException( - "Object->__call(): {$class} cannot pass control to {$property}({$index})." - . ' Perhaps this object was mistakenly destroyed?' - ); + // Call without setOwner + if (empty($config['callSetOwnerFirst'])) { + return $obj->$method(...$arguments); + } + + /** @var Extension $obj */ + try { + $obj->setOwner($this); + return $obj->$method(...$arguments); + } finally { + $obj->clearOwner(); + } } case isset($config['wrap']): { array_unshift($arguments, $config['method']); - return call_user_func_array(array($this, $config['wrap']), $arguments); + $wrapped = $config['wrap']; + return $this->$wrapped(...$arguments); } case isset($config['function']): { return $config['function']($this, $arguments); @@ -192,11 +195,14 @@ trait CustomMethods { if (method_exists($extension, 'allMethodNames')) { if ($extension instanceof Extension) { - $extension->setOwner($this); - } - $methods = $extension->allMethodNames(true); - if ($extension instanceof Extension) { - $extension->clearOwner(); + try { + $extension->setOwner($this); + $methods = $extension->allMethodNames(true); + } finally { + $extension->clearOwner(); + } + } else { + $methods = $extension->allMethodNames(true); } } else { $class = get_class($extension); diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index 7eb30ab1b..e4bb1be1b 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -139,8 +139,8 @@ trait Extensible $this->addCallbackMethod($method, function ($inst, $args) use ($method, $extensionClass) { /** @var Extensible $inst */ $extension = $inst->getExtensionInstance($extensionClass); - $extension->setOwner($inst); try { + $extension->setOwner($inst); return call_user_func_array([$extension, $method], $args); } finally { $extension->clearOwner(); @@ -343,6 +343,8 @@ trait Extensible foreach ($extensions as $extension) { list($extensionClass, $extensionArgs) = ClassInfo::parse_class_spec($extension); + // Strip service name specifier + $extensionClass = strtok($extensionClass, '.'); $sources[] = $extensionClass; if (!class_exists($extensionClass)) { @@ -465,8 +467,8 @@ trait Extensible foreach ($this->getExtensionInstances() as $instance) { if (method_exists($instance, $method)) { - $instance->setOwner($this); try { + $instance->setOwner($this); $value = $instance->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7); } finally { $instance->clearOwner(); @@ -551,8 +553,7 @@ trait Extensible if ($extensions) { foreach ($extensions as $extension) { - $instance = Injector::inst()->create($extension); - $instance->setOwner(null, $class); + $instance = Injector::inst()->get($extension); $this->extension_instances[get_class($instance)] = $instance; } } diff --git a/src/Core/Extension.php b/src/Core/Extension.php index 253ea0d6a..c7fe7f6c3 100644 --- a/src/Core/Extension.php +++ b/src/Core/Extension.php @@ -30,15 +30,7 @@ abstract class Extension protected $owner; /** - * The base class that this extension was applied to; $this->owner must be one of these - * - * @var DataObject - */ - protected $ownerBaseClass; - - /** - * Ownership stack for recursive methods. - * Last item is current owner. + * Stack of all parent owners, not including current owner * * @var array */ @@ -63,24 +55,29 @@ abstract class Extension /** * Set the owner of this extension. * - * @param object $owner The owner object, - * @param string $ownerBaseClass The base class that the extension is applied to; this may be - * the class of owner, or it may be a parent. For example, if Versioned was applied to SiteTree, - * and then a Page object was instantiated, $owner would be a Page object, but $ownerBaseClass - * would be 'SiteTree'. + * @param object $owner The owner object */ - public function setOwner($owner, $ownerBaseClass = null) + public function setOwner($owner) { - if ($owner) { - $this->ownerStack[] = $owner; - } + $this->ownerStack[] = $this->owner; $this->owner = $owner; + } - // Set ownerBaseClass - if ($ownerBaseClass) { - $this->ownerBaseClass = $ownerBaseClass; - } elseif (!$this->ownerBaseClass && $owner) { - $this->ownerBaseClass = get_class($owner); + /** + * Temporarily modify the owner. The original owner is ensured to be restored + * + * @param mixed $owner Owner to set + * @param callable $callback Callback to invoke + * @param array $args Args to pass to callback + * @return mixed + */ + public function withOwner($owner, callable $callback, $args = []) + { + try { + $this->setOwner($owner); + return $callback(...$args); + } finally { + $this->clearOwner(); } } @@ -92,12 +89,7 @@ abstract class Extension if (empty($this->ownerStack)) { throw new BadMethodCallException("clearOwner() called more than setOwner()"); } - array_pop($this->ownerStack); - if ($this->ownerStack) { - $this->owner = end($this->ownerStack); - } else { - $this->owner = null; - } + $this->owner = array_pop($this->ownerStack); } /** @@ -120,7 +112,7 @@ abstract class Extension */ public static function get_classname_without_arguments($extensionStr) { - $parts = explode('(', $extensionStr); - return $parts[0]; + // Split out both args and service name + return strtok(strtok($extensionStr, '('), '.'); } } diff --git a/src/Core/Injector/Injector.php b/src/Core/Injector/Injector.php index 446b7d22a..583ed4534 100644 --- a/src/Core/Injector/Injector.php +++ b/src/Core/Injector/Injector.php @@ -193,6 +193,16 @@ class Injector implements ContainerInterface */ protected $configLocator; + /** + * Specify a service type singleton + */ + const SINGLETON = 'singleton'; + + /** + * Specif ya service type prototype + */ + const PROTOTYPE = 'prototype'; + /** * Create a new injector. * @@ -584,7 +594,7 @@ class Injector implements ContainerInterface $type = isset($spec['type']) ? $spec['type'] : null; } - if ($id && (!$type || $type != 'prototype')) { + if ($id && (!$type || $type !== self::PROTOTYPE)) { // this ABSOLUTELY must be set before the object is injected. // This prevents circular reference errors down the line $this->serviceCache[$id] = $object; @@ -827,8 +837,8 @@ class Injector implements ContainerInterface */ public function getServiceName($name) { - // common case, get it overwith first - if (isset($this->specs[$name])) { + // Lazy load in spec (disable inheritance to check exact service name) + if ($this->getServiceSpec($name, false)) { return $name; } @@ -919,10 +929,9 @@ class Injector implements ContainerInterface * * @param string $name The name of the service to retrieve. If not a registered * service, then a class of the given name is instantiated - * @param boolean $asSingleton Whether to register the created object as a singleton - * if no other configuration is found - * @param array $constructorArgs Optional set of arguments to pass as constructor arguments - * if this object is to be created from scratch (with $asSingleton = false) + * @param bool $asSingleton If set to false a new instance will be returned. + * If true a singleton will be returned unless the spec is type=prototype' + * @param array $constructorArgs Args to pass in to the constructor. Note: Ignored for singletons * @return mixed Instance of the specified object */ public function get($name, $asSingleton = true, $constructorArgs = []) @@ -939,70 +948,42 @@ class Injector implements ContainerInterface /** * Returns the service, or `null` if it doesnt' exist. See {@link get()} for main usage. * - * @param string $name - * @param boolean $asSingleton - * @param array $constructorArgs - * @return mixed|null Instance of the specified object (if it exists) + * @param string $name The name of the service to retrieve. If not a registered + * service, then a class of the given name is instantiated + * @param bool $asSingleton If set to false a new instance will be returned. + * If true a singleton will be returned unless the spec is type=prototype' + * @param array $constructorArgs Args to pass in to the constructor. Note: Ignored for singletons + * @return mixed Instance of the specified object */ protected function getNamedService($name, $asSingleton = true, $constructorArgs = []) { - // Allow service names of the form "%$ServiceName" - if (substr($name, 0, 2) == '%$') { - $name = substr($name, 2); - } - // Normalise service / args list($name, $constructorArgs) = $this->normaliseArguments($name, $constructorArgs); - // reassign the name as it might actually be a compound name - if ($serviceName = $this->getServiceName($name)) { - // check to see what the type of bean is. If it's a prototype, - // we don't want to return the singleton version of it. - $spec = $this->specs[$serviceName]; - $type = isset($spec['type']) ? $spec['type'] : null; - // if we're explicitly a prototype OR we're not wanting a singleton - if (($type && $type == 'prototype') || !$asSingleton) { - if ($spec && $constructorArgs) { - $spec['constructor'] = $constructorArgs; - } else { - // convert any _configured_ constructor args. - // we don't call this for get() calls where someone passes in - // constructor args, otherwise we end up calling convertServiceParams - // way too often - $this->updateSpecConstructor($spec); - } - return $this->instantiate($spec, $serviceName, !$type ? 'prototype' : $type); - } else { - if (!isset($this->serviceCache[$serviceName])) { - $this->updateSpecConstructor($spec); - $this->instantiate($spec, $serviceName); - } - return $this->serviceCache[$serviceName]; - } - } - $config = $this->configLocator->locateConfigFor($name); - if ($config) { - $this->load(array($name => $config)); - if (isset($this->specs[$name])) { - $spec = $this->specs[$name]; - $this->updateSpecConstructor($spec); - if ($constructorArgs) { - $spec['constructor'] = $constructorArgs; - } - return $this->instantiate($spec, $name); - } - } - // If we've got this far, we're dealing with a case of a user wanting - // to create an object based on its name. So, we need to fake its config - // if the user wants it managed as a singleton service style object - $spec = array('class' => $name, 'constructor' => $constructorArgs); - if ($asSingleton) { - // need to load the spec in; it'll be given the singleton type by default - $this->load(array($name => $spec)); - return $this->instantiate($spec, $name); + // Resolve name with the appropriate spec, or a suitable mock for new services + list($name, $spec) = $this->getServiceNamedSpec($name, $constructorArgs); + + // Check if we are getting a prototype or singleton + $type = $asSingleton + ? (isset($spec['type']) ? $spec['type'] : self::SINGLETON) + : self::PROTOTYPE; + + // Return existing instance for singletons + if ($type === self::SINGLETON && isset($this->serviceCache[$name])) { + return $this->serviceCache[$name]; } - return $this->instantiate($spec); + // Update constructor args + if ($type === self::PROTOTYPE && $constructorArgs) { + // Passed in args are expected to already be normalised (no service references) + $spec['constructor'] = $constructorArgs; + } else { + // Resolve references in constructor args + $this->updateSpecConstructor($spec); + } + + // Build instance + return $this->instantiate($spec, $name, $type); } /** @@ -1016,6 +997,11 @@ class Injector implements ContainerInterface */ protected function normaliseArguments($name, $args = []) { + // Allow service names of the form "%$ServiceName" + if (substr($name, 0, 2) == '%$') { + $name = substr($name, 2); + } + if (strstr($name, '(')) { list($name, $extraArgs) = ClassInfo::parse_class_spec($name); if ($args) { @@ -1027,6 +1013,61 @@ class Injector implements ContainerInterface return [ $name, $args ]; } + /** + * Get or build a named service and specification + * + * @param string $name Service name + * @param array $constructorArgs Optional constructor args + * @return array + */ + protected function getServiceNamedSpec($name, $constructorArgs = []) + { + $spec = $this->getServiceSpec($name); + if ($spec) { + // Resolve to exact service name (in case inherited) + $name = $this->getServiceName($name); + } else { + // Late-generate config spec for non-configured spec + $spec = [ + 'class' => $name, + 'constructor' => $constructorArgs, + ]; + } + return [$name, $spec]; + } + + /** + * Search for spec, lazy-loading in from config locator. + * Falls back to parent service name if unloaded + * + * @param string $name + * @param bool $inherit Set to true to inherit from parent service if `.` suffixed + * E.g. 'Psr/Log/LoggerInterface.custom' would fail over to 'Psr/Log/LoggerInterface' + * @return mixed|object + */ + public function getServiceSpec($name, $inherit = true) + { + if (isset($this->specs[$name])) { + return $this->specs[$name]; + } + + // Lazy load + $config = $this->configLocator->locateConfigFor($name); + if ($config) { + $this->load([$name => $config]); + if (isset($this->specs[$name])) { + return $this->specs[$name]; + } + } + + // Fail over to parent service if allowed + if (!$inherit || !strpos($name, '.')) { + return null; + } + + return $this->getServiceSpec(substr($name, 0, strrpos($name, '.'))); + } + /** * Magic method to return an item directly * diff --git a/src/Dev/State/ExtensionTestState.php b/src/Dev/State/ExtensionTestState.php index af33ba74c..29c4dd09c 100644 --- a/src/Dev/State/ExtensionTestState.php +++ b/src/Dev/State/ExtensionTestState.php @@ -5,7 +5,6 @@ namespace SilverStripe\Dev\State; use LogicException; use SilverStripe\Core\Extension; use SilverStripe\Core\Injector\Injector; -use SilverStripe\Dev\Debug; use SilverStripe\Dev\SapphireTest; use SilverStripe\ORM\DataObject; @@ -90,7 +89,10 @@ class ExtensionTestState implements TestState // clear singletons, they're caching old extension info // which is used in DatabaseAdmin->doBuild() - Injector::inst()->unregisterObjects(DataObject::class); + Injector::inst()->unregisterObjects([ + DataObject::class, + Extension::class + ]); // If we have altered the schema, but SapphireTest::setUpBeforeClass() would not otherwise // reset the schema (if there were extra objects) then force a reset diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index 8db80a394..73f0a4e9b 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -23,21 +23,6 @@ use Exception; */ class Hierarchy extends DataExtension { - - /** - * Cache for {@see numChildren()} - * - * @var int - */ - protected $_cache_numChildren = null; - - /** - * Cache for {@see Children()} - * - * @var SS_List - */ - protected $_cache_children = null; - /** * The lower bounds for the amount of nodes to mark. If set, the logic will expand nodes until it reaches at least * this number, and then stops. Root nodes will always show regardless of this settting. Further nodes can be @@ -86,6 +71,17 @@ class Hierarchy extends DataExtension */ private static $hide_from_cms_tree = array(); + /** + * Prevent virtual page virtualising these fields + * + * @config + * @var array + */ + private static $non_virtual_fields = [ + '_cache_children', + '_cache_numChildren', + ]; + public static function get_extra_config($class, $extension, $args) { return array( @@ -176,17 +172,19 @@ class Hierarchy extends DataExtension */ public function Children() { - if ($this->_cache_children) { - return $this->_cache_children; + $children = $this->owner->_cache_children; + if ($children) { + return $children; } - $this->_cache_children = $this + $children = $this ->owner ->stageChildren(false) ->filterByCallback(function (DataObject $record) { return $record->canView(); }); - return $this->_cache_children; + $this->owner->_cache_children = $children; + return $children; } /** @@ -268,18 +266,21 @@ class Hierarchy extends DataExtension public function numChildren($cache = true) { // Load if caching - if ($cache && isset($this->_cache_numChildren)) { - return $this->_cache_numChildren; + if ($cache) { + $numChildren = $this->owner->_cache_numChildren; + if (isset($numChildren)) { + return $numChildren; + } } // We call stageChildren(), because Children() has canView() filtering - $children = (int)$this->owner->stageChildren(true)->Count(); + $numChildren = (int)$this->owner->stageChildren(true)->Count(); // Save if caching if ($cache) { - $this->_cache_numChildren = $children; + $this->owner->_cache_numChildren = $numChildren; } - return $children; + return $numChildren; } /** @@ -308,7 +309,8 @@ class Hierarchy extends DataExtension { $hideFromHierarchy = $this->owner->config()->hide_from_hierarchy; $hideFromCMSTree = $this->owner->config()->hide_from_cms_tree; - $staged = DataObject::get($this->ownerBaseClass) + $baseClass = $this->owner->baseClass(); + $staged = DataObject::get($baseClass) ->filter('ParentID', (int)$this->owner->ID) ->exclude('ID', (int)$this->owner->ID); if ($hideFromHierarchy) { @@ -374,11 +376,12 @@ class Hierarchy extends DataExtension if (empty($parentID)) { return null; } - $idSQL = $this->owner->getSchema()->sqlColumnForField($this->ownerBaseClass, 'ID'); - return DataObject::get_one($this->ownerBaseClass, array( - array($idSQL => $parentID), + $baseClass = $this->owner->baseClass(); + $idSQL = $this->owner->getSchema()->sqlColumnForField($baseClass, 'ID'); + return DataObject::get_one($baseClass, [ + [$idSQL => $parentID], $filter - )); + ]); } /** @@ -424,13 +427,10 @@ class Hierarchy extends DataExtension * Flush all Hierarchy caches: * - Children (instance) * - NumChildren (instance) - * - Marked (global) - * - Expanded (global) - * - TreeOpened (global) */ public function flushCache() { - $this->_cache_children = null; - $this->_cache_numChildren = null; + $this->owner->_cache_children = null; + $this->owner->_cache_numChildren = null; } } diff --git a/src/Security/Group.php b/src/Security/Group.php index e843b26a2..5ba3fc424 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -616,11 +616,7 @@ class Group extends DataObject */ public function AllChildrenIncludingDeleted() { - /** @var Hierarchy $extInstance */ - $extInstance = $this->getExtensionInstance(Hierarchy::class); - $extInstance->setOwner($this); - $children = $extInstance->AllChildrenIncludingDeleted(); - $extInstance->clearOwner(); + $children = parent::AllChildrenIncludingDeleted(); $filteredChildren = new ArrayList(); diff --git a/tests/php/Core/ObjectTest.php b/tests/php/Core/ObjectTest.php index 9ed76f09c..46a4c4619 100644 --- a/tests/php/Core/ObjectTest.php +++ b/tests/php/Core/ObjectTest.php @@ -52,7 +52,14 @@ class ObjectTest extends SapphireTest $objs[] = new ObjectTest\T2(); // All these methods should exist and return true - $trueMethods = array('testMethod','otherMethod','someMethod','t1cMethod','normalMethod', 'failoverCallback'); + $trueMethods = [ + 'testMethod', + 'otherMethod', + 'someMethod', + 't1cMethod', + 'normalMethod', + 'failoverCallback' + ]; foreach ($objs as $i => $obj) { foreach ($trueMethods as $method) { @@ -224,33 +231,33 @@ class ObjectTest extends SapphireTest $this->assertTrue( singleton(ExtensionTest::class)->hasExtension(ExtendTest1::class), "Extensions are detected when set on Object::\$extensions on instance hasExtension() without" - . " case-sensitivity" + . " case-sensitivity" ); // ObjectTest_ExtendTest2 is built in via $extensions (with parameters) $this->assertTrue( ExtensionTest::has_extension(ExtendTest2::class), "Extensions are detected with static has_extension() when set on Object::\$extensions with" - . " additional parameters" + . " additional parameters" ); $this->assertTrue( singleton(ExtensionTest::class)->hasExtension(ExtendTest2::class), "Extensions are detected with instance hasExtension() when set on Object::\$extensions with" - . " additional parameters" + . " additional parameters" ); $this->assertFalse( ExtensionTest::has_extension(ExtendTest3::class), "Other extensions available in the system are not present unless explicitly added to this object" - . " when checking through has_extension()" + . " when checking through has_extension()" ); $this->assertFalse( singleton(ExtensionTest::class)->hasExtension(ExtendTest3::class), "Other extensions available in the system are not present unless explicitly added to this object" - . " when checking through instance hasExtension()" + . " when checking through instance hasExtension()" ); // ObjectTest_ExtendTest3 is added manually - ExtensionTest::add_extension(ExtendTest3::class .'("Param")'); + ExtensionTest::add_extension(ExtendTest3::class . '("Param")'); $this->assertTrue( ExtensionTest::has_extension(ExtendTest3::class), "Extensions are detected with static has_extension() when added through add_extension()" @@ -322,7 +329,7 @@ class ObjectTest extends SapphireTest public function testRemoveExtensionWithParameters() { - ObjectTest\ExtensionRemoveTest::add_extension(ExtendTest2::class.'("MyParam")'); + ObjectTest\ExtensionRemoveTest::add_extension(ExtendTest2::class . '("MyParam")'); $this->assertTrue( ObjectTest\ExtensionRemoveTest::has_extension(ExtendTest2::class), @@ -361,7 +368,7 @@ class ObjectTest extends SapphireTest public function testExtend() { - $object = new ObjectTest\ExtendTest(); + $object = new ObjectTest\ExtendTest(); $argument = 'test'; $this->assertEquals($object->extend('extendableMethod'), array('ExtendTest2()')); @@ -399,17 +406,25 @@ class ObjectTest extends SapphireTest { // Simple case $this->assertEquals( - array(Versioned::class,array('Stage', 'Live')), + array(Versioned::class, array('Stage', 'Live')), ClassInfo::parse_class_spec("SilverStripe\\Versioned\\Versioned('Stage','Live')") ); + // Case with service identifier + $this->assertEquals( + [ + Versioned::class . '.versioned', + ['Versioned'], + ], + ClassInfo::parse_class_spec("SilverStripe\\Versioned\\Versioned.versioned('Versioned')") + ); // String with commas $this->assertEquals( - array(Versioned::class,array('Stage,Live', 'Stage')), + array(Versioned::class, array('Stage,Live', 'Stage')), ClassInfo::parse_class_spec("SilverStripe\\Versioned\\Versioned('Stage,Live','Stage')") ); // String with quotes $this->assertEquals( - array(Versioned::class,array('Stage\'Stage,Live\'Live', 'Live')), + array(Versioned::class, array('Stage\'Stage,Live\'Live', 'Live')), ClassInfo::parse_class_spec("SilverStripe\\Versioned\\Versioned('Stage\\'Stage,Live\\'Live','Live')") ); @@ -425,26 +440,36 @@ class ObjectTest extends SapphireTest // Array $this->assertEquals( - array('Enum',array(array('Accepted', 'Pending', 'Declined', 'Unsubmitted'), 'Unsubmitted')), + array('Enum', array(array('Accepted', 'Pending', 'Declined', 'Unsubmitted'), 'Unsubmitted')), ClassInfo::parse_class_spec("Enum(array('Accepted', 'Pending', 'Declined', 'Unsubmitted'), 'Unsubmitted')") ); // Nested array $this->assertEquals( - array('Enum',array(array('Accepted', 'Pending', 'Declined', array('UnsubmittedA','UnsubmittedB')), - 'Unsubmitted')), + [ + 'Enum', + [ + ['Accepted', 'Pending', 'Declined', ['UnsubmittedA', 'UnsubmittedB']], + 'Unsubmitted' + ] + ], ClassInfo::parse_class_spec( "Enum(array('Accepted', 'Pending', 'Declined', array('UnsubmittedA','UnsubmittedB')), 'Unsubmitted')" ) ); // 5.4 Shorthand Array $this->assertEquals( - array('Enum',array(array('Accepted', 'Pending', 'Declined', 'Unsubmitted'), 'Unsubmitted')), + array('Enum', array(array('Accepted', 'Pending', 'Declined', 'Unsubmitted'), 'Unsubmitted')), ClassInfo::parse_class_spec("Enum(['Accepted', 'Pending', 'Declined', 'Unsubmitted'], 'Unsubmitted')") ); // 5.4 Nested shorthand array $this->assertEquals( - array('Enum',array(array('Accepted', 'Pending', 'Declined', array('UnsubmittedA','UnsubmittedB')), - 'Unsubmitted')), + [ + 'Enum', + [ + ['Accepted', 'Pending', 'Declined', ['UnsubmittedA', 'UnsubmittedB']], + 'Unsubmitted' + ] + ], ClassInfo::parse_class_spec( "Enum(['Accepted', 'Pending', 'Declined', ['UnsubmittedA','UnsubmittedB']], 'Unsubmitted')" ) diff --git a/tests/php/ORM/DataExtensionTest.php b/tests/php/ORM/DataExtensionTest.php index 62bc5df66..2f935e7da 100644 --- a/tests/php/ORM/DataExtensionTest.php +++ b/tests/php/ORM/DataExtensionTest.php @@ -292,7 +292,11 @@ class DataExtensionTest extends SapphireTest $extension->clearOwner(); $this->assertEquals($obj1, $extension->getOwner()); - // Clear original owner + // Clear pushed null + $extension->clearOwner(); + $this->assertNull($extension->getOwner()); + + // Clear original null $extension->clearOwner(); $this->assertNull($extension->getOwner());