diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feature #29935 [DI] Fix bad error message for unused bind under _defaults (przemyslaw-bogusz)

Browse Source 
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
-------

35bf4203e8 [DI] Fix bad error message for unused bind under _defaults
This commit is contained in:
Nicolas Grekas 2019-04-07 12:29:22 +02:00
commit b0989aaffc
11 changed files with 104 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
src/Symfony/Component/DependencyInjection/Argument/BoundArgument.php
View File

@ -16,13 +16,19 @@ namespace Symfony\Component\DependencyInjection\Argument;
*/
final class BoundArgument implements ArgumentInterface
{
const SERVICE_BINDING = 0;
const DEFAULTS_BINDING = 1;
const INSTANCEOF_BINDING = 2;
private static $sequence = 0;
private $value;
private $identifier;
private $used;
private $type;
private $file;
public function __construct($value, bool $trackUsage = true)
public function __construct($value, bool $trackUsage = true, int $type = 0, string $file = null)
{
$this->value = $value;
if ($trackUsage) {
@ -30,6 +36,8 @@ final class BoundArgument implements ArgumentInterface
} else {
$this->used = true;
}
$this->type = $type;
$this->file = $file;
}
/**
@ -37,7 +45,7 @@ final class BoundArgument implements ArgumentInterface
*/
public function getValues()
{
return [$this->value, $this->identifier, $this->used];
return [$this->value, $this->identifier, $this->used, $this->type, $this->file];
}
/**
@ -45,6 +53,6 @@ final class BoundArgument implements ArgumentInterface
*/
public function setValues(array $values)
{
list($this->value, $this->identifier, $this->used) = $values;
list($this->value, $this->identifier, $this->used, $this->type, $this->file) = $values;
}
}

39
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
View File

