Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Dec 11, 2017

Conversation

@nicolas-grekas
Copy link
Member

commented Dec 7, 2017

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
Copy link
Contributor

left a comment

Brilliant!

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

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

@internal?

@@ -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)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

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.

{
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)));

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

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?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 7, 2017

Author Member

see new test case :)

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 7, 2017

Author Member

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

$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)));

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

Maybe:

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

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Dec 7, 2017

Member

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

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

This comment has been minimized.

Copy link
@stof

stof Dec 7, 2017

Member

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

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

This comment has been minimized.

Copy link
@stof

stof Dec 7, 2017

Member

typo here depencency vs dependency

@@ -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);

This comment has been minimized.

Copy link
@stof

stof Dec 7, 2017

Member

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-dx-locator branch 8 times, most recently from 6d2f355 to d34e760 Dec 7, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2017

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

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

@ro0NL

ro0NL approved these changes Dec 9, 2017

Copy link
Contributor

left a comment

next level error messages :) i like it.

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);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 9, 2017

Contributor

with DEBUG_BACKTRACE_IGNORE_ARGS or is it negligible?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 9, 2017

Author Member

good idea, added

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

@chalasr

chalasr approved these changes Dec 9, 2017

Copy link
Member

left a comment

nice!

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

This comment has been minimized.

Copy link
@chalasr

chalasr Dec 9, 2017

Member

extra spaces

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-dx-locator branch from 27d8d69 to 9512f26 Dec 9, 2017

@weaverryan
Copy link
Member

left a comment

👍 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

fabpot approved these changes Dec 11, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

Thank you @nicolas-grekas.

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

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

fabpot added a commit that referenced this pull request Dec 11, 2017

bug #25381 [DI] Add context to service-not-found exceptions thrown by…
… 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 nicolas-grekas deleted the nicolas-grekas:di-dx-locator branch Dec 11, 2017

This was referenced Dec 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.