Skip to content

Commit 44bfa20

Browse files
przemyslaw-bogusznicolas-grekas
przemyslaw-bogusz
authored and
nicolas-grekas
committed
[DI] Fix bad error message for unused bind under _defaults
1 parent a0d286d commit 44bfa20

File tree

10 files changed

+102
-15
lines changed

10 files changed

+102
-15
lines changed

‎Argument/BoundArgument.php

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,35 +16,43 @@
1616
*/
1717
final class BoundArgument implements ArgumentInterface
1818
{
19+
const SERVICE_BINDING = 0;
20+
const DEFAULTS_BINDING = 1;
21+
const INSTANCEOF_BINDING = 2;
22+
1923
private static $sequence = 0;
2024

2125
private $value;
2226
private $identifier;
2327
private $used;
28+
private $type;
29+
private $file;
2430

25-
public function __construct($value, bool $trackUsage = true)
31+
public function __construct($value, bool $trackUsage = true, int $type = 0, string $file = null)
2632
{
2733
$this->value = $value;
2834
if ($trackUsage) {
2935
$this->identifier = ++self::$sequence;
3036
} else {
3137
$this->used = true;
3238
}
39+
$this->type = $type;
40+
$this->file = $file;
3341
}
3442

3543
/**
3644
* {@inheritdoc}
3745
*/
3846
public function getValues()
3947
{
40-
return [$this->value, $this->identifier, $this->used];
48+
return [$this->value, $this->identifier, $this->used, $this->type, $this->file];
4149
}
4250

4351
/**
4452
* {@inheritdoc}
4553
*/
4654
public function setValues(array $values)
4755
{
48-
list($this->value, $this->identifier, $this->used) = $values;
56+
list($this->value, $this->identifier, $this->used, $this->type, $this->file) = $values;
4957
}
5058
}

