From 96d0874953b1009fce4f88821c9a63c5bfc165a4 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 12 Mar 2014 14:58:49 +1300 Subject: [PATCH] BUG Fix issue with inheritance of Injector service configuration --- control/injector/Injector.php | 7 ++ .../injector/ServiceConfigurationLocator.php | 12 ++- ...ilverStripeServiceConfigurationLocator.php | 76 ++++++++++++------- tests/injector/InjectorTest.php | 58 +++++++++++--- 4 files changed, 114 insertions(+), 39 deletions(-) diff --git a/control/injector/Injector.php b/control/injector/Injector.php index 2a7470bf7..9d1b103e7 100644 --- a/control/injector/Injector.php +++ b/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. diff --git a/control/injector/ServiceConfigurationLocator.php b/control/injector/ServiceConfigurationLocator.php index 3bcc00404..30f171703 100644 --- a/control/injector/ServiceConfigurationLocator.php +++ b/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) {} } \ No newline at end of file diff --git a/control/injector/SilverStripeServiceConfigurationLocator.php b/control/injector/SilverStripeServiceConfigurationLocator.php index faf86d755..a0a22bd25 100644 --- a/control/injector/SilverStripeServiceConfigurationLocator.php +++ b/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; - } - - // 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; + } else { + return null; } } -} \ No newline at end of file +} diff --git a/tests/injector/InjectorTest.php b/tests/injector/InjectorTest.php index 2e48fe1f0..2c3ceafb2 100644 --- a/tests/injector/InjectorTest.php +++ b/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') { - return array('class' => 'ConstructableObject', 'constructor' => array('%$OtherTestObject')); + + protected function configFor($name) { + + 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 array('class' => 'ConstructableObject', 'constructor' => array('value')); - } - - return parent::locateConfigFor($name); + return parent::configFor($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 {