8000 [DI] Mixing constructor injection and setter injection breaks php dumper · Issue #28824 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DI] Mixing constructor injection and setter injection breaks php dumper #28824

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
Closed
discordier opened this issue Oct 11, 2018 · 13 comments
Closed

[DI] Mixing constructor injection and setter injection breaks php dumper #28824

discordier opened this issue Oct 11, 2018 · 13 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@discordier
Copy link
Contributor

Symfony version(s) affected: 4.1 (since 4.1.5 - previous not affected)

Description
When having a "complex" service graph consisting a mixture of constructor injection and setter injection, the dumped function in the container will attempt to use a variable before initializing it.

The container will error out with something like:

Notice: Undefined variable: b

How to reproduce
I tried to reduce the amount of involved services to the absolute minimum, this is how far I got it narrowed down.
The following unit test breaks the container dumper on my machine.
Reducing it more, like removing any of the ->addMethodCall() declarations or any of the arguments of multiuse1, will not exhibit the issue anymore.

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
index 8cb11568f2..2767fd9847 100644
--- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
@@ -1071,6 +1071,49 @@ class PhpDumperTest extends TestCase
         $container = new \Symfony_DI_PhpDumper_Errored_Definition();
         $container->get('runtime_error');
     }
+
+    public function testCircularWithAddCalls()
+    {
+        $container = new ContainerBuilder();
+
+        $container
+            ->register('root', 'stdClass')
+            ->setArguments([new Reference('level2'), new Reference('multiuse1')])
+            ->setPublic(true);
+
+        $container
+            ->register('level2', FooForCircularWithAddCalls::class)
+            ->addMethodCall('call', [new Reference('level3')]);
+
+        $container->register('multiuse1', 'stdClass');
+
+        $container
+            ->register('level3', 'stdClass')
+            ->addArgument(new Reference('level4'));
+
+        $container
+            ->register('level4', 'stdClass')
+            ->setArguments([new Reference('multiuse1'), new Reference('level5')]);
+
+        $container
+            ->register('level5', 'stdClass')
+            ->addArgument(new Reference('level6'));
+
+        $container
+            ->register('level6', FooForCircularWithAddCalls::class)
+            ->addMethodCall('call', [new Reference('level5')]);
+
+        $container->compile();
+        $dumper = new PhpDumper($container);
+
+        $content = $dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Circular_With_Add_Calls'));
+
+        eval('?>'.$content);
+
+        $container = new \Symfony_DI_PhpDumper_Test_Circular_With_Add_Calls();
+
+        $this->assertInstanceOf(\stdClass::class, $container->get('root'));
+    }
 }
 
 class Rot13EnvVarProcessor implements EnvVarProcessorInterface
@@ -1096,3 +1139,10 @@ class FooForDeepGraph
         $this->bClone = clone $b;
     }
 }
+
+class FooForCircularWithAddCalls
+{
+    public function call(\stdClass $argument)
+    {
+    }
+}
@nicolas-grekas
Copy link
Member

This should be already fixed in 4.1@dev, can you please confirm?

@discordier
Copy link
Contributor Author

I get test failure on 61cf143

$ git describe 
v4.1.6-52-g61cf143727

@nicolas-grekas
Copy link
Member

I meant 4.1@dev for the symfony/dependency-injection component

@discordier
Copy link
Contributor Author

Still the same problem in symfony/dependency-injection@faaa406

git describe 
v4.1.6-2-gfaaa406b

@discordier
Copy link
Contributor Author
discordier commented Oct 11, 2018 •
edited
Loading

bisect lead me to symfony/dependency-injection@ef44ed4 which is the first bad commit.

The comment in #28366 (comment) seems related as he also gets an undefined variable notice so #28366 is an hot candidate for having introduced this.

Interesting fact: commit 6fec32c is broken but it's 2 parents 432487f + 8bc014c are both good - might there be some merge conflict?

@pkruithof
Copy link
Contributor

I have this issue too, though without any addMethodCall declarations. Here is the (truncated and redacted) compiled file:

<?php

use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;

// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the private 'feed.command_bus.prooph' shared autowired service.

$a = ($this->privates['Some\Class'] ?? $this->load('getSomeService.php'));

if (isset($this->privates['feed.command_bus.prooph'])) {
    return $this->privates['feed.command_bus.prooph'];
}

$c = ($this->privates['Prooph\EventStore\ActionEventEmitterEventStore'] ?? $this->load('getActionEventEmitterEventStoreService.php'));

if (isset($this->privates['feed.command_bus.prooph'])) {
    return $this->privates['feed.command_bus.prooph'];
}
$b = \Some\Class::create($c, $this->getEnv('foo'));
$e = ($this->privates['Some\Other\Class'] ?? $this->load('getSomeOtherService.php'));

if (isset($this->privates['feed.command_bus.prooph'])) {
    return $this->privates['feed.command_bus.prooph'];
}
$f = ($this->privates['Another\Class'] ?? $this->load('getAnotherService.php'));

if (isset($this->privates['feed.command_bus.prooph'])) {
    return $this->privates['feed.command_bus.prooph'];
}

$this->privates['feed.command_bus.prooph'] = $instance = \CommandBus\Factory::create(array(
    'Some\\CommandA' => new CommandHandler($a), 
    'Some\\CommandB' => new CommandHandler($b),
    'Some\\CommandC' => new CommandHandler($d),
    'Some\\CommandD' => new CommandHandler($e),
    'Some\\CommandE' => new CommandHandler($f), 
    'Some\\CommandF' => new CommandHandler($g), 
);

$d = \Some\Other\Class::create($c, $this->getEnv('bar'));

$g = \Another\Class::create($c, $this->getEnv('baz'));

return $instance;

A couple of things:

  • A check if the service is already in privates keeps repeating (if (isset(service)) { return service })
  • The dependencies are out of order ($c before $b), but maybe that's not a bad thing, as long as the service itself is created last. Which is problem Untitled #3
  • After the service creation, dependencies are created.

@nicolas-grekas
Copy link
Member

Can you please share the dumped container as xml with me please, eg using slack?

@discordier
Copy link
Contributor Author

@nicolas-grekas: Do you need anything else from me or is my unit test above enough?

@nicolas-grekas
Copy link
Member

I need to check your unit test first, thanks.

@pkruithof
Copy link
Contributor

Any news on this? A second project of mine now also has a broken container 😢

@nicolas-grekas
Copy link
Member

I'm having a look early next week.

@nicolas-grekas nicolas-grekas mentioned this issue Nov 6, 2018
Closed
@nicolas-grekas
Copy link
Member

Should be fixed in #29103

@nicolas-grekas nicolas-grekas mentioned this issue Nov 6, 2018
Merged
nicolas-grekas added a commit that referenced this issue Nov 6, 2018
@nicolas-grekas
bug #29104 [DI] fix dumping inlined services (nicolas-grekas)
6006448 
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix dumping inlined services

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28824 #29078
| License       | MIT
| Doc PR        | -

Same as #29103 but for 3.4.

This PR dump inline services using the call-stack to sort the code for instantiating them.
This makes easier to follow and matches the behavior one would expect (and has when using `ContainerBuiler` directly to create services.)

Commits
-------

a97606d [DI] fix dumping inlined services
@nicolas-grekas
Copy link
Member

(now merged up to master, should be easier to test)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @pkruithof @discordier @chalasr
0