From 4f63f91cc8168affdccfb64051eac6fcd2078a3b Mon Sep 17 00:00:00 2001 From: Marcus Nyeholt Date: Fri, 7 Dec 2012 13:16:33 +1100 Subject: [PATCH] BUG Fixed issue with convertServiceProperty Fixed issue where convertServiceProperty is called when creating objects with user-supplied constructor arguments, so that it's only called when creating objects using injector configuration. This reduces the overhead of unnecessary calls to convertServiceProperty. Updated test cases to validate behaviour --- control/injector/Injector.php | 35 ++++++++++--- tests/injector/InjectorTest.php | 92 +++++++++++++++++++++++++++------ 2 files changed, 106 insertions(+), 21 deletions(-) diff --git a/control/injector/Injector.php b/control/injector/Injector.php index e5a776fcc..cfe06843e 100644 --- a/control/injector/Injector.php +++ b/control/injector/Injector.php @@ -362,6 +362,7 @@ class Injector { // EXCEPT when there's already an existing instance at this id. // if so, we need to instantiate and replace immediately if (isset($this->serviceCache[$id])) { + $this->updateSpecConstructor($spec); $this->instantiate($spec, $id); } } @@ -403,6 +404,20 @@ class Injector { } } } + + /** + * Update a class specification to convert constructor configuration information if needed + * + * We do this as a separate process to avoid unneeded calls to convertServiceProperty + * + * @param array $spec + * The class specification to update + */ + protected function updateSpecConstructor(&$spec) { + if (isset($spec['constructor'])) { + $spec['constructor'] = $this->convertServiceProperty($spec['constructor']); + } + } /** * Recursively convert a value into its proper representation with service references @@ -468,7 +483,7 @@ class Injector { $constructorParams = $spec['constructor']; } - $object = $this->objectCreator->create($this, $class, $constructorParams); + $object = $this->objectCreator->create($class, $constructorParams); // figure out if we have a specific id set or not. In some cases, we might be instantiating objects // that we don't manage directly; we don't want to store these in the service cache below @@ -730,15 +745,22 @@ class Injector { // 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]; @@ -750,6 +772,7 @@ class Injector { $this->load(array($name => $config)); if (isset($this->specs[$name])) { $spec = $this->specs[$name]; + $this->updateSpecConstructor($spec); return $this->instantiate($spec, $name); } } @@ -816,10 +839,10 @@ class InjectionCreator { * @param array $params * An array of parameters to be passed to the constructor */ - public function create(Injector $injector, $class, $params = array()) { + public function create($class, $params = array()) { $reflector = new ReflectionClass($class); if (count($params)) { - return $reflector->newInstanceArgs($injector->convertServiceProperty($params)); + return $reflector->newInstanceArgs($params); } return $reflector->newInstance(); } @@ -833,10 +856,10 @@ class SilverStripeInjectionCreator { * @param array $params * An array of parameters to be passed to the constructor */ - public function create(Injector $injector, $class, $params = array()) { + public function create($class, $params = array()) { $class = Object::getCustomClass($class); $reflector = new ReflectionClass($class); - return $reflector->newInstanceArgs($injector->convertServiceProperty($params)); + return $reflector->newInstanceArgs($params); } } diff --git a/tests/injector/InjectorTest.php b/tests/injector/InjectorTest.php index d5cd5cfe6..297ba3af2 100644 --- a/tests/injector/InjectorTest.php +++ b/tests/injector/InjectorTest.php @@ -442,7 +442,7 @@ class InjectorTest extends SapphireTest { public function testCustomObjectCreator() { $injector = new Injector(); - $injector->setObjectCreator(new SSObjectCreator()); + $injector->setObjectCreator(new SSObjectCreator($injector)); $config = array( 'OriginalRequirementsBackend', 'DummyRequirements' => array( @@ -485,9 +485,65 @@ class InjectorTest extends SapphireTest { $again = $injector->get('NeedsBothCirculars'); $this->assertEquals($again->var, 'One'); } + + public function testConvertServicePropertyOnCreate() { + // make sure convert service property is not called on direct calls to create, only on configured + // declarations to avoid un-needed function calls + $injector = new Injector(); + $item = $injector->create('ConstructableObject', '%$TestObject'); + $this->assertEquals('%$TestObject', $item->property); + + // do it again but have test object configured as a constructor dependency + $injector = new Injector(); + $config = array( + 'ConstructableObject' => array( + 'constructor' => array( + '%$TestObject' + ) + ) + ); + + $injector->load($config); + $item = $injector->get('ConstructableObject'); + $this->assertTrue($item->property instanceof TestObject); + + // and with a configured object defining TestObject to be something else! + $injector = new Injector(array('locator' => 'InjectorTestConfigLocator')); + $config = array( + 'ConstructableObject' => array( + 'constructor' => array( + '%$TestObject' + ) + ), + ); + + $injector->load($config); + $item = $injector->get('ConstructableObject'); + $this->assertTrue($item->property instanceof ConstructableObject); + + $this->assertInstanceOf('OtherTestObject', $item->property->property); + } } -class TestObject { +class InjectorTestConfigLocator extends SilverStripeServiceConfigurationLocator implements TestOnly { + public function locateConfigFor($name) { + if ($name == 'TestObject') { + return array('class' => 'ConstructableObject', 'constructor' => array('%$OtherTestObject')); + } + + return parent::locateConfigFor($name); + } +} + +class ConstructableObject implements TestOnly { + public $property; + + public function __construct($prop) { + $this->property = $prop; + } +} + +class TestObject implements TestOnly { public $sampleService; @@ -497,7 +553,7 @@ class TestObject { } -class OtherTestObject { +class OtherTestObject implements TestOnly { private $sampleService; @@ -511,13 +567,13 @@ class OtherTestObject { } -class CircularOne { +class CircularOne implements TestOnly { public $circularTwo; } -class CircularTwo { +class CircularTwo implements TestOnly { public $circularOne; @@ -528,7 +584,7 @@ class CircularTwo { } } -class NeedsBothCirculars { +class NeedsBothCirculars implements TestOnly{ public $circularOne; public $circularTwo; @@ -536,15 +592,15 @@ class NeedsBothCirculars { } -class MyParentClass { +class MyParentClass implements TestOnly { public $one; } -class MyChildClass extends MyParentClass { +class MyChildClass extends MyParentClass implements TestOnly { } -class DummyRequirements { +class DummyRequirements implements TestOnly { public $backend; @@ -558,15 +614,15 @@ class DummyRequirements { } -class OriginalRequirementsBackend { +class OriginalRequirementsBackend implements TestOnly { } -class NewRequirementsBackend { +class NewRequirementsBackend implements TestOnly { } -class TestStaticInjections { +class TestStaticInjections implements TestOnly { public $backend; static $dependencies = array( @@ -582,13 +638,19 @@ class TestStaticInjections { * @see https://github.com/silverstripe/sapphire */ class SSObjectCreator extends InjectionCreator { + private $injector; + + public function __construct($injector) { + $this->injector = $injector; + } - public function create(Injector $injector, $class, $params = array()) { + public function create($class, $params = array()) { if (strpos($class, '(') === false) { - return parent::create($injector, $class, $params); + return parent::create($class, $params); } else { list($class, $params) = self::parse_class_spec($class); - return parent::create($injector, $class, $params); + $params = $this->injector->convertServiceProperty($params); + return parent::create($class, $params); } }