Skip to content

Commit

Permalink
bug #50262 [DependencyInjection] Fix dumping non-shared factories wit…
Browse files Browse the repository at this point in the history
…h TaggedIteratorArgument (marphi)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[DependencyInjection] Fix dumping non-shared factories with TaggedIteratorArgument

Fix dumping non-shared factories which have a configured `methodCall` with `TaggedIteratorArgument`.

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #50257
| License       | MIT

I've updated my project to the fresh 6.3-beta versions and discovered that DI generates an invalid code. I've reported issue #50257, where you can find more info and context.

The PHPDumper class generated code where use the `$containerRef` variable located in into anonymous function where this variable is not accessible.

`@nicolas`-grekas quickly hinted to me where probably the bug is located. It works! I've prepared PR with this fix, and a PHPUnit test covers this case.

Before the fix, my test case generated this code:
```
    protected static function getFooService($container)
    {
        $containerRef = $container->ref;

        $container->factories['foo'] = function ($container) {
            $instance = new \Symfony\Component\DependencyInjection\Tests\Dumper\FooClass();

            $instance->setOtherInstances(new RewindableGenerator(function () use ($containerRef) {
                $container = $containerRef->get();

                yield 0 => ($container->privates['Bar'] ??= new \Bar());
                yield 1 => ($container->privates['stdClass'] ??= new \stdClass());
            }, 2));

            return $instance;
        };

        return $container->factories['foo']($container);
    }
```
Which threw an error: `Warning: Undefined variable $containerRef` at this line:
```
            $instance->setOtherInstances(new RewindableGenerator(function () use ($containerRef) {
```

After the fix, looks good and works.
```
    protected static function getFooService($container)
    {
        $containerRef = $container->ref;

        $container->factories['foo'] = function ($container) use ($containerRef) {
            $instance = new \Symfony\Component\DependencyInjection\Tests\Dumper\FooClass();

            $instance->setOtherInstances(new RewindableGenerator(function () use ($containerRef) {
                $container = $containerRef->get();

                yield 0 => ($container->privates['Bar'] ??= new \Bar());
                yield 1 => ($container->privates['stdClass'] ??= new \stdClass());
            }, 2));

            return $instance;
        };

        return $container->factories['foo']($container);
    }
```

Thx `@nicolas`-grekas for your help!

Commits
-------

a60dfcf076 [DependencyInjection] Fix dumping non-shared factories with TaggedIteratorArgument
  • Loading branch information
fabpot committed May 12, 2023
2 parents bdac390 + 74b1e82 commit fef6389
Show file tree
Hide file tree
Showing 4 changed files with 168 additions and 1 deletion.
3 changes: 2 additions & 1 deletion Dumper/PhpDumper.php
Original file line number Diff line number Diff line change
Expand Up @@ -945,8 +945,9 @@ protected static function {$methodName}(\$container$lazyInitialization)
if (!$isProxyCandidate && !$definition->isShared()) {
$c = implode("\n", array_map(fn ($line) => $line ? ' '.$line : $line, explode("\n", $c)));
$lazyloadInitialization = $definition->isLazy() ? ', $lazyLoad = true' : '';
$useContainerRef = $this->addContainerRef ? ' use ($containerRef)' : '';

$c = sprintf(" %s = function (\$container%s) {\n%s };\n\n return %1\$s(\$container);\n", $factory, $lazyloadInitialization, $c);
$c = sprintf(" %s = function (\$container%s)%s {\n%s };\n\n return %1\$s(\$container);\n", $factory, $lazyloadInitialization, $useContainerRef, $c);
}

$code .= $c;
Expand Down
30 changes: 30 additions & 0 deletions Tests/Dumper/PhpDumperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
use Symfony\Component\DependencyInjection\Argument\ServiceLocator as ArgumentServiceLocator;
use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
use Symfony\Component\DependencyInjection\Argument\TaggedIteratorArgument;
use Symfony\Component\DependencyInjection\Attribute\Autowire;
use Symfony\Component\DependencyInjection\Attribute\AutowireCallable;
use Symfony\Component\DependencyInjection\Attribute\AutowireServiceClosure;
Expand Down Expand Up @@ -285,6 +286,35 @@ public function testDumpAsFilesWithFactoriesInlined()
$this->assertStringMatchesFormatFile(self::$fixturesPath.'/php/services9_inlined_factories.txt', $dump);
}

