8000 [DependencyInjection] Bad error message for unused bind under _defaults · Issue #27828 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Bad error message for unused bind under _defaults #27828

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.

Sign up for GitHub

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

Jump to bottom
Closed
weaverryan opened this issue Jul 3, 2018 · 8 comments
Closed

[DependencyInjection] Bad error message for unused bind under _defaults #27828

weaverryan opened this issue Jul 3, 2018 · 8 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@weaverryan
Copy link
Member

Symfony version(s) affected: 3.4

How to reproduce

Bind a value beneath _defaults or _instanceof:

# config/services.yaml
services:
    _defaults:
        # ...
        bind:
            $variableName: 'someValue'

If this bind is not actually used, the error is attached to the last service that the rule was applied to. So, something like:

Unused binding "$variableName" in service ".abstract.instanceof.App\Twig\AppExtension".

A better error would be:

You have a "bind" configured for an argument named $variableName. But, this argument was not found in any of the services it was applied to. The bind may be unused and can be removed. Or, the bind may have a typo.
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jul 3, 2018 •
edited
Loading

See #27834 for the noisy .abstract.instanceof. prefix.

On the implementation side, here is an idea: in loaders, track changes to bindings as already done for e.g. "public". When no changes are recorded but bindings are found on a definition, it means they come from some "defaults". When changes were recorded for a definition, the binding was on the definition itself.

PR welcome :)

@Loupax
Copy link
Loupax commented Jul 25, 2018

Not 100% sure it's related, if not I'll create a different issue.

This also happens when you bind a variable to a specific service. In my case where I inject my dependencies on the controller actions rather than the controller constructor what happens is the following:

services.yml:

App\Controller\UIController:
        bind:
            $displayCurrencies: '%display_currencies%'

UIController.php

namespace App\Controller;

final class UIController extends Controller{
    public function myAction(Request $request, $displayCurrencies){
      ...
    }
}

After the latest update I started getting the error mentioned on the issue, even the bound variable is actually used

@nicolas-grekas
Copy link
Member

@Loupax see #28041

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Sep 7, 2018
@przemyslaw-bogusz
Copy link
Contributor

I have tried to implement the solution proposed by @nicolas-grekas and the problem is that in YamlFileLoader, both, binds from defaults and binds from a service are merged before being set on a definition:

symfony/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

Lines 551 to 564 in 3be0445

