[DI] Add context to service-not-found exceptions thrown by service locators #25381

nicolas-grekas · 2017-12-07T16:31:42Z

Q	A
Branch?	3.4
Bug fix?	yes (DX)
New feature?	yes (...)
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25342, #25196
License	MIT
Doc PR	-

Here hopefully is the fully-context aware message you're looking for @weaverryan @curry684.

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()".

curry684

Brilliant!

weaverryan · 2017-12-07T17:15:25Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

@@ -31,6 +36,15 @@ public function __construct(array $factories)
        $this->factories = $factories;
    }

+    public function withContext($externalId, Container $container)


@internal?

weaverryan · 2017-12-07T17:20:25Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php

@@ -94,6 +97,15 @@ public static function register(ContainerBuilder $container, array $refMap)
            $container->setDefinition($id, $locator);
        }

+        if (null !== $callerId = 2 < func_num_args() ? func_get_arg(2) : null) {
+            $locatorId = $id;
+            $container->register($id .= '.'.$callerId, ServiceLocator::class)


I know you hate it when I suggest this, but a comment above this explaining what's going on would help. I can't locate where and why registering this service is significant.

weaverryan · 2017-12-07T17:23:27Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+    {
+        if ($this->loading) {
+            $msg = sprintf('The service "%s" has a dependency on a non-existent service "%s".', end($this->loading), $id);
+            $msg .= sprintf(' This locator only knows about "%s"', implode('", "', array_keys($this->factories)));


How does this situation happen? Once I use $this->container->get('foo'), if foo depends on a non-existent bar service, wouldn't the container have already exploded at build time?

see new test case :)

weaverryan · 2017-12-07T17:25:37Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+        } else {
+            $class = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 3);
+            $class = isset($class['object'][2]) ? get_class($class['object'][2]) : null;
+            $externalId = $this->externalId ?: $class;


When would $externalId be set versus not set? It obviously makes a difference in the error message below, so just curious.

Eg the locator is used from a function.
But in practice, should not happen to users.

weaverryan · 2017-12-07T17:27:06Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+                $msg .= 'even though it exists in the app\'s container, ';
+            }
+            if ($externalId) {
+                $msg .= sprintf('"%s" only knows about pre-listed services "%s".', $externalId, implode('", "', array_keys($this->factories)));


Maybe:

The container inside "%s" is a smaller service locator that only contains the following services: "%s".

weaverryan · 2017-12-07T17:31:24Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+            }
+
+            if ($class && is_subclass_of($class, ServiceSubscriberInterface::class)) {
+                $msg .= sprintf(' Did you forget to declare the depencency using "%s::getSubscribedServices()"?', preg_replace('/([^\\\\]++\\\\)++/', '', $class));


Not to go too crazy, but if the actual class that implements ServiceSubscriberInterface is a vendor, then this error message doesn't make much sense. The screenshot of the error in the PR is a good example of this - the correct solution for a controller is probably not to override getSubscribedServices().

For the controller situations specifically (which probably would work for any case where getSubscribedServices() is actually implemented by a vendor class), the better message might be:

Instead of using the container, try using dependency injection to get the service.

We could go even more bonkers and do something (for AbstractController) about type-hinting something on the controller... but probably that's overboard. We just need to say "Hey! Stop using the container! Do what you see recommended everywhere in the docs instead".

stof · 2017-12-07T18:15:05Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php

     */
-    public static function register(ContainerBuilder $container, array $refMap)
+    public static function register(ContainerBuilder $container, array $refMap/*, string $callerId = null*/)


the class is final (not even @final but actually final), so you can add the new argument without this magic

stof · 2017-12-07T18:17:17Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+            }
+
+            if ($class && is_subclass_of($class, ServiceSubscriberInterface::class)) {
+                $msg .= sprintf(' Did you forget to declare the depencency using "%s::getSubscribedServices()"?', preg_replace('/([^\\\\]++\\\\)++/', '', $class));


typo here depencency vs dependency

stof · 2017-12-07T18:17:56Z

src/Symfony/Component/HttpKernel/HttpKernel.php

@@ -150,7 +150,7 @@ private function handleRaw(Request $request, $type = self::MASTER_REQUEST)
        $arguments = $event->getArguments();

        // call controller
-        $response = call_user_func_array($controller, $arguments);
+        $response = \call_user_func_array($controller, $arguments);


I suggest submitting this to 2.7 instead. It is unrelated to this PR.

nicolas-grekas · 2017-12-07T18:50:43Z

@stof @weaverryan thanks for the review, comments addressed.

nicolas-grekas · 2017-12-08T12:37:35Z

PR green and ready.
We could even improve the message further, by suggesting what to type-hint for.
But this requires some code infrastructure that we don't have right now, so for 4.1 if someone wants to make it happen after merge.

Status: needs review

ro0NL

next level error messages :) i like it.

