From 9512f268f4fb5164eee59731adaabba464b7c0df Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Wed, 6 Dec 2017 21:55:31 +0100 Subject: [PATCH] [DI] Add context to service-not-found exceptions thrown by service locators --- .../RegisterServiceSubscribersPass.php | 2 +- .../Compiler/ServiceLocatorTagPass.php | 15 +++- .../DependencyInjection/ServiceLocator.php | 90 +++++++++++++++++-- .../RegisterServiceSubscribersPassTest.php | 4 +- .../Fixtures/php/services_subscriber.php | 5 +- .../Tests/ServiceLocatorTest.php | 69 ++++++++++++-- 6 files changed, 165 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php b/src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php index f8dba86a0b547..4ed990ac08d2b 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.php @@ -94,7 +94,7 @@ protected function processValue($value, $isRoot = false) throw new InvalidArgumentException(sprintf('Service %s not exist in the map returned by "%s::getSubscribedServices()" for service "%s".', $message, $class, $this->currentId)); } - $value->addTag('container.service_subscriber.locator', array('id' => (string) ServiceLocatorTagPass::register($this->container, $subscriberMap))); + $value->addTag('container.service_subscriber.locator', array('id' => (string) ServiceLocatorTagPass::register($this->container, $subscriberMap, $this->currentId))); return parent::processValue($value); } diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php index 2c4a09f8476f9..fb32d501ac29e 100644 --- a/src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php +++ b/src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php @@ -72,10 +72,11 @@ protected function processValue($value, $isRoot = false) /** * @param ContainerBuilder $container * @param Reference[] $refMap + * @param string|null $callerId * * @return Reference */ - public static function register(ContainerBuilder $container, array $refMap) + public static function register(ContainerBuilder $container, array $refMap, $callerId = null) { foreach ($refMap as $id => $ref) { if (!$ref instanceof Reference) { @@ -94,6 +95,18 @@ public static function register(ContainerBuilder $container, array $refMap) $container->setDefinition($id, $locator); } + if (null !== $callerId) { + $locatorId = $id; + // Locators are shared when they hold the exact same list of factories; + // to have them specialized per consumer service, we use a cloning factory + // to derivate customized instances from the prototype one. + $container->register($id .= '.'.$callerId, ServiceLocator::class) + ->setPublic(false) + ->setFactory(array(new Reference($locatorId), 'withContext')) + ->addArgument($callerId) + ->addArgument(new Reference('service_container')); + } + return new Reference($id); } } diff --git a/src/Symfony/Component/DependencyInjection/ServiceLocator.php b/src/Symfony/Component/DependencyInjection/ServiceLocator.php index 270de6b935630..324cccab98913 100644 --- a/src/Symfony/Component/DependencyInjection/ServiceLocator.php +++ b/src/Symfony/Component/DependencyInjection/ServiceLocator.php @@ -22,6 +22,9 @@ class ServiceLocator implements PsrContainerInterface { private $factories; + private $loading = array(); + private $externalId; + private $container; /** * @param callable[] $factories @@ -45,18 +48,22 @@ public function has($id) public function get($id) { if (!isset($this->factories[$id])) { - throw new ServiceNotFoundException($id, null, null, array_keys($this->factories)); + throw new ServiceNotFoundException($id, end($this->loading) ?: null, null, array(), $this->createServiceNotFoundMessage($id)); } - if (true === $factory = $this->factories[$id]) { - throw new ServiceCircularReferenceException($id, array($id, $id)); + if (isset($this->loading[$id])) { + $ids = array_values($this->loading); + $ids = array_slice($this->loading, array_search($id, $ids)); + $ids[] = $id; + + throw new ServiceCircularReferenceException($id, $ids); } - $this->factories[$id] = true; + $this->loading[$id] = $id; try { - return $factory(); + return $this->factories[$id](); } finally { - $this->factories[$id] = $factory; + unset($this->loading[$id]); } } @@ -64,4 +71,75 @@ public function __invoke($id) { return isset($this->factories[$id]) ? $this->get($id) : null; } + + /** + * @internal + */ + public function withContext($externalId, Container $container) + { + $locator = clone $this; + $locator->externalId = $externalId; + $locator->container = $container; + + return $locator; + } + + private function createServiceNotFoundMessage($id) + { + if ($this->loading) { + return sprintf('The service "%s" has a dependency on a non-existent service "%s". This locator %s', end($this->loading), $id, $this->formatAlternatives()); + } + + $class = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT | DEBUG_BACKTRACE_IGNORE_ARGS, 3); + $class = isset($class[2]['object']) ? get_class($class[2]['object']) : null; + $externalId = $this->externalId ?: $class; + + $msg = sprintf('Service "%s" not found: ', $id); + + if (!$this->container) { + $class = null; + } elseif ($this->container->has($id) || isset($this->container->getRemovedIds()[$id])) { + $msg .= 'even though it exists in the app\'s container, '; + } else { + try { + $this->container->get($id); + $class = null; + } catch (ServiceNotFoundException $e) { + if ($e->getAlternatives()) { + $msg .= sprintf(' did you mean %s? Anyway, ', $this->formatAlternatives($e->getAlternatives(), 'or')); + } else { + $class = null; + } + } + } + if ($externalId) { + $msg .= sprintf('the container inside "%s" is a smaller service locator that %s', $externalId, $this->formatAlternatives()); + } else { + $msg .= sprintf('the current service locator %s', $this->formatAlternatives()); + } + + if (!$class) { + // no-op + } elseif (is_subclass_of($class, ServiceSubscriberInterface::class)) { + $msg .= sprintf(' Unless you need extra laziness, try using dependency injection instead. Otherwise, you need to declare it using "%s::getSubscribedServices()".', preg_replace('/([^\\\\]++\\\\)++/', '', $class)); + } else { + $msg .= 'Try using dependency injection instead.'; + } + + return $msg; + } + + private function formatAlternatives(array $alternatives = null, $separator = 'and') + { + $format = '"%s"%s'; + if (null === $alternatives) { + if (!$alternatives = array_keys($this->factories)) { + return 'is empty...'; + } + $format = sprintf('only knows about the %s service%s.', $format, 1 < count($alternatives) ? 's' : ''); + } + $last = array_pop($alternatives); + + return sprintf($format, $alternatives ? implode('", "', $alternatives) : $last, $alternatives ? sprintf(' %s "%s"', $separator, $last) : ''); + } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php index 957a04c6c48c7..42f994e821d97 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php @@ -85,7 +85,7 @@ public function testNoAttributes() 'baz' => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)), ); - $this->assertEquals($expected, $locator->getArgument(0)); + $this->assertEquals($expected, $container->getDefinition((string) $locator->getFactory()[0])->getArgument(0)); } public function testWithAttributes() @@ -115,7 +115,7 @@ public function testWithAttributes() 'baz' => new ServiceClosureArgument(new TypedReference(CustomDefinition::class, CustomDefinition::class, TestServiceSubscriber::class, ContainerInterface::IGNORE_ON_INVALID_REFERENCE)), ); - $this->assertEquals($expected, $locator->getArgument(0)); + $this->assertEquals($expected, $container->getDefinition((string) $locator->getFactory()[0])->getArgument(0)); } /** diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php index 967d8012680bc..f682e9752ca6e 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php +++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php @@ -45,6 +45,7 @@ public function getRemovedIds() 'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true, 'Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition' => true, 'service_locator.jmktfsv' => true, + 'service_locator.jmktfsv.foo_service' => true, ); } @@ -82,7 +83,7 @@ protected function getTestServiceSubscriberService() */ protected function getFooServiceService() { - return $this->services['foo_service'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber(new \Symfony\Component\DependencyInjection\ServiceLocator(array('Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition' => function () { + return $this->services['foo_service'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber(\call_user_func(array(new \Symfony\Component\DependencyInjection\ServiceLocator(array('Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition' => function () { $f = function (\Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition $v = null) { return $v; }; return $f(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition()) && false ?: '_'}); }, 'Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\TestServiceSubscriber' => function () { $f = function (\Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber $v) { return $v; }; return $f(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber()) && false ?: '_'}); @@ -90,7 +91,7 @@ protected function getFooServiceService() $f = function (\Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition $v) { return $v; }; return $f(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber()) && false ?: '_'}); }, 'baz' => function () { $f = function (\Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition $v = null) { return $v; }; return $f(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition()) && false ?: '_'}); - }))); + })), 'withContext'), 'foo_service', $this)); } /** diff --git a/src/Symfony/Component/DependencyInjection/Tests/ServiceLocatorTest.php b/src/Symfony/Component/DependencyInjection/Tests/ServiceLocatorTest.php index a1e2fff50fdbe..56fac643eb40b 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ServiceLocatorTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ServiceLocatorTest.php @@ -12,8 +12,9 @@ namespace Symfony\Component\DependencyInjection\Tests; use PHPUnit\Framework\TestCase; -use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException; +use Symfony\Component\DependencyInjection\Container; use Symfony\Component\DependencyInjection\ServiceLocator; +use Symfony\Component\DependencyInjection\ServiceSubscriberInterface; class ServiceLocatorTest extends TestCase { @@ -59,7 +60,7 @@ public function testGetDoesNotMemoize() /** * @expectedException \Psr\Container\NotFoundExceptionInterface - * @expectedExceptionMessage You have requested a non-existent service "dummy". Did you mean one of these: "foo", "bar"? + * @expectedExceptionMessage Service "dummy" not found: the container inside "Symfony\Component\DependencyInjection\Tests\ServiceLocatorTest" is a smaller service locator that only knows about the "foo" and "bar" services. */ public function testGetThrowsOnUndefinedService() { @@ -68,13 +69,50 @@ public function testGetThrowsOnUndefinedService() 'bar' => function () { return 'baz'; }, )); - try { - $locator->get('dummy'); - } catch (ServiceNotFoundException $e) { - $this->assertSame(array('foo', 'bar'), $e->getAlternatives()); + $locator->get('dummy'); + } + + /** + * @expectedException \Psr\Container\NotFoundExceptionInterface + * @expectedExceptionMessage The service "foo" has a dependency on a non-existent service "bar". This locator only knows about the "foo" service. + */ + public function testThrowsOnUndefinedInternalService() + { + $locator = new ServiceLocator(array( + 'foo' => function () use (&$locator) { return $locator->get('bar'); }, + )); + + $locator->get('foo'); + } + + /** + * @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException + * @expectedExceptionMessage Circular reference detected for service "bar", path: "bar -> baz -> bar". + */ + public function testThrowsOnCircularReference() + { + $locator = new ServiceLocator(array( + 'foo' => function () use (&$locator) { return $locator->get('bar'); }, + 'bar' => function () use (&$locator) { return $locator->get('baz'); }, + 'baz' => function () use (&$locator) { return $locator->get('bar'); }, + )); - throw $e; - } + $locator->get('foo'); + } + + /** + * @expectedException \Psr\Container\NotFoundExceptionInterface + * @expectedExceptionMessage Service "foo" not found: even though it exists in the app's container, the container inside "caller" is a smaller service locator that only knows about the "bar" service. Unless you need extra laziness, try using dependency injection instead. Otherwise, you need to declare it using "SomeServiceSubscriber::getSubscribedServices()". + */ + public function testThrowsInServiceSubscriber() + { + $container = new Container(); + $container->set('foo', new \stdClass()); + $subscriber = new SomeServiceSubscriber(); + $subscriber->container = new ServiceLocator(array('bar' => function () {})); + $subscriber->container = $subscriber->container->withContext('caller', $container); + + $subscriber->getFoo(); } public function testInvoke() @@ -89,3 +127,18 @@ public function testInvoke() $this->assertNull($locator('dummy'), '->__invoke() should return null on invalid service'); } } + +class SomeServiceSubscriber implements ServiceSubscriberinterface +{ + public $container; + + public function getFoo() + { + return $this->container->get('foo'); + } + + public static function getSubscribedServices() + { + return array('bar' => 'stdClass'); + } +}