Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[DI] Add "factory" support to named args and autowiring #22277

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 4, 2017
Merged

[DI] Add "factory" support to named args and autowiring #22277

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
90 changes: 89 additions & 1 deletion 90 src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Argument\ArgumentInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\Reference;

/**
* @author Nicolas Grekas <p@tchwork.com>
Expand Down Expand Up @@ -73,4 +75,90 @@ protected function processValue($value, $isRoot = false)

return $value;
}

/**
* @param Definition $definition
* @param bool $required
*
* @return \ReflectionFunctionAbstract|null
*
* @throws RuntimeException
*/
protected function getConstructor(Definition $definition, $required)
{
if (is_string($factory = $definition->getFactory())) {
if (!function_exists($factory)) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": function "%s" does not exist.', $this->currentId, $factory));
}
$r = new \ReflectionFunction($factory);
if (false !== $r->getFileName() && file_exists($r->getFileName())) {
$this->container->fileExists($r->getFileName());
}

return $r;
}

if ($factory) {
list($class, $method) = $factory;
if ($class instanceof Reference) {
$class = $this->container->findDefinition((string) $class)->getClass();
} elseif (null === $class) {
$class = $definition->getClass();
}
if ('__construct' === $method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really belong to this method? Shouldn't it be in CheckDefinitionValidityPass 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.

this is required to prevent a potential infinite loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, good point too :) It should still be added in CheckDefinitionValidityPass imo though.

throw new RuntimeException(sprintf('Unable to resolve service "%s": "__construct()" cannot be used as a factory method.', $this->currentId));
}

return $this->getReflectionMethod(new Definition($class), $method);
}

$class = $definition->getClass();

if (!$r = $this->container->getReflectionClass($class)) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": class "%s" does not exist.', $this->currentId, $class));
}
if (!$r = $r->getConstructor()) {
if ($required) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": class%s has no constructor.', $this->currentId, sprintf($class !== $this->currentId ? ' "%s"' : '', $class)));
}
} elseif (!$r->isPublic()) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": %s must be public.', $this->currentId, sprintf($class !== $this->currentId ? 'constructor of class "%s"' : 'its constructor', $class)));
}

return $r;
}

/**
* @param Definition $definition
* @param string $method
*
* @throws RuntimeException
*
* @return \ReflectionFunctionAbstract
*/
protected function getReflectionMethod(Definition $definition, $method)
{
if ('__construct' === $method) {
return $this->getConstructor($definition, true);
}

if (!$class = $definition->getClass()) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId));
}

if (!$r = $this->container->getReflectionClass($class)) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": class "%s" does not exist.', $this->currentId, $class));
}

if (!$r->hasMethod($method)) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": method "%s()" does not exist.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
}

$r = $r->getMethod($method);
if (!$r->isPublic()) {
throw new RuntimeException(sprintf('Unable to resolve service "%s": method "%s()" must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
}

return $r;
}
}
39 changes: 15 additions & 24 deletions 39 src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ protected function processValue($value, $isRoot = false)
if (!$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
return parent::processValue($value, $isRoot);
}
if ($value->getFactory()) {
throw new RuntimeException(sprintf('Service "%s" can use either autowiring or a factory, not both.', $this->currentId));
}
if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
$this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));

Expand All @@ -107,8 +104,8 @@ protected function processValue($value, $isRoot = false)
$autowiredMethods = $this->getMethodsToAutowire($reflectionClass);
$methodCalls = $value->getMethodCalls();

