Skip to content

[DependencyInjection] Automatically detect the definitions class when possible #19191

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

Merged
merged 1 commit into from
Sep 19, 2016
Merged
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
@@ -0,0 +1,89 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;

/**
* @author Guilhem N. <egetick@gmail.com>
*/
class FactoryReturnTypePass implements CompilerPassInterface
{
/**
* {@inheritdoc}
*/
public function process(ContainerBuilder $container)
{
// works only since php 7.0 and hhvm 3.11
if (!method_exists(\ReflectionMethod::class, 'getReturnType')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not PHP_VERSION_ID < 70000?

Copy link
Contributor Author

@GuilhemN GuilhemN Jun 27, 2016

Choose a reason for hiding this comment

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

Because this has to work on hhvm too.

Copy link
Member

Choose a reason for hiding this comment

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

Then you need to add a comment about this.

return;
}

foreach ($container->getDefinitions() as $id => $definition) {
$this->updateDefinition($container, $id, $definition);
}
}

private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $previous = array())
{
// circular reference
if (isset($previous[$id])) {
return;
}

$factory = $definition->getFactory();
if (null === $factory || null !== $definition->getClass()) {
return;
}

$class = null;
if (is_string($factory)) {
try {
$m = new \ReflectionFunction($factory);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like having reflection involved in a compiler pass. I know that this is the case for the autowiring pass (and one of the reasons I don't like it), but I'm not comfortable adding this "constraint" for the default and mainstream behavior of the DI engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's only used if the user doesn't explicitly define the service class (so will never be executed on current code base as it is currently throwing an error).
I see only static analysis to get ride of reflection but i don't think that's better.

As an example, this pass could be used to create services from callables (that's what psr is doing if i'm not wrong?) used with autowiring:

class ServiceBuilder {
   public function getFoo(): Foo
   {}

   public function getBar(Foo $foo): Bar
   {}

   public function getDeclaredServices()
   {
       return ['foo', 'bar'];
    }
}

} catch (\ReflectionException $e) {
return;
}
} else {
if ($factory[0] instanceof Reference) {
$previous[$id] = true;
$factoryDefinition = $container->findDefinition((string) $factory[0]);
$this->updateDefinition($container, (string) $factory[0], $factoryDefinition, $previous);
$class = $factoryDefinition->getClass();
} else {
$class = $factory[0];
}

try {
$m = new \ReflectionMethod($class, $factory[1]);
} catch (\ReflectionException $e) {
return;
}
}

$returnType = $m->getReturnType();
if (null !== $returnType && !$returnType->isBuiltin()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like hhvm doesn't detect built in types... Maybe i should just remove this check as this is unexpected anyway ?

Copy link
Member

Choose a reason for hiding this comment

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

should be reported as a bug to HHVM then

Copy link
Member

Choose a reason for hiding this comment

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

OK no, the issue is that the version of HHVM we use does not support scalar type hints, and so considers it as a typehint for the class Symfony\Component\DependencyInjection\Tests\Fixtures\int (which is indeed not builtin).

So please simply skip this test on HHVM

$returnType = (string) $returnType;
if (null !== $class) {
$declaringClass = $m->getDeclaringClass()->getName();
if ('self' === $returnType) {
$returnType = $declaringClass;
} elseif ('parent' === $returnType) {
$returnType = get_parent_class($declaringClass) ?: null;
}
}

$definition->setClass($returnType);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public function __construct()
new CheckDefinitionValidityPass(),
new ResolveReferencesToAliasesPass(),
new ResolveInvalidReferencesPass(),
new FactoryReturnTypePass(),
new AutowirePass(),
new AnalyzeServiceReferencesPass(true),
new CheckCircularReferencesPass(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection\Tests\Compiler;

use Symfony\Component\DependencyInjection\Compiler\FactoryReturnTypePass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\Tests\Fixtures\factoryFunction;
use Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryDummy;
use Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryParent;

/**
* @author Guilhem N. <egetick@gmail.com>
*/
class FactoryReturnTypePassTest extends \PHPUnit_Framework_TestCase
{
public function testProcess()
{
$container = new ContainerBuilder();

$factory = $container->register('factory');
$factory->setFactory(array(FactoryDummy::class, 'createFactory'));

$foo = $container->register('foo');
$foo->setFactory(array(new Reference('factory'), 'create'));

$bar = $container->register('bar', __CLASS__);
$bar->setFactory(array(new Reference('factory'), 'create'));

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

if (method_exists(\ReflectionMethod::class, 'getReturnType')) {
$this->assertEquals(FactoryDummy::class, $factory->getClass());
$this->assertEquals(\stdClass::class, $foo->getClass());
} else {
$this->assertNull($factory->getClass());
$this->assertNull($foo->getClass());
}
$this->assertEquals(__CLASS__, $bar->getClass());
}

/**
* @dataProvider returnTypesProvider
*/
public function testReturnTypes($factory, $returnType, $hhvmSupport = true)
{
if (!$hhvmSupport && defined('HHVM_VERSION')) {
$this->markTestSkipped('Scalar typehints not supported by hhvm.');
}

$container = new ContainerBuilder();

$service = $container->register('service');
$service->setFactory($factory);

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

if (method_exists(\ReflectionMethod::class, 'getReturnType')) {
$this->assertEquals($returnType, $service->getClass());
} else {
$this->assertNull($service->getClass());
}
}

public function returnTypesProvider()
{
return array(
// must be loaded before the function as they are in the same file
array(array(FactoryDummy::class, 'createBuiltin'), null, false),
array(array(FactoryDummy::class, 'createParent'), FactoryParent::class),
array(array(FactoryDummy::class, 'createSelf'), FactoryDummy::class),
array(factoryFunction::class, FactoryDummy::class),
);
}

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

$factory = $container->register('factory');
$factory->setFactory(array(new Reference('factory2'), 'createSelf'));

$factory2 = $container->register('factory2');
$factory2->setFactory(array(new Reference('factory'), 'create'));

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

$this->assertNull($factory->getClass());
$this->assertNull($factory2->getClass());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection\Tests\Fixtures;

class FactoryDummy extends FactoryParent
{
public static function createFactory(): FactoryDummy
{
}
Copy link
Member

Choose a reason for hiding this comment

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

please add a test with self as return type, and another one with parent, to ensure that they are resolved properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHP doesn't resolve this values. As they are reserved keywords I guess we should do it ourselves ?

Copy link
Member

Choose a reason for hiding this comment

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

yes


public function create(): \stdClass
{
}

// Not supported by hhvm
public function createBuiltin(): int
{
}

public static function createSelf(): self
{
}

public static function createParent(): parent
{
}
}

class FactoryParent
{
}

function factoryFunction(): FactoryDummy
{
}