Skip to content

Commit 169a250

Browse files
nicolas-grekasnicolas-grekas
committed
bug #62715 [DependencyInjection] Fix sharing services used only by tagged iterators (nicolas-grekas)
This PR was merged into the 6.4 branch. Discussion ---------- [DependencyInjection] Fix sharing services used only by tagged iterators | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #51095 | License | MIT Because tagged iterators are rewindable, the services they yield must be remembered, even if the iterator is the only consumer of said services. At the moment, there's an optimization in PhpDumper that inlines single-use services into their consumers, but when it's a tagged iterator, this conflicts with the rewindable feature. Commits ------- 77fef82 [DependencyInjection] Fix sharing services used only by tagged iterators
2 parents a50c2bf + 77fef82 commit 169a250

File tree

7 files changed

+123
-9
lines changed

7 files changed

+123
-9
lines changed

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class AnalyzeServiceReferencesPass extends AbstractRecursivePass
4141
private bool $lazy;
4242
private bool $byConstructor;
4343
private bool $byFactory;
44+
private bool $byMultiUseArgument;
4445
private array $definitions;
4546
private array $aliases;
4647

@@ -67,6 +68,7 @@ public function process(ContainerBuilder $container)
6768
$this->lazy = false;
6869
$this->byConstructor = false;
6970
$this->byFactory = false;
71+
$this->byMultiUseArgument = false;
7072
$this->definitions = $container->getDefinitions();
7173
$this->aliases = $container->getAliases();
7274

@@ -89,7 +91,12 @@ protected function processValue(mixed $value, bool $isRoot = false): mixed
8991

9092
if ($value instanceof ArgumentInterface) {
9193
$this->lazy = !$this->byFactory || !$value instanceof IteratorArgument;
94+
$byMultiUseArgument = $this->byMultiUseArgument;
95+
if ($value instanceof IteratorArgument) {
96+
$this->byMultiUseArgument = true;
97+
}
9298
parent::processValue($value->getValues());
99+
$this->byMultiUseArgument = $byMultiUseArgument;
93100
$this->lazy = $lazy;
94101

95102
return $value;
@@ -106,7 +113,8 @@ protected function processValue(mixed $value, bool $isRoot = false): mixed
106113
$value,
107114
$this->lazy || ($this->hasProxyDumper && $targetDefinition?->isLazy()),
108115
ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $value->getInvalidBehavior(),
109-
$this->byConstructor
116+
$this->byConstructor,
117+
$this->byMultiUseArgument
110118
);
111119

112120
if ($inExpression) {
@@ -117,7 +125,9 @@ protected function processValue(mixed $value, bool $isRoot = false): mixed
117125
$targetDefinition,
118126
$value,
119127
$this->lazy || $targetDefinition?->isLazy(),
120-
true
128+
true,
129+
$this->byConstructor,
130+
$this->byMultiUseArgument
121131
);
122132
}
123133