if ($constructor = $reflectionClass->getConstructor()) {
array_unshift($methodCalls, array($constructor->name, $value->getArguments()));
if ($constructor = $this->getConstructor($value, false)) {
array_unshift($methodCalls, array($constructor, $value->getArguments()));
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 keep ->name to remove the dedicated case, line 183 (https://github.com/symfony/symfony/pull/22277/files#diff-62df969ae028c559d33ffd256de1ac49R183)?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope we can't, because when a factory is used, a simple string is not enough to reference the target

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, good point 👍

}

$methodCalls = $this->autowireCalls($reflectionClass, $methodCalls, $autowiredMethods);
Expand Down Expand Up @@ -143,13 +140,13 @@ private function getMethodsToAutowire(\ReflectionClass $reflectionClass)
$found = array();
$methodsToAutowire = array();

if ($reflectionMethod = $reflectionClass->getConstructor()) {
$methodsToAutowire[strtolower($reflectionMethod->name)] = $reflectionMethod;
}

foreach ($reflectionClass->getMethods() as $reflectionMethod) {
$r = $reflectionMethod;

if ($r->isConstructor()) {
continue;
}

while (true) {
if (false !== $doc = $r->getDocComment()) {
if (false !== stripos($doc, '@required') && preg_match('#(?:^/\*\*|\n\s*+\*)\s*+@required(?:\s|\*/$)#i', $doc)) {
Expand Down Expand Up @@ -183,19 +180,13 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC
foreach ($methodCalls as $i => $call) {
list($method, $arguments) = $call;

if (isset($autowiredMethods[$lcMethod = strtolower($method)]) && $autowiredMethods[$lcMethod]->isPublic()) {
if ($method instanceof \ReflectionFunctionAbstract) {
$reflectionMethod = $method;
} elseif (isset($autowiredMethods[$lcMethod = strtolower($method)]) && $autowiredMethods[$lcMethod]->isPublic()) {
$reflectionMethod = $autowiredMethods[$lcMethod];
unset($autowiredMethods[$lcMethod]);
} else {
if (!$reflectionClass->hasMethod($method)) {
$class = $reflectionClass->name;
throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s() does not exist.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
}
$reflectionMethod = $reflectionClass->getMethod($method);
if (!$reflectionMethod->isPublic()) {
$class = $reflectionClass->name;
throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s() must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
}
$reflectionMethod = $this->getReflectionMethod($this->currentDefinition, $method);
}

$arguments = $this->autowireMethod($reflectionMethod, $arguments);
Expand All @@ -210,7 +201,7 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC

if (!$reflectionMethod->isPublic()) {
$class = $reflectionClass->name;
throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s() must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
throw new RuntimeException(sprintf('Cannot autowire service "%s": method "%s()" must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method));
}
$methodCalls[] = array($method, $this->autowireMethod($reflectionMethod, array()));
}
Expand All @@ -221,16 +212,16 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC
/**
* Autowires the constructor or a method.
*
* @param \ReflectionMethod $reflectionMethod
* @param array $arguments
* @param \ReflectionFunctionAbstract $reflectionMethod
* @param array $arguments
*
* @return array The autowired arguments
*
* @throws RuntimeException
*/
private function autowireMethod(\ReflectionMethod $reflectionMethod, array $arguments)
private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, array $arguments)
{
$class = $reflectionMethod->class;
$class = $reflectionMethod instanceof \ReflectionMethod ? $reflectionMethod->class : $this->currentId;
$method = $reflectionMethod->name;
$parameters = $reflectionMethod->getParameters();
if (method_exists('ReflectionMethod', 'isVariadic') && $reflectionMethod->isVariadic()) {
Expand Down
47 changes: 7 additions & 40 deletions 47 src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,11 @@ protected function processValue($value, $isRoot = false)
return parent::processValue($value, $isRoot);
}

$parameterBag = $this->container->getParameterBag();

if ($class = $value->getClass()) {
$class = $parameterBag->resolveValue($class);
}

$calls = $value->getMethodCalls();
$calls[] = array('__construct', $value->getArguments());

foreach ($calls as $i => $call) {
list($method, $arguments) = $call;
$method = $parameterBag->resolveValue($method);
$parameters = null;
$resolvedArguments = array();

Expand All @@ -51,10 +44,14 @@ protected function processValue($value, $isRoot = false)
continue;
}
if ('' === $key || '$' !== $key[0]) {
throw new InvalidArgumentException(sprintf('Invalid key "%s" found in arguments of method "%s" for service "%s": only integer or $named arguments are allowed.', $key, $method, $this->currentId));
throw new InvalidArgumentException(sprintf('Invalid key "%s" found in arguments of method "%s()" for service "%s": only integer or $named arguments are allowed.', $key, $method, $this->currentId));
}

$parameters = null !== $parameters ? $parameters : $this->getParameters($class, $method);
if (null === $parameters) {
$r = $this->getReflectionMethod($value, $method);
$class = $r instanceof \ReflectionMethod ? $r->class : $this->currentId;
$parameters = $r->getParameters();
}

foreach ($parameters as $j => $p) {
if ($key === '$'.$p->name) {
Expand All @@ -64,7 +61,7 @@ protected function processValue($value, $isRoot = false)
}
}

throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" has no argument named "%s". Check your service definition.', $this->currentId, $class, $method, $key));
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s()" has no argument named "%s". Check your service definition.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method, $key));
}

if ($resolvedArguments !== $call[1]) {
Expand All @@ -84,34 +81,4 @@ protected function processValue($value, $isRoot = false)

return parent::processValue($value, $isRoot);
}

/**
* @param string|null $class
* @param string $method
*
* @throws InvalidArgumentException
*
* @return array
*/
private function getParameters($class, $method)
{
if (!$class) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId));
}

if (!$r = $this->container->getReflectionClass($class)) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": class "%s" does not exist.', $this->currentId, $class));
}

if (!$r->hasMethod($method)) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" does not exist.', $this->currentId, $class, $method));
}

$method = $r->getMethod($method);
if (!$method->isPublic()) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" must be public.', $this->currentId, $class, $method->name));
}

return $method->getParameters();
}
}
16 changes: 9 additions & 7 deletions 16 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 @@ -611,20 +611,19 @@ public function testEmptyStringIsKept()
$this->assertEquals(array(new Reference('a'), '', new Reference('lille')), $container->getDefinition('foo')->getArguments());
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Service "a" can use either autowiring or a factory, not both.
*/
public function testWithFactory()
{
$container = new ContainerBuilder();

$container->register('a', __NAMESPACE__.'\A')
->setFactory('foo')
$container->register('foo', Foo::class);
$definition = $container->register('a', A::class)
->setFactory(array(A::class, 'create'))
->setAutowired(true);

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

$this->assertEquals(array(new Reference('foo')), $definition->getArguments());
}

/**
Expand Down Expand Up @@ -662,7 +661,7 @@ public function provideNotWireableCalls()
{
return array(
array('setNotAutowireable', 'Cannot autowire service "foo": argument $n of method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setNotAutowireable() has type "Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass" but this class does not exist.'),
array(null, 'Cannot autowire service "foo": method Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setProtectedMethod() must be public.'),
array(null, 'Cannot autowire service "foo": method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setProtectedMethod()" must be public.'),
);
}

Expand Down Expand Up @@ -745,6 +744,9 @@ public function __construct(Foo $foo)

class A
{
public static function create(Foo $foo)
{
}
}

class B extends A
Expand Down
24 changes: 21 additions & 3 deletions 24 src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveNamedArgumentsPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,23 @@ public function testProcess()
$this->assertEquals(array(array('setApiKey', array('123'))), $definition->getMethodCalls());
}

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

$container->register('factory', NoConstructor::class);
$definition = $container->register('foo', NoConstructor::class)
->setFactory(array(new Reference('factory'), 'create'))
->setArguments(array('$apiKey' => '123'));

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

$this->assertSame(array(0 => '123'), $definition->getArguments());
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
*/
public function testClassNull()
{
Expand All @@ -52,7 +67,7 @@ public function testClassNull()
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
*/
public function testClassNotExist()
{
Expand All @@ -66,7 +81,7 @@ public function testClassNotExist()
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
*/
public function testClassNoConstructor()
{
Expand Down Expand Up @@ -96,4 +111,7 @@ public function testArgumentNotFound()

class NoConstructor
{
public static function create($apiKey)
{
}
}
Morty Proxy This is a proxified and sanitized view of the page, visit original site.