Improve error reporting when targetting the wrong Request class

[nicolas-grekas]

When I create a controller action with a Request $request argument, the IDE will suggest me to use Symfony\Component\BrowserKit\Request first. If I'm not careful and select this, I'll get an error saying this:

Cannot autowire argument $request of "App\Controller[...]()": it references class "Symfony\Component\BrowserKit\Request" but no such service exists.

That's not good enough. Instead, it'd be better to tell something like "did you mean to use Symfony\Component\HttpFoundation\Request instead?". The same could happen with any other "Request" class of course.

The current error message comes from ServiceValueResolver trying to access an errored "request" service. This looks wrong also since this prevents any other resolver (lower priority ones) from resolving the argument. The error I would expect instead is the one thrown by ArgumentResolver itself when the resolver chain is over:

Controller "App\Controller[...]" requires that you provide a value for the "$request" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or there is a non-optional argument after this one.

My proposal to fix both issues would be to introduce a special kind of exceptions that ArgumentResolver would use to compute its error message if no other resolver resolved the argument.

Then, we would make RequestValueResolver throw this exception when a type ends with Request but has the wrong namespace for example.

WDYT?

Help wanted :)

[nicolas-grekas]

I figured out that RemoveEmptyControllerArgumentLocatorsPass could also remove all errored services in non-debug mode, so that we don't generated locators that only contain errored references.

[ilyachase]

Hi @nicolas-grekas I'm trying to tackle the issue and on the 7.1 branch when trying to reproduce, I'm getting this error right away:

Controller "App\Controller\LuckyController::number" requires that you provide a value for the "$request" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or there is a non-optional argument after this one.

that comes from vendor/symfony/http-kernel/Controller/ArgumentResolver.php:102. I'm not getting an error from ServiceValueResolver.

Could you please post a full trace of the error so I could trace back why it's not happening the same way for me?

[nicolas-grekas]

I'm doing these steps to reproduce:

symfony new test --webapp
cd test
symfony make:controller hello
# then add Symfony\Component\BrowserKit\Request as argument to HelloController::index()

Going to /hello yields "Cannot autowire argument $request [...]"

[ilyachase]

@nicolas-grekas I reproduced it, thanks. Now the question regarding this part:

My proposal to fix both issues would be to introduce a special kind of exceptions that ArgumentResolver would use to compute its error message if no other resolver resolved the argument. Then, we would make RequestValueResolver throw this exception when a type ends with Request but has the wrong namespace for example.

Problem is, ServiceValueResolver breaks the chain by throwing an exception whenever it fails to fetch a service, and there is no logic inside ArgumentResolver to handle that, while other resolvers return [] in such cases. So I see two ways:

1. Collect exceptions from resolvers inside ArgumentResolver and then use them to somehow compute a final exception (maybe using exceptions chain). But then there is no consistency - some resolvers return [], some throw an exception.
2. Fix ServiceValueResolver so instead of throwing a RuntimeException, it returns an empty array as the rest of the resolvers, but then make so ArgumentResolver throws a RuntimeException with the same message by default, with a special case for Request arguments.

I personally prefer second way but I may lack context since it's my first contribution.

[nicolas-grekas]

You've go it right. The first way is basically what I describe in this PR so that's my preference. It improves things by providing a way for resolvers to give more accurate feedback in case an argument could not be found but a resolver "thought" it was still the one targeted.

[ilyachase]

@nicolas-grekas Awesome! I will follow the first way then. Quick question - in a hypothetical case when multiple resolvers throw an exception, is there a preferred way how to combine them in a single one?

[nicolas-grekas]

Not exactly, maybe a list with an intro saying "Here are some possible hints:". We'll see :)

[ilyachase]

@nicolas-grekas I've created a PR. The error will look like this:

Let me know if you prefer another format 👍