8000 [DI][Bug] Autowiring thinks optional args on core classes are required by weaverryan · Pull Request #23605 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DI][Bug] Autowiring thinks optional args on core classes are required #23605

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
Jul 21, 2017
Merged

[DI][Bug] Autowiring thinks optional args on core classes are required #23605

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
Fixing a bug where if a core class was autowired, autowiring tried to… 
… autowire optional args as if they were required
  • Loading branch information
@weaverryan
weaverryan committed Jul 20, 2017
commit 178a0f73b7d3ee78c04e51fbf77b2f2bfec02f4b
6 changes: 6 additions & 0 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 @@ -273,6 +273,12 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a

// no default value? Then fail
if (!$parameter->isDefaultValueAvailable()) {
// For core classes, isDefaultValueAvailable() can
// be false when isOptional() returns true. If the
// argument *is* optional, allow it to be missing
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.

Why not change the !$parameter->isDefaultValueAvailable() condition instead?
Copy link
Member
@dunglas dunglas Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@fabpot it was the default behavior, but @nicolas-grekas changed it to make the pass more predictible: https://github.com/symfony/symfony/pull/22256/files#diff-62df969ae028c559d33ffd256de1ac49R253

Copy link
Member

Choose a reason for hiding this comment

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

But yes, this change basically reverts @nicolas-grekas's change.

Copy link
Member

Choose a reason for hiding this comment

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

nope, it's doesn't revert it, it makes it stronger - while reading, the previous was unclear ("why does this imply that" questions) - now it's doubly clearer :)

continue;
Copy link
Member
@nicolas-grekas nicolas-grekas Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this has to be a "break", because we cannot have holes in the parameters list

Copy link
Member Author

Choose a reason for hiding this comment

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

There could be holes with break or continue. The already-specified $arguments are passed into this function (imagine that thanks to named arguments, someone has specified the 1st and 4th arguments to something (i.e. $arguments has0 and 3 indexes). If we hit this code when trying to autowire the 2nd argument (index 1), then a break would still mean that $arguments ultimately has indexes 0 and 3 (the same with a continue: continue would loop again, but not fill in any args).

So I think changing to break makes sense... but we still have holes. This is caught later by CheckArgumentsValidityPass with:

Invalid constructor argument 4 for service "foo_bar": argument 2 must be defined before. Check your service definition.

tl;dr This DOES allow for holes in the parameters list in this pass... but that problem is caught by a later pass.

Copy link
Member
@nicolas-grekas nicolas-grekas Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

it does not create holes - of course, if the input has, it can only fill some of them :)

}
throw new AutowiringFailedException($this->currentId, sprintf('Cannot autowire service "%s": argument "$%s" of method "%s()" must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));
}

Expand Down
17 changes: 17 additions & 0 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 @@ -512,6 +512,23 @@ public function testOptionalScalarArgsNotPassedIfLast()
);
}

public function testOptionalArgsNoRequiredForCoreClasses()
{
$container = new ContainerBuilder();

$container->register('pdo_service', \PDO::class)
->addArgument('sqlite:/foo.db')
->setAutowired(true);

(new AutowirePass())->process($container);

$definition = $container->getDefinition('pdo_service');
$this->assertEquals(
array('sqlite:/foo.db'),
$definition->getArguments()
);
}

public function testSetterInjection()
{
$container = new ContainerBuilder();
Expand Down
0