bug symfony#12393 [DependencyInjection] inlined factory not reference…

…d (boekkooi)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes symfony#12393).

Discussion
----------

[DependencyInjection] inlined factory not referenced

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

While working with the DI I encountered a `You have requested a non-existent service "xxxxx"` exception.
Research it seems that a private reference was inlined but the private factory that was also referencing to the service was ignored.

A example of the problem:
```XML
        <service id="manager"
                 class="\stdClass"
                 factory-method="getX"
                 factory-service="factory"
                 public="false">
            <argument>X</argument>
        </service>
        <service id="repository"
                 class="\stdClass"
                 factory-method="getRepository"
                 factory-service="manager"
                 public="false">
            <argument>X</argument>
        </service>
        <service id="storage" class="\stdClass" public="false">
            <argument type="service" id="manager"/>
            <argument type="service" id="repository"/>
        </service>
```

What happens before the patch:
1. repository get's inlined
2. manager get's inlined for the first argument
3. manager get's removed since there was no reference.

After the first commit the following will happen:
1. repository get's inlined
2. manager get's inlined for the first argument
3. manager will not be removed since the inlined repository still references manager

This introduced a smell since InlineServiceDefinitionsPass was still inlining the manager for the first argument.

To fix this I have chosen that not inline factories if they are used more then once by the same definition.

So after the second commit the following will happen:
1. repository get's inlined

Personally I feel that the InlineServiceDefinitionsPass patch isn't the best possible one but that a different fix would probably mean breaking BC so it's probably a good idea to look at this for Symfony 3.0.

Commits
-------

7816a98 [DependencyInjection] inlined factory not referenced

src/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php

@@ -111,6 +111,10 @@ private function processArguments(array $arguments)
                 $this->processArguments($argument->getArguments());
                 $this->processArguments($argument->getMethodCalls());
                 $this->processArguments($argument->getProperties());
+
+                if ($argument->getFactoryService()) {
+                    $this->processArguments(array(new Reference($argument->getFactoryService())));
+                }
             }
         }
     }

src/Symfony/Component/DependencyInjection/Compiler/InlineServiceDefinitionsPass.php

@@ -138,6 +138,10 @@ private function isInlineableDefinition(ContainerBuilder $container, $id, Defini
             return false;
         }
 
+        if (count($ids) > 1 && $definition->getFactoryService()) {
+            return false;
+        }
+
         return $container->getDefinition(reset($ids))->getScope() === $definition->getScope();
     }
 }

src/Symfony/Component/DependencyInjection/Tests/Compiler/AnalyzeServiceReferencesPassTest.php

@@ -79,6 +79,28 @@ public function testProcessDetectsReferencesFromInlinedDefinitions()
         $this->assertSame($ref, $refs[0]->getValue());
     }
 
+    public function testProcessDetectsReferencesFromInlinedFactoryDefinitions()
+    {
+        $container = new ContainerBuilder();
+
+        $container
+            ->register('a')
+        ;
+
+        $factory = new Definition();
+        $factory->setFactoryService('a');
+
+        $container
+            ->register('b')
+            ->addArgument($factory)
+        ;
+
+        $graph = $this->process($container);
+
+        $this->assertTrue($graph->hasNode('a'));
+        $this->assertCount(1, $refs = $graph->getNode('a')->getInEdges());
+    }
+
     public function testProcessDoesNotSaveDuplicateReferences()
     {
         $container = new ContainerBuilder();

src/Symfony/Component/DependencyInjection/Tests/Compiler/InlineServiceDefinitionsPassTest.php

@@ -110,6 +110,84 @@ public function testProcessInlinesIfMultipleReferencesButAllFromTheSameDefinitio
         $this->assertSame($a, $inlinedArguments[0]);
     }
 
+    public function testProcessInlinesPrivateFactoryReference()
+    {
+        $container = new ContainerBuilder();
+
+        $container->register('a')->setPublic(false);
+        $b = $container
+            ->register('b')
+            ->setPublic(false)
+            ->setFactoryService('a')
+        ;
+
+        $container
+            ->register('foo')
+            ->setArguments(array(
+                $ref = new Reference('b'),
+            ));
+
+        $this->process($container);
+
+        $inlinedArguments = $container->getDefinition('foo')->getArguments();
+        $this->assertSame($b, $inlinedArguments[0]);
+    }
+
+    public function testProcessDoesNotInlinePrivateFactoryIfReferencedMultipleTimesWithinTheSameDefinition()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('a')
+        ;
+        $container
+            ->register('b')
+            ->setPublic(false)
+            ->setFactoryService('a')
+        ;
+
+        $container
+            ->register('foo')
+            ->setArguments(array(
+                    $ref1 = new Reference('b'),
+                    $ref2 = new Reference('b'),
+                ))
+        ;
+        $this->process($container);
+
+        $args = $container->getDefinition('foo')->getArguments();
+        $this->assertSame($ref1, $args[0]);
+        $this->assertSame($ref2, $args[1]);
+    }
+
+    public function testProcessDoesNotInlineReferenceWhenUsedByInlineFactory()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('a')
+        ;
+        $container
+            ->register('b')
+            ->setPublic(false)
+            ->setFactoryService('a')
+        ;
+
+        $inlineFactory = new Definition();
+        $inlineFactory->setPublic(false);
+        $inlineFactory->setFactoryService('b');
+
+        $container
+            ->register('foo')
+            ->setArguments(array(
+                    $ref = new Reference('b'),
+                    $inlineFactory,
+                ))
+        ;
+        $this->process($container);
+
+        $args = $container->getDefinition('foo')->getArguments();
+        $this->assertSame($ref, $args[0]);
+    }
+
     public function testProcessInlinesOnlyIfSameScope()
     {
         $container = new ContainerBuilder();