Skip to content

[DI] Don't use auto-registered services to populate type-candidates #22254

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

Merged
merged 1 commit into from
Apr 3, 2017
Merged

[DI] Don't use auto-registered services to populate type-candidates #22254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 32 additions & 24 deletions src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class AutowirePass implements CompilerPassInterface
private $definedTypes = array();
private $types;
private $notGuessableTypes = array();
private $usedTypes = array();
private $autowired = array();

/**
* {@inheritdoc}
Expand All @@ -45,15 +45,6 @@ public function process(ContainerBuilder $container)
$this->completeDefinition($id, $definition);
}
}

foreach ($this->usedTypes as $type => $id) {
if (isset($this->usedTypes[$type]) && isset($this->notGuessableTypes[$type])) {
$classOrInterface = class_exists($type) ? 'class' : 'interface';
$matchingServices = implode(', ', $this->types[$type]);

throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $type, $id, $classOrInterface, $matchingServices));
}
}
} catch (\Exception $e) {
} catch (\Throwable $e) {
}
Expand All @@ -66,7 +57,7 @@ public function process(ContainerBuilder $container)
$this->definedTypes = array();
$this->types = null;
$this->notGuessableTypes = array();
$this->usedTypes = array();
$this->autowired = array();

if (isset($e)) {
throw $e;
Expand All @@ -92,47 +83,56 @@ private function completeDefinition($id, Definition $definition)
if (!$constructor = $reflectionClass->getConstructor()) {
return;
}
$parameters = $constructor->getParameters();
if (method_exists('ReflectionMethod', 'isVariadic') && $constructor->isVariadic()) {
array_pop($parameters);
}

$arguments = $definition->getArguments();
foreach ($constructor->getParameters() as $index => $parameter) {
foreach ($parameters as $index => $parameter) {
if (array_key_exists($index, $arguments) && '' !== $arguments[$index]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we actually need this, it makes one more thing to learn while we could achieve the same using manual indexes or named arguments.

Copy link
Member

Choose a reason for hiding this comment

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

named arguments are a 3.3 feature. This PR targets 2.8. So we need to keep this feature in 2.8

Copy link
Member Author

Choose a reason for hiding this comment

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

this is committed behavior, we can't change now

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, that's a comment for 3.3, I'll open a different PR to discuss this.

Copy link
Member

Choose a reason for hiding this comment

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

This behavior is still useful in XML (but not in YAML IMO).

continue;
}

try {
if (!$typeHint = $parameter->getClass()) {
if (isset($arguments[$index])) {
continue;
}

// no default value? Then fail
if (!$parameter->isOptional()) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this check isDefaultValueAvailable and allowsNull instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check for isDefaultValueAvailable - but none for allowsNull because unless I'm wrong, we don't need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

and I just reverted this change, because there is nothing wrong here: at this stage, an optional arg must have a default value, (but an arg with a default value is not necessarily optional)
so all good to me as is

Copy link
Member Author

@nicolas-grekas nicolas-grekas Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the current behavior is a tested one, so I'll adjust on master to fit named args, nothing to do here to me

throw new RuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.', $index, $parameter->name, $id));
}

if (!array_key_exists($index, $arguments)) {
// specifically pass the default value
$arguments[$index] = $parameter->getDefaultValue();
}
// specifically pass the default value
$arguments[$index] = $parameter->getDefaultValue();

continue;
}

if (isset($this->autowired[$typeHint->name])) {
return $this->autowired[$typeHint->name] ? new Reference($this->autowired[$typeHint->name]) : null;
}

if (null === $this->types) {
$this->populateAvailableTypes();
}

if (isset($this->types[$typeHint->name]) && !isset($this->notGuessableTypes[$typeHint->name])) {
$value = new Reference($this->types[$typeHint->name]);
$this->usedTypes[$typeHint->name] = $id;
} else {
try {
$value = $this->createAutowiredDefinition($typeHint, $id);
$this->usedTypes[$typeHint->name] = $id;
} catch (RuntimeException $e) {
if ($parameter->allowsNull()) {
$value = null;
} elseif ($parameter->isDefaultValueAvailable()) {
if ($parameter->isDefaultValueAvailable()) {
$value = $parameter->getDefaultValue();
} elseif ($parameter->allowsNull()) {
$value = null;
} else {
throw $e;
}
$this->autowired[$typeHint->name] = false;
}
}
} catch (\ReflectionException $e) {
Expand All @@ -148,6 +148,16 @@ private function completeDefinition($id, Definition $definition)
$arguments[$index] = $value;
}

if ($parameters && !isset($arguments[++$index])) {
while (0 <= --$index) {
$parameter = $parameters[$index];
if (!$parameter->isDefaultValueAvailable() || $parameter->getDefaultValue() !== $arguments[$index]) {
break;
}
unset($arguments[$index]);
}
}

// it's possible index 1 was set, then index 0, then 2, etc
// make sure that we re-order so they're injected as expected
ksort($arguments);
Expand Down Expand Up @@ -252,13 +262,11 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint, $id)
throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeHint->name, $id, $classOrInterface));
}

$argumentId = sprintf('autowired.%s', $typeHint->name);
$this->autowired[$typeHint->name] = $argumentId = sprintf('autowired.%s', $typeHint->name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we populate just one type instead? e.g. $this->types[$typeHint->name] = $argumentId

Copy link
Member Author

@nicolas-grekas nicolas-grekas Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I looked into this, and didn't find much benefit in doing so, counting LOC - also conceptually that's no exactly the same (as hinted by https://github.com/symfony/symfony/pull/22254/files#diff-62df969ae028c559d33ffd256de1ac49R135)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as there is the false support I don't see much benefit too.
Fine for me as is then 👍


$argumentDefinition = $this->container->register($argumentId, $typeHint->name);
$argumentDefinition->setPublic(false);

$this->populateAvailableType($argumentId, $argumentDefinition);

try {
$this->completeDefinition($argumentId, $argumentDefinition);
} catch (RuntimeException $e) {
Expand Down
21 changes: 0 additions & 21 deletions src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,6 @@ public function testOptionalScalarArgsNotPassedIfLast()
array(
new Reference('a'),
new Reference('lille'),
// third arg shouldn't *need* to be passed
// but that's hard to "pull of" with autowiring, so
// this assumes passing the default val is ok
'some_val',
),
$definition->getArguments()
);
Expand Down Expand Up @@ -462,23 +458,6 @@ public function testEmptyStringIsKept()
$this->assertEquals(array(new Reference('a'), '', new Reference('lille')), $container->getDefinition('foo')->getArguments());
}

/**
* @dataProvider provideAutodiscoveredAutowiringOrder
*
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMEssage Unable to autowire argument of type "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" for the service "a". Multiple services exist for this interface (autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionA, autowired.Symfony\Component\DependencyInjection\Tests\Compiler\CollisionB).
*/
public function testAutodiscoveredAutowiringOrder($class)
{
$container = new ContainerBuilder();

$container->register('a', __NAMESPACE__.'\\'.$class)
->setAutowired(true);

$pass = new AutowirePass();
$pass->process($container);
}

public function provideAutodiscoveredAutowiringOrder()
{
return array(
Expand Down