‎Compiler/ResolveBindingsPass.php

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,39 @@ public function process(ContainerBuilder $container)
3737
try {
3838
parent::process($container);
3939

40-
foreach ($this->unusedBindings as list($key, $serviceId)) {
41-
$message = sprintf('Unused binding "%s" in service "%s".', $key, $serviceId);
40+
foreach ($this->unusedBindings as list($key, $serviceId, $bindingType, $file)) {
41+
$argumentType = $argumentName = $message = null;
42+
43+
if (false !== strpos($key, ' ')) {
44+
list($argumentType, $argumentName) = explode(' ', $key, 2);
45+
} elseif ('$' === $key[0]) {
46+
$argumentName = $key;
47+
} else {
48+
$argumentType = $key;
49+
}
50+
51+
if ($argumentType) {
52+
$message .= sprintf('of type "%s" ', $argumentType);
53+
}
54+
55+
if ($argumentName) {
56+
$message .= sprintf('named "%s" ', $argumentName);
57+
}
58+
59+
if (BoundArgument::DEFAULTS_BINDING === $bindingType) {
60+
$message .= 'under "_defaults"';
61+
} elseif (BoundArgument::INSTANCEOF_BINDING === $bindingType) {
62+
$message .= 'under "_instanceof"';
63+
} else {
64+
$message .= sprintf('for service "%s"', $serviceId);
65+
}
66+
67+
if ($file) {
68+
$message .= sprintf(' in file "%s"', $file);
69+
}
70+
71+
$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);
72+
4273
if ($this->errorMessages) {
4374
$message .= sprintf("\nCould be related to%s:", 1 < \count($this->errorMessages) ? ' one of' : '');
4475
}
@@ -75,12 +106,12 @@ protected function processValue($value, $isRoot = false)
75106
}
76107

77108
foreach ($bindings as $key => $binding) {
78-
list($bindingValue, $bindingId, $used) = $binding->getValues();
109+
list($bindingValue, $bindingId, $used, $bindingType, $file) = $binding->getValues();
79110
if ($used) {
80111
$this->usedBindings[$bindingId] = true;
81112
unset($this->unusedBindings[$bindingId]);
82113
} elseif (!isset($this->usedBindings[$bindingId])) {
83-
$this->unusedBindings[$bindingId] = [$key, $this->currentId];
114+
$this->unusedBindings[$bindingId] = [$key, $this->currentId, $bindingType, $file];
84115
}
85116

86117
if (preg_match('/^(?:(?:array|bool|float|int|string) )?\$/', $key)) {

‎Loader/Configurator/DefaultsConfigurator.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
1313

14+
use Symfony\Component\DependencyInjection\Definition;
1415
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
1516

1617
/**
@@ -25,6 +26,15 @@ class DefaultsConfigurator extends AbstractServiceConfigurator
2526
use Traits\BindTrait;
2627
use Traits\PublicTrait;
2728

29+
private $path;
30+
31+
public function __construct(ServicesConfigurator $parent, Definition $definition, string $path = null)
32+
{
33+
parent::__construct($parent, $definition, null, []);
34+
35+
$this->path = $path;
36+
}
37+
2838
/**
2939
* Adds a tag for this definition.
3040
*

‎Loader/Configurator/InstanceofConfigurator.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Loader\Configurator;
1313

14+
use Symfony\Component\DependencyInjection\Definition;
15+
1416
/**
1517
* @author Nicolas Grekas <[email protected]>
1618
*/
@@ -28,6 +30,15 @@ class InstanceofConfigurator extends AbstractServiceConfigurator
2830
use Traits\TagTrait;
2931
use Traits\BindTrait;
3032

33+
private $path;
34+
35+
public function __construct(ServicesConfigurator $parent, Definition $definition, string $id, string $path = null)
36+
{
37+
parent::__construct($parent, $definition, $id, []);
38+
39+
$this->path = $path;
40+
}
41+
3142
/**
3243
* Defines an instanceof-conditional to be applied to following service definitions.
3344
*/

‎Loader/Configurator/ServiceConfigurator.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,14 @@ class ServiceConfigurator extends AbstractServiceConfigurator
4545
private $container;
4646
private $instanceof;
4747
private $allowParent;
48+
private $path;
4849

49-
public function __construct(ContainerBuilder $container, array $instanceof, bool $allowParent, ServicesConfigurator $parent, Definition $definition, $id, array $defaultTags)
50+
public function __construct(ContainerBuilder $container, array $instanceof, bool $allowParent, ServicesConfigurator $parent, Definition $definition, $id, array $defaultTags, string $path = null)
5051
{
5152
$this->container = $container;
5253
$this->instanceof = $instanceof;
5354
$this->allowParent = $allowParent;
55+
$this->path = $path;
5456

5557
parent::__construct($parent, $definition, $id, $defaultTags);
5658
}

‎Loader/Configurator/ServicesConfigurator.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ class ServicesConfigurator extends AbstractConfigurator
2929
private $container;
3030
private $loader;
3131
private $instanceof;
32+
private $path;
3233
private $anonymousHash;
3334
private $anonymousCount;
3435

@@ -38,6 +39,7 @@ public function __construct(ContainerBuilder $container, PhpFileLoader $loader,
3839
$this->container = $container;
3940
$this->loader = $loader;
4041
$this->instanceof = &$instanceof;
42+
$this->path = $path;
4143
$this->anonymousHash = ContainerBuilder::hash($path ?: mt_rand());
4244
$this->anonymousCount = &$anonymousCount;
4345
$instanceof = [];
@@ -48,7 +50,7 @@ public function __construct(ContainerBuilder $container, PhpFileLoader $loader,
4850
*/
4951
final public function defaults(): DefaultsConfigurator
5052
{
51-
return new DefaultsConfigurator($this, $this->defaults = new Definition());
53+
return new DefaultsConfigurator($this, $this->defaults = new Definition(), $this->path);
5254
}
5355

5456
/**
@@ -58,7 +60,7 @@ final public function instanceof(string $fqcn): InstanceofConfigurator
5860
{
5961
$this->instanceof[$fqcn] = $definition = new ChildDefinition('');
6062

61-
return new InstanceofConfigurator($this, $definition, $fqcn);
63+
return new InstanceofConfigurator($this, $definition, $fqcn, $this->path);
6264
}
6365

6466
/**
@@ -90,7 +92,7 @@ final public function set(?string $id, string $class = null): ServiceConfigurato
9092
$definition->setBindings($defaults->getBindings());
9193
$definition->setChanges([]);
9294

93-
$configurator = new ServiceConfigurator($this->container, $this->instanceof, $allowParent, $this, $definition, $id, $defaults->getTags());
95+
$configurator = new ServiceConfigurator($this->container, $this->instanceof, $allowParent, $this, $definition, $id, $defaults->getTags(), $this->path);
9496

9597
return null !== $class ? $configurator->class($class) : $configurator;
9698
}

‎Loader/Configurator/Traits/BindTrait.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@
1111

1212
namespace Symfony\Component\DependencyInjection\Loader\Configurator\Traits;
1313

14+
use Symfony\Component\DependencyInjection\Argument\BoundArgument;
1415
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
16+
use Symfony\Component\DependencyInjection\Loader\Configurator\DefaultsConfigurator;
17+
use Symfony\Component\DependencyInjection\Loader\Configurator\InstanceofConfigurator;
1518
use Symfony\Component\DependencyInjection\Reference;
1619

1720
trait BindTrait
@@ -35,7 +38,8 @@ final public function bind($nameOrFqcn, $valueOrRef)
3538
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));
3639
}
3740
$bindings = $this->definition->getBindings();
38-
$bindings[$nameOrFqcn] = $valueOrRef;
41+
$type = $this instanceof DefaultsConfigurator ? BoundArgument::DEFAULTS_BINDING : ($this instanceof InstanceofConfigurator ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING);
42+
$bindings[$nameOrFqcn] = new BoundArgument($valueOrRef, true, $type, $this->path ?? null);
3943
$this->definition->setBindings($bindings);
4044

4145
return $this;

‎Loader/XmlFileLoader.php

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,15 @@ private function getServiceDefaults(\DOMDocument $xml, $file)
175175
if (null === $defaultsNode = $xpath->query('//container:services/container:defaults')->item(0)) {
176176
return [];
177177
}
178+
179+
$bindings = [];
180+
foreach ($this->getArgumentsAsPhp($defaultsNode, 'bind', $file) as $argument => $value) {
181+
$bindings[$argument] = new BoundArgument($value, true, BoundArgument::DEFAULTS_BINDING, $file);
182+
}
183+
178184
$defaults = [
179185
'tags' => $this->getChildren($defaultsNode, 'tag'),
180-
'bind' => array_map(function ($v) { return new BoundArgument($v); }, $this->getArgumentsAsPhp($defaultsNode, 'bind', $file)),
186+
'bind' => $bindings,
181187
];
182188

183189
foreach ($defaults['tags'] as $tag) {
@@ -368,6 +374,11 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults)
368374
}
369375

370376
$bindings = $this->getArgumentsAsPhp($service, 'bind', $file);
377+
$bindingType = $this->isLoadingInstanceof ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING;
378+
foreach ($bindings as $argument => $value) {
379+
$bindings[$argument] = new BoundArgument($value, true, $bindingType, $file);
380+
}
381+
371382
if (isset($defaults['bind'])) {
372383
// deep clone, to avoid multiple process of the same instance in the passes
373384
$bindings = array_merge(unserialize(serialize($defaults['bind'])), $bindings);

‎Loader/YamlFileLoader.php

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,9 @@ private function parseDefaults(array &$content, string $file): array
284284
throw new InvalidArgumentException(sprintf('Parameter "bind" in "_defaults" must be an array in %s. Check your YAML syntax.', $file));
285285
}
286286

287-
$defaults['bind'] = array_map(function ($v) { return new BoundArgument($v); }, $this->resolveServices($defaults['bind'], $file));
287+
foreach ($this->resolveServices($defaults['bind'], $file) as $argument => $value) {
288+
$defaults['bind'][$argument] = new BoundArgument($value, true, BoundArgument::DEFAULTS_BINDING, $file);
289+
}
288290
}
289291

290292
return $defaults;
@@ -534,6 +536,12 @@ private function parseDefinition($id, $service, $file, array $defaults)
534536
}
535537

536538
$bindings = array_merge($bindings, $this->resolveServices($service['bind'], $file));
539+
$bindingType = $this->isLoadingInstanceof ? BoundArgument::INSTANCEOF_BINDING : BoundArgument::SERVICE_BINDING;
540+
foreach ($bindings as $argument => $value) {
541+
if (!$value instanceof BoundArgument) {
542+
$bindings[$argument] = new BoundArgument($value, true, $bindingType, $file);
543+
}
544+
}
537545
}
538546

539547
$definition->setBindings($bindings);

‎Tests/Compiler/ResolveBindingsPassTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public function testProcess()
5050

5151
/**
5252
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
53-
* @expectedExceptionMessage Unused binding "$quz" in service "Symfony\Component\DependencyInjection\Tests\Fixtures\NamedArgumentsDummy".
53+
* @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.
5454
*/
5555
public function testUnusedBinding()
5656
{

0 commit comments

Comments
 (0)