From 9b4d689bb23134c09ed5acc9c72461f88a2d46df Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 22 Aug 2017 11:22:08 +1200 Subject: [PATCH] Lazy-load custom methods and extensions on CustomMethods and Extensible traits No longer need constructExtensions() --- docs/en/04_Changelogs/4.0.0.md | 11 +-- src/Control/Director.php | 1 - src/Core/CustomMethods.php | 61 ++++++++++++----- src/Core/Extensible.php | 71 ++++++++++---------- src/Dev/BuildTask.php | 1 - src/Forms/DefaultFormFactory.php | 1 - src/Forms/FormTransformation.php | 1 - src/Forms/GridField/GridFieldConfig.php | 1 - src/Forms/GridField/GridFieldDetailForm.php | 1 - src/Forms/GridField/GridFieldPrintButton.php | 1 - src/Forms/Validator.php | 1 - src/Security/DefaultAdminService.php | 1 - src/View/Parsers/ShortcodeParser.php | 1 - src/View/ViewableData.php | 1 - tests/php/Core/ObjectTest.php | 4 +- tests/php/Core/ObjectTest/BaseObject.php | 1 - tests/php/Core/ObjectTest/T2.php | 3 + tests/php/ORM/ArrayListTest.php | 12 ++-- 18 files changed, 90 insertions(+), 84 deletions(-) diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index 75324b8fe..dd7a8af17 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -429,8 +429,9 @@ Object has been superseded by a trio of traits which replace components of this - Injectable: Provides `MyClass::create()` and `MyClass::singleton()` - Configurable: Provides `MyClass::config()` - - Extensible: Provides all methods related to extensions (E.g. add_extension()). Note: - All classes which use this trait MUST invoke `constructExtensions()` in its constructor. + - Extensible: Provides all methods related to extensions (E.g. add_extension()). + +All custom method and extension instances are constructed lazily. In particular specific Object class usages should be replaced as below: @@ -450,12 +451,6 @@ Upgrade subclasses + use Extensible; + use Injectable; + use Configurable; -+ -+ public function __construct() -+ { -+ // Only needed if using Extensible trait -+ $this->constructExtensions(); -+ } +} ``` diff --git a/src/Control/Director.php b/src/Control/Director.php index c41a2c8f0..90d73807b 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -108,7 +108,6 @@ class Director implements TemplateGlobalProvider public function __construct() { - $this->constructExtensions(); } /** diff --git a/src/Core/CustomMethods.php b/src/Core/CustomMethods.php index f37be48f5..4a9b30a55 100644 --- a/src/Core/CustomMethods.php +++ b/src/Core/CustomMethods.php @@ -4,6 +4,7 @@ namespace SilverStripe\Core; use BadMethodCallException; use InvalidArgumentException; +use SilverStripe\Dev\Deprecation; /** * Allows an object to declare a set of custom methods @@ -16,7 +17,7 @@ trait CustomMethods * * @var array */ - protected static $extra_methods = array(); + protected static $extra_methods = []; /** * Name of methods to invoke by defineMethods for this instance @@ -48,10 +49,6 @@ trait CustomMethods // If the method cache was cleared by an an Object::add_extension() / Object::remove_extension() // call, then we should rebuild it. $class = static::class; - if (!array_key_exists($class, self::$extra_methods)) { - $this->defineMethods(); - } - $config = $this->getExtraMethodConfig($method); if (empty($config)) { throw new BadMethodCallException( @@ -60,6 +57,9 @@ trait CustomMethods } switch (true) { + case isset($config['callback']): { + return $config['callback']($this, $arguments); + } case isset($config['property']) : { $obj = $config['index'] !== null ? $this->{$config['property']}[$config['index']] : @@ -89,18 +89,19 @@ trait CustomMethods ); } } - case isset($config['wrap']): + case isset($config['wrap']): { array_unshift($arguments, $config['method']); return call_user_func_array(array($this, $config['wrap']), $arguments); - - case isset($config['function']): + } + case isset($config['function']): { return $config['function']($this, $arguments); - - default: + } + default: { throw new BadMethodCallException( "Object->__call(): extra method $method is invalid on $class:" - . var_export($config, true) + . var_export($config, true) ); + } } } @@ -156,9 +157,13 @@ trait CustomMethods */ protected function getExtraMethodConfig($method) { - $class = static::class; - if (isset(self::$extra_methods[$class][strtolower($method)])) { - return self::$extra_methods[$class][strtolower($method)]; + // Lazy define methods + if (!isset(self::$extra_methods[static::class])) { + $this->defineMethods(); + } + + if (isset(self::$extra_methods[static::class][strtolower($method)])) { + return self::$extra_methods[static::class][strtolower($method)]; } return null; } @@ -228,9 +233,16 @@ trait CustomMethods $methods = $this->findMethodsFromExtension($extension); if ($methods) { + if ($extension instanceof Extension) { + Deprecation::notice( + '5.0', + 'Register custom methods from extensions with addCallbackMethod.' + . ' callSetOwnerFirst will be removed in 5.0' + ); + } $methodInfo = array( 'property' => $property, - 'index' => $index, + 'index' => $index, 'callSetOwnerFirst' => $extension instanceof Extension, ); @@ -287,10 +299,23 @@ trait CustomMethods */ protected function addWrapperMethod($method, $wrap) { - $class = static::class; - self::$extra_methods[$class][strtolower($method)] = array ( - 'wrap' => $wrap, + self::$extra_methods[static::class][strtolower($method)] = array( + 'wrap' => $wrap, 'method' => $method ); } + + /** + * Add callback as a method. + * + * @param string $method Name of method + * @param callable $callback Callback to invoke. + * Note: $this is passed as first parameter to this callback and then $args as array + */ + protected function addCallbackMethod($method, $callback) + { + self::$extra_methods[static::class][strtolower($method)] = [ + 'callback' => $callback, + ]; + } } diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php index cfc03103e..b5f4c5efa 100644 --- a/src/Core/Extensible.php +++ b/src/Core/Extensible.php @@ -6,29 +6,18 @@ use InvalidArgumentException; use SilverStripe\Control\RequestHandler; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; +use SilverStripe\Dev\Deprecation; +use SilverStripe\ORM\DataExtension; +use SilverStripe\ORM\DataObject; use SilverStripe\View\ViewableData; /** * Allows an object to have extensions applied to it. - * - * Bootstrap by calling $this->constructExtensions() in your class constructor. - * - * Requires CustomMethods trait */ trait Extensible { use CustomMethods { - getExtraMethodConfig as getCustomMethodsConfig; - } - - protected function getExtraMethodConfig($method) - { - $config = $this->getCustomMethodsConfig($method); - // Ensure extension instances are populated before being accessed - if (isset($config['property']) && $config['property'] === 'extension_instances') { - $this->getExtensionInstances(); - } - return $config; + defineMethods as defineMethodsCustom; } /** @@ -52,8 +41,6 @@ trait Extensible */ private static $extensions = []; - private static $classes_constructed = array(); - /** * Classes that cannot be extended * @@ -121,17 +108,20 @@ trait Extensible $this->afterExtendCallbacks[$method][] = $callback; } + /** + * @deprecated 4.0..5.0 Extensions and methods are now lazy-loaded + */ protected function constructExtensions() { - // Register this trait as a method source - $this->registerExtraMethodCallback('defineExtensionMethods', function () { - $this->defineExtensionMethods(); - }); + Deprecation::notice('5.0', 'constructExtensions does not need to be invoked and will be removed in 5.0'); + } - if (!isset(self::$classes_constructed[static::class])) { - $this->defineMethods(); - self::$classes_constructed[static::class] = true; - } + protected function defineMethods() + { + $this->defineMethodsCustom(); + + // Define extension methods + $this->defineExtensionMethods(); } /** @@ -139,17 +129,27 @@ trait Extensible * All these methods can then be called directly on the instance (transparently * mapped through {@link __call()}), or called explicitly through {@link extend()}. * - * @uses addMethodsFrom() + * @uses addCallbackMethod() */ protected function defineExtensionMethods() { $extensions = $this->getExtensionInstances(); - foreach (array_keys($extensions) as $key) { - $this->addMethodsFrom('extension_instances', $key); + foreach ($extensions as $extensionClass => $extensionInstance) { + foreach ($this->findMethodsFromExtension($extensionInstance) as $method) { + $this->addCallbackMethod($method, function ($inst, $args) use ($method, $extensionClass) { + /** @var Extensible $inst */ + $extension = $inst->getExtensionInstance($extensionClass); + $extension->setOwner($inst); + try { + return call_user_func_array([$extension, $method], $args); + } finally { + $extension->clearOwner(); + } + }); + } } } - /** * Add an extension to a specific class. * @@ -202,7 +202,6 @@ trait Extensible if ($subclasses) { foreach ($subclasses as $subclass) { - unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); } } @@ -215,8 +214,8 @@ trait Extensible Injector::inst()->unregisterNamedObject($class); // load statics now for DataObject classes - if (is_subclass_of($class, 'SilverStripe\\ORM\\DataObject')) { - if (!is_subclass_of($extensionClass, 'SilverStripe\\ORM\\DataExtension')) { + if (is_subclass_of($class, DataObject::class)) { + if (!is_subclass_of($extensionClass, DataExtension::class)) { user_error("$extensionClass cannot be applied to $class without being a DataExtension", E_USER_ERROR); } } @@ -272,7 +271,6 @@ trait Extensible $subclasses[] = $class; if ($subclasses) { foreach ($subclasses as $subclass) { - unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); } } @@ -468,11 +466,14 @@ trait Extensible foreach ($this->getExtensionInstances() as $instance) { if (method_exists($instance, $method)) { $instance->setOwner($this); - $value = $instance->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7); + try { + $value = $instance->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7); + } finally { + $instance->clearOwner(); + } if ($value !== null) { $values[] = $value; } - $instance->clearOwner(); } } diff --git a/src/Dev/BuildTask.php b/src/Dev/BuildTask.php index b8c403e8d..26a86d139 100644 --- a/src/Dev/BuildTask.php +++ b/src/Dev/BuildTask.php @@ -22,7 +22,6 @@ abstract class BuildTask public function __construct() { - $this->constructExtensions(); } /** diff --git a/src/Forms/DefaultFormFactory.php b/src/Forms/DefaultFormFactory.php index 55ec66a15..8aeaf3088 100644 --- a/src/Forms/DefaultFormFactory.php +++ b/src/Forms/DefaultFormFactory.php @@ -23,7 +23,6 @@ class DefaultFormFactory implements FormFactory public function __construct() { - $this->constructExtensions(); } /** diff --git a/src/Forms/FormTransformation.php b/src/Forms/FormTransformation.php index ee9cd7032..9b64a1c1e 100644 --- a/src/Forms/FormTransformation.php +++ b/src/Forms/FormTransformation.php @@ -28,7 +28,6 @@ class FormTransformation public function __construct() { - $this->constructExtensions(); } public function transform(FormField $field) diff --git a/src/Forms/GridField/GridFieldConfig.php b/src/Forms/GridField/GridFieldConfig.php index 6491070d2..f74c530ab 100644 --- a/src/Forms/GridField/GridFieldConfig.php +++ b/src/Forms/GridField/GridFieldConfig.php @@ -38,7 +38,6 @@ class GridFieldConfig public function __construct() { $this->components = new ArrayList(); - $this->constructExtensions(); } /** diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index e36d3da87..fd757d766 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/src/Forms/GridField/GridFieldDetailForm.php @@ -84,7 +84,6 @@ class GridFieldDetailForm implements GridField_URLHandler public function __construct($name = 'DetailForm') { $this->name = $name; - $this->constructExtensions(); } /** diff --git a/src/Forms/GridField/GridFieldPrintButton.php b/src/Forms/GridField/GridFieldPrintButton.php index 3e1c5db09..acc18649b 100644 --- a/src/Forms/GridField/GridFieldPrintButton.php +++ b/src/Forms/GridField/GridFieldPrintButton.php @@ -49,7 +49,6 @@ class GridFieldPrintButton implements GridField_HTMLProvider, GridField_ActionPr { $this->targetFragment = $targetFragment; $this->printColumns = $printColumns; - $this->constructExtensions(); } /** diff --git a/src/Forms/Validator.php b/src/Forms/Validator.php index 6ac357894..b51e39631 100644 --- a/src/Forms/Validator.php +++ b/src/Forms/Validator.php @@ -21,7 +21,6 @@ abstract class Validator public function __construct() { $this->resetResult(); - $this->constructExtensions(); } /** diff --git a/src/Security/DefaultAdminService.php b/src/Security/DefaultAdminService.php index 95d151f63..0b13824f0 100644 --- a/src/Security/DefaultAdminService.php +++ b/src/Security/DefaultAdminService.php @@ -37,7 +37,6 @@ class DefaultAdminService public function __construct() { - $this->constructExtensions(); } /** diff --git a/src/View/Parsers/ShortcodeParser.php b/src/View/Parsers/ShortcodeParser.php index 411727699..444078ed5 100644 --- a/src/View/Parsers/ShortcodeParser.php +++ b/src/View/Parsers/ShortcodeParser.php @@ -26,7 +26,6 @@ class ShortcodeParser public function __construct() { - $this->constructExtensions(); } public function img_shortcode($attrs) diff --git a/src/View/ViewableData.php b/src/View/ViewableData.php index e62e852f8..aca9ffa77 100644 --- a/src/View/ViewableData.php +++ b/src/View/ViewableData.php @@ -85,7 +85,6 @@ class ViewableData implements IteratorAggregate public function __construct() { - $this->constructExtensions(); } // ----------------------------------------------------------------------------------------------------------------- diff --git a/tests/php/Core/ObjectTest.php b/tests/php/Core/ObjectTest.php index 17805a118..9ed76f09c 100644 --- a/tests/php/Core/ObjectTest.php +++ b/tests/php/Core/ObjectTest.php @@ -4,10 +4,8 @@ namespace SilverStripe\Core\Tests; use SilverStripe\Control\Controller; use SilverStripe\Core\ClassInfo; -use SilverStripe\Core\Config\Config; use SilverStripe\Core\Extension; use SilverStripe\Core\Injector\Injector; -use SilverStripe\Core\Manifest\ClassLoader; use SilverStripe\Core\Tests\ObjectTest\BaseObject; use SilverStripe\Core\Tests\ObjectTest\ExtendTest1; use SilverStripe\Core\Tests\ObjectTest\ExtendTest2; @@ -54,7 +52,7 @@ class ObjectTest extends SapphireTest $objs[] = new ObjectTest\T2(); // All these methods should exist and return true - $trueMethods = array('testMethod','otherMethod','someMethod','t1cMethod','normalMethod'); + $trueMethods = array('testMethod','otherMethod','someMethod','t1cMethod','normalMethod', 'failoverCallback'); foreach ($objs as $i => $obj) { foreach ($trueMethods as $method) { diff --git a/tests/php/Core/ObjectTest/BaseObject.php b/tests/php/Core/ObjectTest/BaseObject.php index a51be4cc1..a0f1b752d 100644 --- a/tests/php/Core/ObjectTest/BaseObject.php +++ b/tests/php/Core/ObjectTest/BaseObject.php @@ -15,6 +15,5 @@ class BaseObject implements TestOnly public function __construct() { - $this->constructExtensions(); } } diff --git a/tests/php/Core/ObjectTest/T2.php b/tests/php/Core/ObjectTest/T2.php index 885e32610..d7e6ed47e 100644 --- a/tests/php/Core/ObjectTest/T2.php +++ b/tests/php/Core/ObjectTest/T2.php @@ -23,6 +23,9 @@ class T2 extends BaseObject $this->addMethodsFrom('failover'); $this->addMethodsFrom('failoverArr', 0); $this->addMethodsFrom('failoverArr', 1); + $this->addCallbackMethod('failoverCallback', function ($inst, $args) { + return true; + }); } public function wrappedMethod($val) diff --git a/tests/php/ORM/ArrayListTest.php b/tests/php/ORM/ArrayListTest.php index 7ddfccd27..513b5d79b 100644 --- a/tests/php/ORM/ArrayListTest.php +++ b/tests/php/ORM/ArrayListTest.php @@ -941,9 +941,9 @@ class ArrayListTest extends SapphireTest { $list = new ArrayList( array( - array('Name' => 'Steve', 'ID' => 1, 'Age' => 21), + $steve = array('Name' => 'Steve', 'ID' => 1, 'Age' => 21), array('Name' => 'Bob', 'ID' => 2, 'Age' => 18), - array('Name' => 'Clair', 'ID' => 2, 'Age' => 21), + $clair = array('Name' => 'Clair', 'ID' => 2, 'Age' => 21), array('Name' => 'Oscar', 'ID' => 2, 'Age' => 52), array('Name' => 'Mike', 'ID' => 3, 'Age' => 43) ) @@ -955,13 +955,9 @@ class ArrayListTest extends SapphireTest } ); - $expected = array( - new ArrayData(array('Name' => 'Steve', 'ID' => 1, 'Age' => 21)), - new ArrayData(array('Name' => 'Clair', 'ID' => 2, 'Age' => 21)), - ); - $this->assertEquals(2, $list->count()); - $this->assertEquals($expected, $list->toArray(), 'List should only contain Steve and Clair'); + $this->assertEquals($steve, $list[0]->toMap(), 'List should only contain Steve and Clair'); + $this->assertEquals($clair, $list[1]->toMap(), 'List should only contain Steve and Clair'); $this->assertTrue($list instanceof Filterable, 'The List should be of type SS_Filterable'); }