Skip to content

[DependencyInjection] Fix dumping non-shared factories with TaggedIteratorArgument #50262

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

Jump to bottom
Merged
merged 1 commit into from
May 12, 2023
Merged

[DependencyInjection] Fix dumping non-shared factories with TaggedIteratorArgument #50262

merged 1 commit into from
May 12, 2023

Conversation

marphi
Copy link
Contributor

@marphi marphi commented May 7, 2023

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!

@carsonbot carsonbot added this to the 6.3 milestone May 7, 2023
@fabpot
Copy link
Member

fabpot commented May 12, 2023

Thank you @marphi.

@fabpot fabpot force-pushed the issue_50257_di_generates_invalid_code branch from 68768f7 to a60dfcf Compare May 12, 2023 07:59
@fabpot fabpot merged commit 882d7a6 into symfony:6.3 May 12, 2023
@fabpot fabpot mentioned this pull request May 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

DI generates an invalid code. 'Undefined variable $containerRef'
5 participants
@marphi @fabpot @nicolas-grekas @stof @carsonbot