10000 Fixes issues #27828 and #28326 by przemyslaw-bogusz · Pull Request #29897 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Fixes issues #27828 and #28326 #29897

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,37 @@
*/
final class BoundArgument implements ArgumentInterface
{
const SERVICE_BIND = 0;
const DEFAULT_BIND = 1;
const INSTANCE_BIND = 2;

const MESSAGES = [
1 => 'under "_defaults"',
2 => 'under "_instanceof"',
];

private static $sequence = 0;

private $value;
private $identifier;
private $used;
private $type;
private $file;

public function __construct($value)
public function __construct($value, $type = 0, $file = null)
{
$this->value = $value;
$this->identifier = ++self::$sequence;
$this->type = (int) $type;
$this->file = (string) $file;
}

/**
* {@inheritdoc}
*/
public function getValues()
{
return [$this->value, $this->identifier, $this->used];
return [$this->value, $this->identifier, $this->used, $this->type, $this->file];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,34 @@ class ResolveBindingsPass extends AbstractRecursivePass
*/
public function process(ContainerBuilder $container)
{
foreach ($container->getBindings() as $definition) {
foreach ($definition as $argument => $values) {
if (1 < \count($values)) {
foreach (\array_slice(array_keys($values), 0, -1) as $value) {
$this->usedBindings[$value] = true;
}
}
}
}

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, $type, $file)) {
$message = sprintf('You have a "bind" configured for an argument named "%s" ', $key);

if ($type) {
$message .= BoundArgument::MESSAGES[$type];
} else {
$message .= sprintf('in service "%s"', $serviceId);
}

if ($file) {
$message .= sprintf(' in file "%s"', $file);
}

$message .= '. But, this argument was not found in any of the services it was applied to. It may be unused and can be removed, or it may have a typo.';

if ($this->errorMessages) {
$message .= sprintf("\nCould be related to%s:", 1 < \count($this->errorMessages) ? ' one of' : '');
}
Expand Down Expand Up @@ -75,12 +98,12 @@ protected function processValue($value, $isRoot = false)
}

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 (isset($key[0]) && '$' === $key[0]) {
Expand Down
18 changes: 18 additions & 0 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface

private $removedIds = [];

/**
* @var bool[][][] a map of values bound to arguments in definitions
*/
private $bindings = [];

private static $internalTypes = [
'int' => true,
'float' => true,
Expand Down Expand Up @@ -1021,6 +1026,14 @@ public function setDefinition($id, Definition $definition)

unset($this->aliasDefinitions[$id], $this->removedIds[$id]);

$bindings = $definition->getBindings();
if (!empty($bindings)) {
foreach ($bindings as $argument => $binding) {
list(, $identifier) = $binding->getValues();
$this->bindings[$id][$argument][$identifier] = true;
Copy link
Contributor
@GuilhemN GuilhemN Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this marking all bindings as used? If I'm correct, no exception will ever be thrown.

Edit: ok, I misunderstood your code :)
But the issue we have is about overriden services (when a service is overriden the set of services bindings are tested on is smaller than expected, hence an exception might be thrown that wouldn't have been if the former service hadn't been replaced, it only happens when using defaults/shared bindings).

About what you solve here, I don't think we should support manual overriding of bindings since I almost consider the management of shared bindings in BoundArgument as internal.

}
}

return $this->definitions[$id] = $definition;
}

Expand Down Expand Up @@ -1656,4 +1669,9 @@ private function inVendors($path)

return false;
}

public function getBindings(): array
{
return $this->bindings;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ private function parseDefaults(array &$content, $file)
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, BoundArgument::DEFAULT_BIND, $file);
}
}

return $defaults;
Expand Down Expand Up @@ -558,6 +560,12 @@ private function parseDefinition($id, $service, $file, array $defaults)
}

$bindings = array_merge($bindings, $this->resolveServices($service['bind'], $file));

$bindingType = $this->isLoadingInstanceof ? BoundArgument::INSTANCE_BIND : BoundArgument::SERVICE_BIND;

foreach ($bindings as $argument => $value) {
$bindings[$argument] = new BoundArgument($value, $bindingType, $file);
}
}

$definition->setBindings($bindings);
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/Foo.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

namespace Symfony\Component\DependencyInjection\Tests\Fixtures;

class Foo
{
public function __construct($abc)
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
services:
_defaults:
bind:
$quz: quzFirstValue
$abc: abcFirstValue

bar:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\Bar

factory:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryDummy

foo:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\Foo
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
services:
_defaults:
bind:
$abc: abcSecondValue

foo:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\Foo

factory:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryDummy
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
services:
_defaults:
bind:
$abc: abcThirdValue

foo:
class: Symfony\Component\DependencyInjection\Tests\Fixtures\Foo
bind:
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\Config\Resource\FileResource;
use Symfony\Component\Config\Resource\GlobResource;
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
use Symfony\Component\DependencyInjection\Compiler\ResolveBindingsPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\IniFileLoader;
use Symfony\Component\DependencyInjection\Loader\PhpFileLoader;
Expand Down Expand Up @@ -732,4 +733,22 @@ public function testBindings()
'$factory' => 'factory',
], array_map(function ($v) { return $v->getValues()[0]; }, $definition->getBindings()));
}

/**
* The pass may throw an exception, which will cause the test to fail.
*/
public function testOverriddenDefaultsBindings()
{
$container = new ContainerBuilder();

$loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
$loader->load('defaults_bindings.yml');
$loader->load('defaults_bindings2.yml');
$loader->load('defaults_bindings3.yml');

$pass = new ResolveBindingsPass();
$pass->process($container);

$this->assertTrue(true);
}
}
0