public function testDumpAsFilesWithFactoriesInlinedWithTaggedIterator()
{
$container = new ContainerBuilder();
$container
->register('foo', FooClass::class)
->addMethodCall('setOtherInstances', [new TaggedIteratorArgument('foo')])
->setShared(false)
->setPublic(true);

$container
->register('Bar', 'Bar')
->addTag('foo');

$container
->register('stdClass', '\stdClass')
->addTag('foo');

$container->compile();

$dumper = new PhpDumper($container);
$dump = print_r($dumper->dump(['as_files' => true, 'file' => __DIR__, 'hot_path_tag' => 'hot', 'build_time' => 1563381341, 'inline_factories' => true, 'inline_class_loader' => true]), true);

if ('\\' === \DIRECTORY_SEPARATOR) {
$dump = str_replace("'.\\DIRECTORY_SEPARATOR.'", '/', $dump);
}

$this->assertStringMatchesFormatFile(self::$fixturesPath.'/php/services9_inlined_factories_with_tagged_iterrator.txt', $dump);
}

public function testDumpAsFilesWithLazyFactoriesInlined()
{
$container = new ContainerBuilder();
Expand Down
6 changes: 6 additions & 0 deletions Tests/Fixtures/includes/foo.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class FooClass
public $qux;
public $foo;
public $moo;
public $otherInstances;

public $bar = null;
public $initialized = false;
Expand Down Expand Up @@ -41,4 +42,9 @@ public function setBar($value = null)
{
$this->bar = $value;
}

public function setOtherInstances($otherInstances)
{
$this->otherInstances = $otherInstances;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
Array
(
[Container%s/removed-ids.php] => <?php

namespace Container%s;

return [
'Bar' => true,
'stdClass' => true,
];

[Container%s/ProjectServiceContainer.php] => <?php

namespace Container%s;

use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface;

/**
* @internal This class has been auto-generated by the Symfony Dependency Injection Component.
*/
class ProjectServiceContainer extends Container
{
protected $targetDir;
protected $parameters = [];
protected readonly \WeakReference $ref;

public function __construct(private array $buildParameters = [], protected string $containerDir = __DIR__)
{
$this->ref = \WeakReference::create($this);
$this->targetDir = \dirname($containerDir);
$this->services = $this->privates = [];
$this->methodMap = [
'foo' => 'getFooService',
];

$this->aliases = [];
}

public function compile(): void
{
throw new LogicException('You cannot compile a dumped container that was already compiled.');
}

public function isCompiled(): bool
{
return true;
}

public function getRemovedIds(): array
{
return require $this->containerDir.\DIRECTORY_SEPARATOR.'removed-ids.php';
}

/**
* Gets the public 'foo' service.
*
* @return \Symfony\Component\DependencyInjection\Tests\Dumper\FooClass
*/
protected static function getFooService($container)
{
$containerRef = $container->ref;

$container->factories['foo'] = function ($container) use ($containerRef) {
$instance = new \Symfony\Component\DependencyInjection\Tests\Dumper\FooClass();

$instance->setOtherInstances(new RewindableGenerator(function () use ($containerRef) {
$container = $containerRef->get();

yield 0 => ($container->privates['Bar'] ??= new \Bar());
yield 1 => ($container->privates['stdClass'] ??= new \stdClass());
}, 2));

return $instance;
};

return $container->factories['foo']($container);
}
}

[ProjectServiceContainer.preload.php] => <?php

// This file has been auto-generated by the Symfony Dependency Injection Component
// You can reference it in the "opcache.preload" php.ini setting on PHP >= 7.4 when preloading is desired

use Symfony\Component\DependencyInjection\Dumper\Preloader;

if (in_array(PHP_SAPI, ['cli', 'phpdbg'], true)) {
return;
}

require dirname(__DIR__, %d).'/vendor/autoload.php';
(require __DIR__.'/ProjectServiceContainer.php')->set(\Container%s\ProjectServiceContainer::class, null);

$classes = [];
$classes[] = 'Bar';
$classes[] = 'Symfony\Component\DependencyInjection\Tests\Dumper\FooClass';
$classes[] = 'Symfony\Component\DependencyInjection\ContainerInterface';

$preloaded = Preloader::preload($classes);

[ProjectServiceContainer.php] => <?php

// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.

if (\class_exists(\Container%s\ProjectServiceContainer::class, false)) {
// no-op
} elseif (!include __DIR__.'/Container%s/ProjectServiceContainer.php') {
touch(__DIR__.'/Container%s.legacy');

return;
}

if (!\class_exists(ProjectServiceContainer::class, false)) {
\class_alias(\Container%s\ProjectServiceContainer::class, ProjectServiceContainer::class, false);
}

return new \Container%s\ProjectServiceContainer([
'container.build_hash' => '%s',
'container.build_id' => '3f6e2bc2',
'container.build_time' => 1563381341,
], __DIR__.\DIRECTORY_SEPARATOR.'Container%s');

)

0 comments on commit fef6389

Please sign in to comment.