Merge pull request #1019 from nyeholt/injector_convert_service_params

BUG Fixed issue with convertServiceProperty

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);
 			}
 		}
@@ -404,6 +405,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
 	 * resolved to actual objects
@@ -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
@@ -735,10 +750,17 @@ class Injector {
 			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);
 	}
 }
 

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 create(Injector $injector, $class, $params = array()) {
+	public function __construct($injector) {
+		$this->injector = $injector;
+	}
+
+	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);
 		}
 	}