Merge pull request #2946 from tractorcow/pulls/3.1-fix-injector-inheritance-bug

BUG Fix issue with inheritance of Injector service configuration

control/injector/Injector.php

@@ -180,6 +180,13 @@ class Injector {
 	 * @var Factory
 	 */
 	protected $objectCreator;
+	
+	/**
+	 * Locator for determining Config properties for services
+	 * 
+	 * @var ServiceConfigurationLocator 
+	 */
+	protected $configLocator;
 
 	/**
 	 * Create a new injector. 

control/injector/ServiceConfigurationLocator.php

@@ -9,7 +9,13 @@
  * @subpackage injector
  */
 class ServiceConfigurationLocator {
-	public function locateConfigFor($name) {
+	
-		
+	
-	}
+	/**
+	 * Finds the Injector config for a named service.
+	 * 
+	 * @param string $name
+	 * @return mixed
+	 */
+	public function locateConfigFor($name) {}
 }

control/injector/SilverStripeServiceConfigurationLocator.php

@@ -7,43 +7,65 @@
  * @package framework
  * @subpackage injector
  */
-class SilverStripeServiceConfigurationLocator {
+class SilverStripeServiceConfigurationLocator extends ServiceConfigurationLocator {
 	
-	private $configs = array();
+	/**
+	 * List of Injector configurations cached from Config in class => config format.
+	 * If any config is false, this denotes that this class and all its parents 
+	 * have no configuration specified.
+	 * 
+	 * @var array
+	 */
+	protected $configs = array();
 	
 	public function locateConfigFor($name) {
 		
+		// Check direct or cached result
+		$config = $this->configFor($name);
+		if($config !== null) return $config;
+		
+		// do parent lookup if it's a class
+		if (class_exists($name)) {
+			$parents = array_reverse(array_keys(ClassInfo::ancestry($name)));
+			array_shift($parents);
+
+			foreach ($parents as $parent) {
+				// have we already got for this? 
+				$config = $this->configFor($parent);
+				if($config !== null) {
+					// Cache this result
+					$this->configs[$name] = $config;
+					return $config;
+				}
+			}
+		}
+		
+		// there is no parent config, so we'll record that as false so we don't do the expensive
+		// lookup through parents again
+		$this->configs[$name] = false;
+	}
+	
+	/**
+	 * Retrieves the config for a named service without performing a hierarchy walk
+	 * 
+	 * @param string $name Name of service
+	 * @return mixed Returns either the configuration data, if there is any. A missing config is denoted 
+	 * by a value of either null (there is no direct config assigned and a hierarchy walk is necessary)
+	 * or false (there is no config for this class, nor within the hierarchy for this class). 
+	 */
+	protected function configFor($name) {
+		
+		// Return cached result
 		if (isset($this->configs[$name])) {
-			return $this->configs[$name];
+			return $this->configs[$name]; // Potentially false
 		}
 		
 		$config = Config::inst()->get('Injector', $name);
 		if ($config) {
 			$this->configs[$name] = $config;
 			return $config;
-		}
+		} else {
-		
+			return null;
-		// do parent lookup if it's a class
-		if (class_exists($name)) {
-			$parents = array_reverse(array_keys(ClassInfo::ancestry($name)));
-			array_shift($parents);
-			foreach ($parents as $parent) {
-				// have we already got for this? 
-				if (isset($this->configs[$parent])) {
-					return $this->configs[$parent];
-				}
-				$config = Config::inst()->get('Injector', $parent);
-				if ($config) {
-					$this->configs[$name] = $config;
-					return $config;
-				} else {
-					$this->configs[$parent] = false;
-				}
-			}
-			
-			// there is no parent config, so we'll record that as false so we don't do the expensive
-			// lookup through parents again
-			$this->configs[$name] = false;
 		}
 	}
-}
+}

tests/injector/InjectorTest.php

@@ -486,13 +486,39 @@ class InjectorTest extends SapphireTest {
 	}
 
 	public function testInheritedConfig() {
+		
+		// Test top-down caching of config inheritance
 		$injector = new Injector(array('locator' => 'SilverStripeServiceConfigurationLocator'));
 		Config::inst()->update('Injector', 'MyParentClass', array('properties' => array('one' => 'the one')));
+		Config::inst()->update('Injector', 'MyChildClass', array('properties' => array('one' => 'the two')));
 		$obj = $injector->get('MyParentClass');
 		$this->assertEquals($obj->one, 'the one');
 		
 		$obj = $injector->get('MyChildClass');
-		$this->assertEquals($obj->one, 'the one');
+		$this->assertEquals($obj->one, 'the two');
+		
+		$obj = $injector->get('MyGrandChildClass');
+		$this->assertEquals($obj->one, 'the two');
+		
+		$obj = $injector->get('MyGreatGrandChildClass');
+		$this->assertEquals($obj->one, 'the two');
+		
+		// Test bottom-up caching of config inheritance
+		$injector = new Injector(array('locator' => 'SilverStripeServiceConfigurationLocator'));
+		Config::inst()->update('Injector', 'MyParentClass', array('properties' => array('one' => 'the three')));
+		Config::inst()->update('Injector', 'MyChildClass', array('properties' => array('one' => 'the four')));
+		
+		$obj = $injector->get('MyGreatGrandChildClass');
+		$this->assertEquals($obj->one, 'the four');
+		
+		$obj = $injector->get('MyGrandChildClass');
+		$this->assertEquals($obj->one, 'the four');
+		
+		$obj = $injector->get('MyChildClass');
+		$this->assertEquals($obj->one, 'the four');
+		
+		$obj = $injector->get('MyParentClass');
+		$this->assertEquals($obj->one, 'the three');
 	}
 	
 	public function testSameNamedSingeltonPrototype() {
@@ -654,16 +680,24 @@ class InjectorTest extends SapphireTest {
 }
 
 class InjectorTestConfigLocator extends SilverStripeServiceConfigurationLocator implements TestOnly {
-	public function locateConfigFor($name) {
+	
-		if ($name == 'TestObject') {
+	protected function configFor($name) {
-			return array('class' => 'ConstructableObject', 'constructor' => array('%$OtherTestObject'));
+		
+		switch($name) {
+			case 'TestObject':
+				return $this->configs[$name] = array(
+					'class' => 'ConstructableObject',
+					'constructor' => array('%$OtherTestObject')
+				);
+				
+			case 'ConfigConstructor':
+				return $this->configs[$name] = array(
+					'class' => 'ConstructableObject',
+					'constructor' => array('value')
+				);
 		}
 
-		if ($name == 'ConfigConstructor') {
+		return parent::configFor($name);
-			return array('class' => 'ConstructableObject', 'constructor' => array('value'));
-		}
-
-		return parent::locateConfigFor($name);
 	}
 }
 
@@ -730,6 +764,12 @@ class MyParentClass implements TestOnly {
 
 class MyChildClass extends MyParentClass implements TestOnly {
 	
+}
+class MyGrandChildClass extends MyChildClass implements TestOnly {
+	
+}
+class MyGreatGrandChildClass extends MyGrandChildClass implements TestOnly {
+	
 }
 
 class DummyRequirements implements TestOnly {