@ -37,8 +37,39 @@ class ResolveBindingsPass extends AbstractRecursivePass
try {
parent::process($container);
foreach ($this->unusedBindings as list($key, $serviceId)) {
$message = sprintf('Unused binding "%s" in service "%s".', $key, $serviceId);
foreach ($this->unusedBindings as list($key, $serviceId, $bindingType, $file)) {
$argumentType = $argumentName = $message = null;
if (false !== strpos($key, ' ')) {
list($argumentType, $argumentName) = explode(' ', $key, 2);
} elseif ('$' === $key[0]) {
$argumentName = $key;
} else {
$argumentType = $key;
}
if ($argumentType) {
$message .= sprintf('of type "%s" ', $argumentType);
}
if ($argumentName) {
$message .= sprintf('named "%s" ', $argumentName);
}
if (BoundArgument::DEFAULTS_BINDING === $bindingType) {
$message .= 'under "_defaults"';
} elseif (BoundArgument::INSTANCEOF_BINDING === $bindingType) {
$message .= 'under "_instanceof"';
} else {
$message .= sprintf('for service "%s"', $serviceId);
}
if ($file) {
$message .= sprintf(' in file "%s"', $file);
}
$message = sprintf('A binding is configured for an argument %s, but no corresponding argument has been found. It may be unused and should be removed, or it may have a typo.', $message);
if ($this->errorMessages) {
$message .= sprintf("\nCould be related to%s:", 1 < \count($this->errorMessages) ? ' one of' : '');
}
@ -75,12 +106,12 @@ class ResolveBindingsPass extends AbstractRecursivePass
}
foreach ($bindings as $key => $binding) {
list($bindingValue, $bindingId, $used) = $binding->getValues();
list($bindingValue, $bindingId, $used, $bindingType, $file) = $binding->getValues();
if ($used) {
$this->usedBindings[$bindingId] = true;
unset($this->unusedBindings[$bindingId]);
} elseif (!isset($this->usedBindings[$bindingId])) {
$this->unusedBindings[$bindingId] = [$key, $this->currentId];
$this->unusedBindings[$bindingId] = [$key, $this->currentId, $bindingType, $file];
}
if (preg_match('/^(?:(?:array|bool|float|int|string) )?\$/', $key)) {

10
src/Symfony/Component/DependencyInjection/Loader/Configurator/DefaultsConfigurator.php
View File

@ -11,6 +11,7 @@
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
/**
@ -25,6 +26,15 @@ class DefaultsConfigurator extends AbstractServiceConfigurator
use Traits\BindTrait;
use Traits\PublicTrait;
private $path;
public function __construct(ServicesConfigurator $parent, Definition $definition, string $path = null)
{
parent::__construct($parent, $definition, null, []);
$this->path = $path;
}
/**
* Adds a tag for this definition.
*

11
src/Symfony/Component/DependencyInjection/Loader/Configurator/InstanceofConfigurator.php
View File

@ -11,6 +11,8 @@
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
use Symfony\Component\DependencyInjection\Definition;
/**
* @author Nicolas Grekas <p@tchwork.com>
*/
@ -28,6 +30,15 @@ class InstanceofConfigurator extends AbstractServiceConfigurator
use Traits\TagTrait;
use Traits\BindTrait;
private $path;
public function __construct(ServicesConfigurator $parent, Definition $definition, string $id, string $path = null)
{
parent::__construct($parent, $definition, $id, []);
$this->path = $path;
}
/**
* Defines an instanceof-conditional to be applied to following service definitions.
*/

4
src/Symfony/Component/DependencyInjection/Loader/Configurator/ServiceConfigurator.php
View File

@ -45,12 +45,14 @@ class ServiceConfigurator extends AbstractServiceConfigurator
private $container;
private $instanceof;
private $allowParent;
private $path;
public function __construct(ContainerBuilder $container, array $instanceof, bool $allowParent, ServicesConfigurator $parent, Definition $definition, $id, array $defaultTags)
public function __construct(ContainerBuilder $container, array $instanceof, bool $allowParent, ServicesConfigurator $parent, Definition $definition, $id, array $defaultTags, string $path = null)
{
$this->container = $container;
$this->instanceof = $instanceof;
$this->allowParent = $allowParent;
$this->path = $path;
parent::__construct($parent, $definition, $id, $defaultTags);
}

8
src/Symfony/Component/DependencyInjection/Loader/Configurator/ServicesConfigurator.php
View File

@ -29,6 +29,7 @@ class ServicesConfigurator extends AbstractConfigurator
private $container;
private $loader;
private $instanceof;
private $path;
private $anonymousHash;
private $anonymousCount;
@ -38,6 +39,7 @@ class ServicesConfigurator extends AbstractConfigurator
$this->container = $container;
$this->loader = $loader;
$this->instanceof = &$instanceof;
$this->path = $path;
$this->anonymousHash = ContainerBuilder::hash($path ?: mt_rand());
$this->anonymousCount = &$anonymousCount;
$instanceof = [];
@ -48,7 +50,7 @@ class ServicesConfigurator extends AbstractConfigurator
*/
final public function defaults(): DefaultsConfigurator
{
return new DefaultsConfigurator($this, $this->defaults = new Definition());
return new DefaultsConfigurator($this, $this->defaults = new Definition(), $this->path);
}
/**
@ -58,7 +60,7 @@ class ServicesConfigurator extends AbstractConfigurator
{
$this->instanceof[$fqcn] = $definition = new ChildDefinition('');
return new InstanceofConfigurator($this, $definition, $fqcn);
return new InstanceofConfigurator($this, $definition, $fqcn, $this->path);
}
/**
@ -90,7 +92,7 @@ class ServicesConfigurator extends AbstractConfigurator
$definition->setBindings($defaults->getBindings());
$definition->setChanges([]);
$configurator = new ServiceConfigurator($this->container, $this->instanceof, $allowParent, $this, $definition, $id, $defaults->getTags());
$configurator = new ServiceConfigurator($this->container, $this->instanceof, $allowParent, $this, $definition, $id, $defaults->getTags(), $this->path);
return null !== $class ? $configurator->class($class) : $configurator;
}

6
src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/BindTrait.php
View File

@ -11,7 +11,10 @@
namespace Symfony\Component\DependencyInjection\Loader\Configurator\Traits;
use Symfony\Component\DependencyInjection\Argument\BoundArgument;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Loader\Configurator\DefaultsConfigurator;
use Symfony\Component\DependencyInjection\Loader\Configurator\InstanceofConfigurator;
use Symfony\Component\DependencyInjection\Reference;
trait BindTrait
@ -35,7 +38,8 @@ trait BindTrait
throw new InvalidArgumentException(sprintf('Invalid binding for service "%s": named arguments must start with a "$", and FQCN must map to references. Neither applies to binding "%s".', $this->id, $nameOrFqcn));
}
$bindings = $this->definition->getBindings();
$bindings[$nameOrFqcn] = $valueOrRef;
$type = $this instanceof DefaultsConfigurator ? BoundArgument::DEFAULTS_BINDING : ($this instanceof InstanceofConfigurator ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING);
$bindings[$nameOrFqcn] = new BoundArgument($valueOrRef, true, $type, $this->path ?? null);
$this->definition->setBindings($bindings);
return $this;

13
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
View File

@ -175,9 +175,15 @@ class XmlFileLoader extends FileLoader
if (null === $defaultsNode = $xpath->query('//container:services/container:defaults')->item(0)) {
return [];
}
$bindings = [];
foreach ($this->getArgumentsAsPhp($defaultsNode, 'bind', $file) as $argument => $value) {
$bindings[$argument] = new BoundArgument($value, true, BoundArgument::DEFAULTS_BINDING, $file);
}
$defaults = [
'tags' => $this->getChildren($defaultsNode, 'tag'),
'bind' => array_map(function ($v) { return new BoundArgument($v); }, $this->getArgumentsAsPhp($defaultsNode, 'bind', $file)),
'bind' => $bindings,
];
foreach ($defaults['tags'] as $tag) {
@ -368,6 +374,11 @@ class XmlFileLoader extends FileLoader
}
$bindings = $this->getArgumentsAsPhp($service, 'bind', $file);
$bindingType = $this->isLoadingInstanceof ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING;
foreach ($bindings as $argument => $value) {
$bindings[$argument] = new BoundArgument($value, true, $bindingType, $file);
}
if (isset($defaults['bind'])) {
// deep clone, to avoid multiple process of the same instance in the passes
$bindings = array_merge(unserialize(serialize($defaults['bind'])), $bindings);

10
src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
View File

@ -284,7 +284,9 @@ class YamlFileLoader extends FileLoader
throw new InvalidArgumentException(sprintf('Parameter "bind" in "_defaults" must be an array in %s. Check your YAML syntax.', $file));
}
$defaults['bind'] = array_map(function ($v) { return new BoundArgument($v); }, $this->resolveServices($defaults['bind'], $file));
foreach ($this->resolveServices($defaults['bind'], $file) as $argument => $value) {
$defaults['bind'][$argument] = new BoundArgument($value, true, BoundArgument::DEFAULTS_BINDING, $file);
}
}
return $defaults;
@ -534,6 +536,12 @@ class YamlFileLoader extends FileLoader
}
$bindings = array_merge($bindings, $this->resolveServices($service['bind'], $file));
$bindingType = $this->isLoadingInstanceof ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING;
foreach ($bindings as $argument => $value) {
if (!$value instanceof BoundArgument) {
$bindings[$argument] = new BoundArgument($value, true, $bindingType, $file);
}
}
}
$definition->setBindings($bindings);

2
src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveBindingsPassTest.php
View File

@ -50,7 +50,7 @@ class ResolveBindingsPassTest extends TestCase
/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedExceptionMessage Unused binding "$quz" in service "Symfony\Component\DependencyInjection\Tests\Fixtures\NamedArgumentsDummy".
* @expectedExceptionMessage A binding is configured for an argument named "$quz" for service "Symfony\Component\DependencyInjection\Tests\Fixtures\NamedArgumentsDummy", but no corresponding argument has been found. It may be unused and should be removed, or it may have a typo.
*/
public function testUnusedBinding()
{

4
src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php
View File

@ -139,8 +139,8 @@ class RegisterControllerArgumentLocatorsPass implements CompilerPassInterface
} elseif (isset($bindings[$bindingName = $type.' $'.$p->name]) || isset($bindings[$bindingName = '$'.$p->name]) || isset($bindings[$bindingName = $type])) {
$binding = $bindings[$bindingName];
list($bindingValue, $bindingId) = $binding->getValues();
$binding->setValues([$bindingValue, $bindingId, true]);
list($bindingValue, $bindingId, , $bindingType, $bindingFile) = $binding->getValues();
$binding->setValues([$bindingValue, $bindingId, true, $bindingType, $bindingFile]);
if (!$bindingValue instanceof Reference) {
$args[$p->name] = new Reference('.value.'.$container->hash($bindingValue));