if (isset($defaults['bind']) || isset($service['bind'])) {
// deep clone, to avoid multiple process of the same instance in the passes
$bindings = isset($defaults['bind']) ? unserialize(serialize($defaults['bind'])) : array();
if (isset($service['bind'])) {
if (!\is_array($service['bind'])) {
throw new InvalidArgumentException(sprintf('Parameter "bind" must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file));
}
$bindings = array_merge($bindings, $this->resolveServices($service['bind'], $file));
}
$definition->setBindings($bindings);
}

As I understand, in order to implement the solution, we would have to ignore the merge and set the binds in two passes in order to track a change. Regardless of the fact, if ignoring the merge is a good idea, this solution does not solve the problem completly. Please consider a following configuration:

# config/services.yaml
services:
    _defaults:
        # ...
        bind:
            $defaultsVariable: 'defaultsValue'
    _instanceof:
        App\Controller\DefaultController:
            bind:
                $instanceVariable: 'instanceValue'
    App\Controller\DefaultController:
        bind:
            $serviceVariable: 'serviceValue'

In this case, a bind may come from three different places, and thus maybe it would be a good solution to offer three different error messages? This can be achieved by adding an extra parameter to BoundArgument. However, this either requires, either:

  1. passing an extra parameter in YamlFileLoader when calling $definition->setBindings, or
  2. adding a property to Definition (e.g. bindOrigin) which could be set before calling setBindings.

Taking into account Symfony's performance, am I correct in assuming, that the first solution is better?

@przemyslaw-bogusz przemyslaw-bogusz mentioned this issue Dec 30, 2018
Merged
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 16, 2019
@przemyslaw-bogusz
Fixes issues symfony#27828 and symfony#28326
cc7dd55
@przemyslaw-bogusz przemyslaw-bogusz mentioned this issue Jan 16, 2019
Closed
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 16, 2019
@przemyslaw-bogusz
Fixes issues symfony#27828 and symfony#28326
08163ae
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 16, 2019
@przemyslaw-bogusz
Fixes issues symfony#27828 and symfony#28326
5c037b9
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 16, 2019
@przemyslaw-bogusz
Fixes issues symfony#27828 and symfony#28326
3378889
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 19, 2019
@przemyslaw-bogusz
[DI] Fixes: symfony#27828 - Bad error message for unused bind under _…
3895cab 
…defaults
@przemyslaw-bogusz przemyslaw-bogusz mentioned this issue Jan 19, 2019
Merged
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 20, 2019
@przemyslaw-bogusz
[DI] Fixes: symfony#27828 - Bad error message for unused bind under _…
cd88f77 
…defaults
przemyslaw-bogusz added a commit to przemyslaw-bogusz/symfony that referenced this issue Jan 20, 2019
@przemyslaw-bogusz
[DI] Fixes: symfony#27828 - Bad error message for unused bind under _…
7114689 
…defaults
@Arkemlar
Copy link

I just updated symfony:
- Updating symfony/dependency-injection (v4.2.2 => v4.2.3): Loading from cache
Before update I had no errors about unused default bindings, but after update they appeared.

@spackmat
Copy link
Contributor

@Arkemlar In my case (also updated to 4.2.3), the error is simply wrong, as the bound parameter is used by a service. In this case:

services.yaml:

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false
        bind:
            string $defaultLocale: '%kernel.default_locale%'
    App\EventSubscriber\:
        resource: ../src/EventSubscriber

In src/EventSubscriber/LocaleSubscriber.php:

public function __construct($defaultLocale = 'de')

Solution was to chance the constructor signature to

public function __construct(string $defaultLocale = 'de')

This way, the binding works again. Before 4.2.3 this was not a problem. Maybe this is helpful for your problem, too.

@przemyslaw-bogusz
Copy link
Contributor

@spackmat In your example you are binding by type and name, so the exception message is correct, because your initial configuration indicated, that you wanted to bind a certain value to an argument named $defaultLocale, but only when it was typehinted as string.

If you put just:

bind:
    $defaultLocale: '%kernel.default_locale%'

in your configuration, it would also solve the problem as the value would be bound to any argument named $defaultLocale, regardless of its type.

@Arkemlar What's the service configuration in your case?

@spackmat
Copy link
Contributor

@przemyslaw-bogusz Sorry, my "wrong" was misleading. Of course there was an error in my code. It was meant from the point of view that before 4.2.3, it worked even with this kind of mistake.

@nicolas-grekas nicolas-grekas removed the Help wanted Issues and PRs which are looking for volunteers to complete them. label Feb 25, 2019
nicolas-grekas added a commit that referenced this issue Apr 7, 2019
@nicolas-grekas
feature #29935 [DI] Fix bad error message for unused bind under _defa…
b0989aa 
…ults (przemyslaw-bogusz)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] Fix bad error message for unused bind under _defaults

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27828
| License       | MIT

**Sidenote**: I originally included the fix in #29897, but I decided to close the previous PR and divide it into two separate PRs for clarity.

**Description:**
With this fix, the message regarding an unused bind will have a clear information about the type of the bind (defined under __defaults_, __instanceof_ or _per service_), as well as the name of the file, in which it was configurated. It's for, both, YAML and XML configurations.

For the core team, please note, that the fix assumes a possibility of definings binds under __instanceof_, which was introduced in #27806. But since fixes are merged into other branches, I thought that it might be necessary to inlude this possibility. If this case requires making separate fixes for different branches, I will gladly do it.

Commits
-------

35bf420 [DI] Fix bad error message for unused bind under _defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@javiereguiluz @weaverryan @spackmat @nicolas-grekas @Loupax @carsonbot @Arkemlar @przemyslaw-bogusz
0