‎src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,15 @@ public function clear(): void
7474
/**
7575
* Connects 2 nodes together in the Graph.
7676
*/
77-
public function connect(?string $sourceId, mixed $sourceValue, ?string $destId, mixed $destValue = null, ?Reference $reference = null, bool $lazy = false, bool $weak = false, bool $byConstructor = false): void
77+
public function connect(?string $sourceId, mixed $sourceValue, ?string $destId, mixed $destValue = null, ?Reference $reference = null, bool $lazy = false, bool $weak = false, bool $byConstructor = false, bool $byMultiUseArgument = false): void
7878
{
7979
if (null === $sourceId || null === $destId) {
8080
return;
8181
}
8282

8383
$sourceNode = $this->createNode($sourceId, $sourceValue);
8484
$destNode = $this->createNode($destId, $destValue);
85-
$edge = new ServiceReferenceGraphEdge($sourceNode, $destNode, $reference, $lazy, $weak, $byConstructor);
85+
$edge = new ServiceReferenceGraphEdge($sourceNode, $destNode, $reference, $lazy, $weak, $byConstructor, $byMultiUseArgument);
8686

8787
$sourceNode->addOutEdge($edge);
8888
$destNode->addInEdge($edge);

‎src/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraphEdge.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,17 @@ class ServiceReferenceGraphEdge
2626
private bool $lazy;
2727
private bool $weak;
2828
private bool $byConstructor;
29+
private bool $byMultiUseArgument;
2930

30-
public function __construct(ServiceReferenceGraphNode $sourceNode, ServiceReferenceGraphNode $destNode, mixed $value = null, bool $lazy = false, bool $weak = false, bool $byConstructor = false)
31+
public function __construct(ServiceReferenceGraphNode $sourceNode, ServiceReferenceGraphNode $destNode, mixed $value = null, bool $lazy = false, bool $weak = false, bool $byConstructor = false, bool $byMultiUseArgument = false)
3132
{
3233
$this->sourceNode = $sourceNode;
3334
$this->destNode = $destNode;
3435
$this->value = $value;
3536
$this->lazy = $lazy;
3637
$this->weak = $weak;
3738
$this->byConstructor = $byConstructor;
39+
$this->byMultiUseArgument = $byMultiUseArgument;
3840
}
3941

4042
/**
@@ -84,4 +86,9 @@ public function isReferencedByConstructor(): bool
8486
{
8587
return $this->byConstructor;
8688
}
89+
90+
public function isFromMultiUseArgument(): bool
91+
{
92+
return $this->byMultiUseArgument;
93+
}
8794
}

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -710,7 +710,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
710710
$shouldShareInline = !$isProxyCandidate && $definition->isShared() && !isset($this->singleUsePrivateIds[$id]) && null === $lastWitherIndex;
711711
$serviceAccessor = \sprintf('$container->%s[%s]', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id));
712712
$return = match (true) {
713-
$shouldShareInline && !isset($this->circularReferences[$id]) && $isSimpleInstance=> 'return '.$serviceAccessor.' = ',
713+
$shouldShareInline && !isset($this->circularReferences[$id]) && $isSimpleInstance => 'return '.$serviceAccessor.' = ',
714714
$shouldShareInline && !isset($this->circularReferences[$id]) => $serviceAccessor.' = $instance = ',
715715
$shouldShareInline || !$isSimpleInstance => '$instance = ',
716716
default => 'return ',
@@ -2194,7 +2194,7 @@ private function isSingleUsePrivateNode(ServiceReferenceGraphNode $node): bool
21942194
if (!$value = $edge->getSourceNode()->getValue()) {
21952195
continue;
21962196
}
2197-
if ($edge->isLazy() || !$value instanceof Definition || !$value->isShared()) {
2197+
if ($edge->isLazy() || !$value instanceof Definition || !$value->isShared() || $edge->isFromMultiUseArgument()) {
21982198
return false;
21992199
}
22002200

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\DependencyInjection\ContainerBuilder;
1818
use Symfony\Component\DependencyInjection\Definition;
1919
use Symfony\Component\DependencyInjection\Reference;
20+
use Symfony\Component\ExpressionLanguage\Expression;
2021

2122
class AnalyzeServiceReferencesPassTest extends TestCase
2223
{
@@ -204,6 +205,29 @@ public function testProcessDetectsFactoryReferences()
204205
$this->assertCount(1, $graph->getNode('foo')->getInEdges());
205206
}
206207

208+
public function testExpressionReferenceKeepsConstructorFlag()
209+
{
210+
$container = new ContainerBuilder();
211+
$container->register('bar');
212+
$container
213+
->register('foo')
214+
->addArgument(new Expression('service("bar")'));
215+
216+
$graph = $this->process($container);
217+
218+
$edges = $graph->getNode('bar')->getInEdges();
219+
$exprEdge = null;
220+
foreach ($edges as $edge) {
221+
if ('.internal.reference_in_expression' === $edge->getSourceNode()->getId()) {
222+
$exprEdge = $edge;
223+
break;
224+
}
225+
}
226+
227+
$this->assertNotNull($exprEdge, 'Expression edge should exist.');
228+
$this->assertTrue($exprEdge->isReferencedByConstructor());
229+
}
230+
207231
protected function process(ContainerBuilder $container)
208232
{
209233
$pass = new AnalyzeServiceReferencesPass();

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,38 @@ public function testDumpAsFilesWithFactoriesInlinedWithTaggedIterator()
319319
$this->assertStringMatchesFormatFile(self::$fixturesPath.'/php/services9_inlined_factories_with_tagged_iterrator.txt', $dump);
320320
}
321321

322+
public function testTaggedIteratorServicesRemainSharedWhenUsedByFactory()
323+
{
324+
PhpDumperTest_TaggedIteratorService::reset();
325+
326+
$container = new ContainerBuilder();
327+
$container
328+
->register('tagged_service', PhpDumperTest_TaggedIteratorService::class)
329+
->addTag('tag1');
330+
$container
331+
->register('wrapper', PhpDumperTest_TaggedIteratorWrapper::class)
332+
->setFactory([new Reference('wrapper_factory'), 'create'])
333+
->setPublic(true);
334+
$container
335+
->register('wrapper_factory', PhpDumperTest_TaggedIteratorWrapperFactory::class)
336+
->setArguments([new TaggedIteratorArgument('tag1')]);
337+
338+
$container->compile();
339+
340+
$dumper = new PhpDumper($container);
341+
eval('?>'.$dumper->dump(['class' => 'Symfony_DI_PhpDumper_Test_Tagged_Iterator_Factory']));
342+
343+
$compiled = new \Symfony_DI_PhpDumper_Test_Tagged_Iterator_Factory();
344+
$wrapper = $compiled->get('wrapper');
345+
346+
$firstIteration = iterator_to_array($wrapper->getTaggedServices(), false);
347+
$secondIteration = iterator_to_array($wrapper->getTaggedServices(), false);
348+
349+
$this->assertCount(1, $firstIteration);
350+
$this->assertSame($firstIteration, $secondIteration);
351+
$this->assertSame(1, PhpDumperTest_TaggedIteratorService::$constructed);
352+
}
353+
322354
public function testDumpAsFilesWithLazyFactoriesInlined()
323355
{
324356
$container = new ContainerBuilder();
@@ -2272,3 +2304,44 @@ public static function create(ServiceLocator $locator, object $inner): \stdClass
22722304
return (object) ['inner' => $inner];
22732305
}
22742306
}
2307+
2308+
class PhpDumperTest_TaggedIteratorService
2309+
{
2310+
public static int $constructed = 0;
2311+
2312+
public function __construct()
2313+
{
2314+
++self::$constructed;
2315+
}
2316+
2317+
public static function reset(): void
2318+
{
2319+
self::$constructed = 0;
2320+
}
2321+
}
2322+
2323+
class PhpDumperTest_TaggedIteratorWrapper
2324+
{
2325+
public function __construct(
2326+
private iterable $taggedServices,
2327+
) {
2328+
}
2329+
2330+
public function getTaggedServices(): iterable
2331+
{
2332+
return $this->taggedServices;
2333+
}
2334+
}
2335+
2336+
class PhpDumperTest_TaggedIteratorWrapperFactory
2337+
{
2338+
public function __construct(
2339+
private iterable $taggedServices,
2340+
) {
2341+
}
2342+
2343+
public function create(): PhpDumperTest_TaggedIteratorWrapper
2344+
{
2345+
return new PhpDumperTest_TaggedIteratorWrapper($this->taggedServices);
2346+
}
2347+
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_almost_circular_private.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,7 @@ protected static function getMailer_TransportService($container)
656656
{
657657
$instance = (new \FactoryCircular(new RewindableGenerator(function () use ($container) {
658658
yield 0 => ($container->privates['mailer.transport_factory.amazon'] ?? self::getMailer_TransportFactory_AmazonService($container));
659-
yield 1 => self::getMailerInline_TransportFactory_AmazonService($container);
659+
yield 1 => ($container->privates['mailer_inline.transport_factory.amazon'] ?? self::getMailerInline_TransportFactory_AmazonService($container));
660660
}, 2)))->create();
661661

662662
if (isset($container->privates['mailer.transport'])) {
@@ -708,7 +708,7 @@ protected static function getMailerInline_TransportFactory_AmazonService($contai
708708
$a = new \stdClass();
709709
$a->handler = ($container->privates['mailer_inline.mailer'] ?? self::getMailerInline_MailerService($container));
710710

711-
return new \stdClass($a);
711+
return $container->privates['mailer_inline.transport_factory.amazon'] = new \stdClass($a);
712712
}
713713

714714
/**

0 commit comments

Comments
 (0)