From a7fefb16df2604983fa3ed30b30d727c255f28a3 Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 08:34:17 +0100 Subject: [PATCH 1/7] fold `initialize()` into `get()`: reduces depth of recursion --- src/Container.php | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/src/Container.php b/src/Container.php index ae103e7..afbd8dd 100644 --- a/src/Container.php +++ b/src/Container.php @@ -61,7 +61,21 @@ public function get($name) $this->active[$name] = true; - $this->initialize($name); + if (isset($this->config[$name])) { + foreach ($this->config[$name] as $index => $config) { + $map = $this->config_map[$name][$index]; + + $reflection = Reflection::createFromCallable($config); + + $params = $this->resolve($reflection->getParameters(), $map); + + $value = call_user_func_array($config, $params); + + if ($value !== null) { + $this->values[$name] = $value; + } + } + } } return $this->values[$name]; @@ -227,30 +241,4 @@ protected function inject($name, $value) $this->values[$name] = $value; $this->active[$name] = true; } - - /** - * Internally initialize an active component. - * - * @param string $name component name - * - * @return void - */ - private function initialize($name) - { - if (isset($this->config[$name])) { - foreach ($this->config[$name] as $index => $config) { - $map = $this->config_map[$name][$index]; - - $reflection = Reflection::createFromCallable($config); - - $params = $this->resolve($reflection->getParameters(), $map); - - $value = call_user_func_array($config, $params); - - if ($value !== null) { - $this->values[$name] = $value; - } - } - } - } } From 7c31b073c7b67034947e807947f120013f8ef50f Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 08:37:46 +0100 Subject: [PATCH 2/7] remove duplication of `call()` logic from `get()` --- src/Container.php | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/Container.php b/src/Container.php index afbd8dd..82e97e8 100644 --- a/src/Container.php +++ b/src/Container.php @@ -48,13 +48,7 @@ public function get($name) { if (! isset($this->active[$name])) { if (isset($this->factory[$name])) { - $factory = $this->factory[$name]; - - $reflection = new ReflectionFunction($factory); - - $params = $this->resolve($reflection->getParameters(), $this->factory_map[$name]); - - $this->values[$name] = call_user_func_array($factory, $params); + $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]); } elseif (! array_key_exists($name, $this->values)) { throw new NotFoundException($name); } @@ -63,13 +57,7 @@ public function get($name) if (isset($this->config[$name])) { foreach ($this->config[$name] as $index => $config) { - $map = $this->config_map[$name][$index]; - - $reflection = Reflection::createFromCallable($config); - - $params = $this->resolve($reflection->getParameters(), $map); - - $value = call_user_func_array($config, $params); + $value = $this->call($config, $this->config_map[$name][$index]); if ($value !== null) { $this->values[$name] = $value; From c6fec398b4f7505348797c678c65e6243d5d4cc0 Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 09:47:59 +0100 Subject: [PATCH 3/7] detect and throw for dependency cycles --- src/Container.php | 24 ++++++++++++++ test/test.php | 79 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/Container.php b/src/Container.php index 82e97e8..9c4ee27 100644 --- a/src/Container.php +++ b/src/Container.php @@ -18,6 +18,18 @@ class Container extends Configuration implements ContainerInterface, FactoryInte */ protected $active = []; + /** + * @var string[] list of activations (component names) in the order they occurred + * + * @see get() + */ + private $activations = []; + + /** + * @var true[] map of component names currently being activated + */ + private $activation_map = []; + /** * @param Configuration $config */ @@ -47,6 +59,14 @@ public function __construct(Configuration $config) public function get($name) { if (! isset($this->active[$name])) { + $this->activations[] = $name; + + if (isset($this->activation_map[$name])) { + throw new ContainerException("Dependency cycle detected: " . implode(" -> ", $this->activations)); + } + + $this->activation_map[$name] = true; + if (isset($this->factory[$name])) { $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]); } elseif (! array_key_exists($name, $this->values)) { @@ -64,6 +84,10 @@ public function get($name) } } } + + unset($this->activations[$name]); + + array_pop($this->activations); } return $this->values[$name]; diff --git a/test/test.php b/test/test.php index 388dc70..f8044ac 100644 --- a/test/test.php +++ b/test/test.php @@ -664,6 +664,85 @@ function () { } ); +test( + 'can trap direct dependency cycle', + function () { + $factory = new ContainerFactory(); + + $factory->register("a", function ($b) { + return "A{$b}"; + }); + + $factory->register("b", function ($a) { + return "B{$a}"; + }); + + expect( + ContainerException::class, + "should throw for dependency cycle", + function () use ($factory) { + $factory->createContainer()->get("a"); + }, + "{Dependency cycle detected: a -> b -> a}i" + ); + + expect( + ContainerException::class, + "should throw for dependency cycle", + function () use ($factory) { + $factory->createContainer()->get("b"); + }, + "{Dependency cycle detected: b -> a -> b}i" + ); + } +); + +test( + 'can trap indirect dependency cycle', + function () { + $factory = new ContainerFactory(); + + $factory->register("a", function ($b) { + return "A{$b}"; + }); + + $factory->register("b", function ($c) { + return "B{$c}"; + }); + + $factory->register("c", function ($a) { + return "C{$a}"; + }); + + expect( + ContainerException::class, + "should throw for dependency cycle (a)", + function () use ($factory) { + $factory->createContainer()->get("a"); + }, + "{Dependency cycle detected: a -> b -> c -> a}i" + ); + + expect( + ContainerException::class, + "should throw for dependency cycle (b)", + function () use ($factory) { + $factory->createContainer()->get("b"); + }, + "{Dependency cycle detected: b -> c -> a -> b}i" + ); + + expect( + ContainerException::class, + "should throw for dependency cycle (c)", + function () use ($factory) { + $factory->createContainer()->get("c"); + }, + "{Dependency cycle detected: c -> a -> b -> c}i" + ); + } +); + if (version_compare(PHP_VERSION, "7", ">=")) { require __DIR__ . "/test-php70.php"; } else { From 5359324d6aa21cc9bcd76640de79681a7bce09d7 Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 09:51:14 +0100 Subject: [PATCH 4/7] simplify internal model for activations --- src/Container.php | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/Container.php b/src/Container.php index 9c4ee27..f6a114d 100644 --- a/src/Container.php +++ b/src/Container.php @@ -19,17 +19,12 @@ class Container extends Configuration implements ContainerInterface, FactoryInte protected $active = []; /** - * @var string[] list of activations (component names) in the order they occurred + * @var int[] map where component name => activation depth * * @see get() */ private $activations = []; - /** - * @var true[] map of component names currently being activated - */ - private $activation_map = []; - /** * @param Configuration $config */ @@ -59,13 +54,13 @@ public function __construct(Configuration $config) public function get($name) { if (! isset($this->active[$name])) { - $this->activations[] = $name; - - if (isset($this->activation_map[$name])) { - throw new ContainerException("Dependency cycle detected: " . implode(" -> ", $this->activations)); + if (isset($this->activations[$name])) { + throw new ContainerException( + "Dependency cycle detected: " . implode(" -> ", array_flip($this->activations)) . " -> {$name}" + ); } - $this->activation_map[$name] = true; + $this->activations[$name] = count($this->activations) + 1; if (isset($this->factory[$name])) { $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]); @@ -86,8 +81,6 @@ public function get($name) } unset($this->activations[$name]); - - array_pop($this->activations); } return $this->values[$name]; From 34e6be001d642c6ad97b1f842ad3f3dc0c305c10 Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 10:10:58 +0100 Subject: [PATCH 5/7] bugfix: components were being flagged as active before configuration - this could lead to indirect dependency cycles via `configure()`, which could actually resolve to two different instances of the same component: the factory function would get invoked twice! I've added a regression test that proves this was possible. --- src/Container.php | 12 ++++++------ test/test.php | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/Container.php b/src/Container.php index f6a114d..d503ae2 100644 --- a/src/Container.php +++ b/src/Container.php @@ -55,9 +55,9 @@ public function get($name) { if (! isset($this->active[$name])) { if (isset($this->activations[$name])) { - throw new ContainerException( - "Dependency cycle detected: " . implode(" -> ", array_flip($this->activations)) . " -> {$name}" - ); + $activation_path = implode(" -> ", array_flip($this->activations)) . " -> {$name}"; + + throw new ContainerException("Dependency cycle detected: " . $activation_path); } $this->activations[$name] = count($this->activations) + 1; @@ -68,11 +68,9 @@ public function get($name) throw new NotFoundException($name); } - $this->active[$name] = true; - if (isset($this->config[$name])) { foreach ($this->config[$name] as $index => $config) { - $value = $this->call($config, $this->config_map[$name][$index]); + $value = $this->call($config, [$this->values[$name]] + $this->config_map[$name][$index]); if ($value !== null) { $this->values[$name] = $value; @@ -80,6 +78,8 @@ public function get($name) } } + $this->active[$name] = true; + unset($this->activations[$name]); } diff --git a/test/test.php b/test/test.php index f8044ac..cce0ef1 100644 --- a/test/test.php +++ b/test/test.php @@ -743,6 +743,43 @@ function () use ($factory) { } ); +test( + 'can trap indirect dependency cycle via configuration', + function () { + $factory = new ContainerFactory(); + + $factory->register("a", function ($b) { + return "A{$b}"; + }); + + $factory->register("b", function () { + return "B"; + }); + + $factory->configure("b", function ($b, $a) { + return "{$b}{$a}"; + }); + + expect( + ContainerException::class, + "should throw for dependency cycle", + function () use ($factory) { + $factory->createContainer()->get("a"); + }, + "{Dependency cycle detected: a -> b -> a}i" + ); + + expect( + ContainerException::class, + "should throw for dependency cycle", + function () use ($factory) { + $factory->createContainer()->get("b"); + }, + "{Dependency cycle detected: b -> a -> b}i" + ); + } +); + if (version_compare(PHP_VERSION, "7", ">=")) { require __DIR__ . "/test-php70.php"; } else { From bfc797f685669b50539cb2bc1b405602737f91e6 Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 11:32:23 +0100 Subject: [PATCH 6/7] improve error reporting for deep dependency cycles: if we have a dependency a ->b, and there's a cyclical dependency b -> c -> b, we don't want to see "a" in the error message; just b -> c -> b, which is where the problem is (the actual cycle - "a" just happens to be how we got into the cycle) --- src/Container.php | 52 ++++++++++++++++++++++++++++++----------------- test/test.php | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/Container.php b/src/Container.php index d503ae2..2b44835 100644 --- a/src/Container.php +++ b/src/Container.php @@ -54,33 +54,47 @@ public function __construct(Configuration $config) public function get($name) { if (! isset($this->active[$name])) { - if (isset($this->activations[$name])) { - $activation_path = implode(" -> ", array_flip($this->activations)) . " -> {$name}"; + try { + if (isset($this->activations[$name])) { + $activations = array_flip($this->activations); + + ksort($activations, SORT_NUMERIC); + + for ($i=1; $i ", $activations) . " -> {$name}"; - $this->activations[$name] = count($this->activations) + 1; + throw new ContainerException("Dependency cycle detected: " . $activation_path); + } - if (isset($this->factory[$name])) { - $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]); - } elseif (! array_key_exists($name, $this->values)) { - throw new NotFoundException($name); - } + $this->activations[$name] = count($this->activations) + 1; - if (isset($this->config[$name])) { - foreach ($this->config[$name] as $index => $config) { - $value = $this->call($config, [$this->values[$name]] + $this->config_map[$name][$index]); + if (isset($this->factory[$name])) { + $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]); + } elseif (! array_key_exists($name, $this->values)) { + throw new NotFoundException($name); + } - if ($value !== null) { - $this->values[$name] = $value; + if (isset($this->config[$name])) { + foreach ($this->config[$name] as $index => $config) { + $value = $this->call($config, [$this->values[$name]] + $this->config_map[$name][$index]); + + if ($value !== null) { + $this->values[$name] = $value; + } } } - } - $this->active[$name] = true; - - unset($this->activations[$name]); + $this->active[$name] = true; + } finally { + unset($this->activations[$name]); + } } return $this->values[$name]; diff --git a/test/test.php b/test/test.php index cce0ef1..b38c37c 100644 --- a/test/test.php +++ b/test/test.php @@ -740,6 +740,21 @@ function () use ($factory) { }, "{Dependency cycle detected: c -> a -> b -> c}i" ); + + expect( + ContainerException::class, + "should throw for repeated dependency cycle", + function () use ($factory) { + $container = $factory->createContainer(); + + try { + $container->get("c"); + } catch (ContainerException $error) { + $container->get("c"); + } + }, + "{Dependency cycle detected: c -> a -> b -> c}i" + ); } ); @@ -780,6 +795,34 @@ function () use ($factory) { } ); +test( + 'can trap deep dependency cycle', + function () { + $factory = new ContainerFactory(); + + $factory->register("a", function ($b) { + return "A{$b}"; + }); + + $factory->register("b", function ($c) { + return "B{$c}"; + }); + + $factory->register("c", function ($b) { + return "C{$b}"; + }); + + expect( + ContainerException::class, + "should throw for dependency cycle", + function () use ($factory) { + $factory->createContainer()->get("a"); + }, + "{Dependency cycle detected: b -> c -> b}i" + ); + } +); + if (version_compare(PHP_VERSION, "7", ">=")) { require __DIR__ . "/test-php70.php"; } else { From 11af777f796a3c1a67f10d8f5be683bd64dcb12e Mon Sep 17 00:00:00 2001 From: Rasmus Schultz Date: Thu, 7 Feb 2019 12:11:34 +0100 Subject: [PATCH 7/7] simplify error-reporting a bit --- src/Container.php | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/Container.php b/src/Container.php index 2b44835..edfc0c8 100644 --- a/src/Container.php +++ b/src/Container.php @@ -58,22 +58,18 @@ public function get($name) if (isset($this->activations[$name])) { $activations = array_flip($this->activations); - ksort($activations, SORT_NUMERIC); + ksort($activations, SORT_NUMERIC); // order by activation depth - for ($i=1; $i ", $activations) . " -> {$name}"; + $activation_path = implode(" -> ", $activations); throw new ContainerException("Dependency cycle detected: " . $activation_path); } - $this->activations[$name] = count($this->activations) + 1; + $this->activations[$name] = count($this->activations); if (isset($this->factory[$name])) { $this->values[$name] = $this->call($this->factory[$name], $this->factory_map[$name]);