ro0NL · 2017-12-09T10:00:56Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+            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, 3);


with DEBUG_BACKTRACE_IGNORE_ARGS or is it negligible?

good idea, added

chalasr

nice!

chalasr · 2017-12-09T19:29:38Z

src/Symfony/Component/DependencyInjection/ServiceLocator.php

+        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());
+        }
+


extra spaces

…cators

weaverryan

👍 for the messaging as shown on the test cases. The technical details are a bit complex, but this only touches the messaging and is well-covered with test cases.

fabpot · 2017-12-11T20:28:37Z

Thank you @nicolas-grekas.

@weaverryan

… service locators (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add context to service-not-found exceptions thrown by service locators | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes (DX) | New feature? | yes (...) | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25342, #25196 | License | MIT | Doc PR | - Here hopefully is the fully-context aware message you're looking for @weaverryan @curry684. ![image](https://user-images.githubusercontent.com/243674/33726013-1db38a34-db74-11e7-91dd-ca9d53e58891.png) Commits ------- 9512f26 [DI] Add context to service-not-found exceptions thrown by service locators

nicolas-grekas added this to the 3.4 milestone Dec 7, 2017

nicolas-grekas mentioned this pull request Dec 7, 2017

[DI] Improve service not found in service subscribers #25261

Closed

carsonbot added the Status: Reviewed label Dec 7, 2017

curry684 approved these changes Dec 7, 2017

View reviewed changes

nicolas-grekas force-pushed the di-dx-locator branch 2 times, most recently from 3afa88b to 4c056d9 Compare December 7, 2017 17:19

weaverryan requested changes Dec 7, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Dec 7, 2017

stof requested changes Dec 7, 2017

View reviewed changes

nicolas-grekas force-pushed the di-dx-locator branch 8 times, most recently from 6d2f355 to d34e760 Compare December 8, 2017 12:36

carsonbot added Status: Needs Review Status: Reviewed and removed Status: Needs Work Status: Needs Review labels Dec 8, 2017

Simperfit approved these changes Dec 9, 2017

View reviewed changes

nicolas-grekas force-pushed the di-dx-locator branch 2 times, most recently from 28e1964 to 60cd0f6 Compare December 9, 2017 09:06

ro0NL approved these changes Dec 9, 2017

View reviewed changes

nicolas-grekas force-pushed the di-dx-locator branch from 60cd0f6 to 27d8d69 Compare December 9, 2017 10:10

chalasr approved these changes Dec 9, 2017

View reviewed changes

[DI] Add context to service-not-found exceptions thrown by service lo…

9512f26

…cators

nicolas-grekas force-pushed the di-dx-locator branch from 27d8d69 to 9512f26 Compare December 9, 2017 19:32

weaverryan approved these changes Dec 11, 2017

View reviewed changes

fabpot approved these changes Dec 11, 2017

View reviewed changes

fabpot merged commit 9512f26 into symfony:3.4 Dec 11, 2017

nicolas-grekas deleted the di-dx-locator branch December 11, 2017 20:32

This was referenced Dec 15, 2017

Release v3.4.2 #25505

Merged

Release v4.0.2 #25507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add context to service-not-found exceptions thrown by service locators #25381

[DI] Add context to service-not-found exceptions thrown by service locators #25381

nicolas-grekas commented Dec 7, 2017 •

edited

Loading

curry684 left a comment

weaverryan Dec 7, 2017

weaverryan Dec 7, 2017

weaverryan Dec 7, 2017

nicolas-grekas Dec 7, 2017

weaverryan Dec 7, 2017

nicolas-grekas Dec 7, 2017

weaverryan Dec 7, 2017

weaverryan Dec 7, 2017

stof Dec 7, 2017

stof Dec 7, 2017

stof Dec 7, 2017

nicolas-grekas commented Dec 7, 2017

nicolas-grekas commented Dec 8, 2017

ro0NL left a comment

ro0NL Dec 9, 2017

nicolas-grekas Dec 9, 2017

chalasr left a comment

chalasr Dec 9, 2017

weaverryan left a comment

fabpot commented Dec 11, 2017

[DI] Add context to service-not-found exceptions thrown by service locators #25381

[DI] Add context to service-not-found exceptions thrown by service locators #25381

Conversation

nicolas-grekas commented Dec 7, 2017 • edited Loading

curry684 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 7, 2017

nicolas-grekas commented Dec 8, 2017

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

fabpot commented Dec 11, 2017

nicolas-grekas commented Dec 7, 2017 •

edited

Loading