Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

BUG Fix issue with inheritance of Injector service configuration #2946

Merged
merged 1 commit into from

3 participants

@tractorcow
Owner

This issue came up in a case where a parent class had a particular set of dependencies specified, and while direct subclasses of this would work fine with Injector, attempts to instantiate sub-subclasses of this would result in missing dependencies.

E.g. A extends B extends C, where only C has dependencies specified, and attempts to create an instance of A would break future calls to B::create().

The SilverStripeServiceConfigurationLocator would walk from A to C finding dependencies, and during this process "B" would be marked with a config of false, meaning future calls to B::create() (or sometimes A::create()) would incorrectly ignore all dependencies.

This fix no longer marks a class as having a false config unless its parent has a false config, or if there are no other parents above this.

SilverStripeServiceConfigurationLocator was also refactored to be a bit more test-case friendly and better documentation given.

This was a fun afternoon trying to debug...

@simonwelsh simonwelsh added the 3.1 label
@tractorcow
Owner

This still needs to be fixed. @hafriedlander would you like to look at this?

@tractorcow tractorcow BUG Fix issue with inheritance of Injector service configuration
96d0874
@tractorcow
Owner

This has been rebased for merge.

@halkyon halkyon merged commit 4ae0d90 into silverstripe:3.1

2 checks passed

Details ci/scrutinizer Scrutinizer: No new issues — Tests: passed
Details continuous-integration/travis-ci The Travis CI build passed
@tractorcow tractorcow deleted the tractorcow:pulls/3.1-fix-injector-inheritance-bug branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 24, 2014
  1. @tractorcow

    BUG Fix issue with inheritance of Injector service configuration

    tractorcow authored tractorcow committed
This page is out of date. Refresh to see the latest.
View
7 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.
View
12 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) {}
}
View
68 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) {
- if (isset($this->configs[$name])) {
- return $this->configs[$name];
- }
-
- $config = Config::inst()->get('Injector', $name);
- if ($config) {
- $this->configs[$name] = $config;
- return $config;
- }
+ // 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?
- if (isset($this->configs[$parent])) {
- return $this->configs[$parent];
- }
- $config = Config::inst()->get('Injector', $parent);
- if ($config) {
+ $config = $this->configFor($parent);
+ if($config !== null) {
+ // Cache this result
$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;
+ }
+
+ // 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]; // Potentially false
+ }
+
+ $config = Config::inst()->get('Injector', $name);
+ if ($config) {
+ $this->configs[$name] = $config;
+ return $config;
+ } else {
+ return null;
}
}
-}
+}
View
58 tests/injector/InjectorTest.php
@@ -486,13 +486,39 @@ public function testCustomObjectCreator() {
}
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 @@ public function testNest() {
}
class InjectorTestConfigLocator extends SilverStripeServiceConfigurationLocator implements TestOnly {
- public function locateConfigFor($name) {
- if ($name == 'TestObject') {
- return array('class' => 'ConstructableObject', 'constructor' => array('%$OtherTestObject'));
- }
-
- if ($name == 'ConfigConstructor') {
- return array('class' => 'ConstructableObject', 'constructor' => array('value'));
+
+ 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')
+ );
}
- return parent::locateConfigFor($name);
+ return parent::configFor($name);
}
}
@@ -731,6 +765,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 {
Something went wrong with that request. Please try again.