Skip to content

Commit ad0e25f

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#48093 [DependencyInjection] don't move locator tag for service subscriber (RobertMe)
This PR was merged into the 4.4 branch. Discussion ---------- [DependencyInjection] don't move locator tag for service subscriber | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - <!-- Replace this notice by a short README for your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the latest branch. - For new features, provide some code snippets to help understand usage. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> **From the commit message:** Decorators move tags applied to the decorated service to the decorating service. But this (sometimes) breaks when the decorated service is a service subscriber, which has the argument for the container explicitly set. This mostly works because the locator for the service subscriber is applied twice. The RegisterServiceSubscriberPass which creates the locator also sets a binding on the service. The ResolveServiceSubscriberPass replaces the arguments referencing the ContainerInterface or ServiceProviderInterface for those services tagged with the container.service_subscriber.locator tag. So when the argument isn't provided in the service definition it will automatically be set using the binding. And in case the argument is set, it will be replaced by the Resolver pass based on the tag. But this thus breaks in case a service explicitly sets the argument (which means the binding isn't applied) and the service being decorated (meaning the locator tag is "lost"). So add the locator tag to the list of tags to keep on the original service. **Explanation:** I found this issue when decorating the `Router`. The `Router` (in `FrameworkBundle`) uses a service subscriber, but this lead to a deprecation message for autowiring `Psr\...\ContainerInterface`. Debugging also showed that the full container was injected, and not the extracted service locator (service locator service actually was logged as removed for being unused). After investigation the issue was found to be as described above. The `router` service is declared with an argument for the `$container` parameter of the constructor, i.e.: ```xml <service id="router.default" class="Symfony\Bundle\FrameworkBundle\Routing\Router"> <argument type="service" id="Psr\Container\ContainerInterface" /> ``` Which leads to the binding, as declared in the `RegisterServiceSubscribersPass` pass not being applied. Later on the `DecoratorServicePass` pass moves the tags of the decorated service to the decorating service, where it only keeps the `container.service_subscriber` tag on the original service, and moves the `container.service_subscriber.locator` tag to the decorating service. When afterwards the `ResolveServiceSubscribersPass` pass is executed it will replace the arguments to the `ContainerInterface` with the created locator service (as defined in the `RegisterServiceSubscribersPass`). But this then fails because the `container.service_subscriber.locator` tag isn't applied anymore. So when the `router` isn't decorated the `ResolveServiceSubscribersPass` pass is the one which makes the service subscriber work, replacing the original argument as defined in the service definition. But when it is decorated this breaks because the tag is missing. The unit tests didn't detect this because: 1. the container isn't injected (and thus not validated); 2. even with validation of the container it would work as the binding would be applied. This is why I also kept the original unit test (but expanding the test with validating the container), which would still pass (based on the binding), and adding the additional test which explicitly sets the `$container` argument, which would fail (for the binding not being applied, and the tag being missing because of the decorator). Commits ------- 3c7dbb4 [DependencyInjection] don't move locator tag for service subscriber
2 parents 538491b + 3c7dbb4 commit ad0e25f

File tree

3 files changed

+35
-4
lines changed

3 files changed

+35
-4
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public function process(ContainerBuilder $container)
4242

4343
$tagsToKeep = $container->hasParameter('container.behavior_describing_tags')
4444
? $container->getParameter('container.behavior_describing_tags')
45-
: ['container.do_not_inline', 'container.service_locator', 'container.service_subscriber'];
45+
: ['container.do_not_inline', 'container.service_locator', 'container.service_subscriber', 'container.service_subscriber.locator'];
4646

4747
foreach ($definitions as [$id, $definition]) {
4848
$decoratedService = $definition->getDecoratedService();

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ public function testProcessLeavesServiceSubscriberTagOnOriginalDefinition()
249249
$container = new ContainerBuilder();
250250
$container
251251
->register('foo')
252-
->setTags(['container.service_subscriber' => [], 'bar' => ['attr' => 'baz']])
252+
->setTags(['container.service_subscriber' => [], 'container.service_subscriber.locator' => [], 'bar' => ['attr' => 'baz']])
253253
;
254254
$container
255255
->register('baz')
@@ -259,7 +259,7 @@ public function testProcessLeavesServiceSubscriberTagOnOriginalDefinition()
259259

260260
$this->process($container);
261261

262-
$this->assertEquals(['container.service_subscriber' => []], $container->getDefinition('baz.inner')->getTags());
262+
$this->assertEquals(['container.service_subscriber' => [], 'container.service_subscriber.locator' => []], $container->getDefinition('baz.inner')->getTags());
263263
$this->assertEquals(['bar' => ['attr' => 'baz'], 'foobar' => ['attr' => 'bar']], $container->getDefinition('baz')->getTags());
264264
}
265265

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\DependencyInjection\Tests\Compiler;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Psr\Container\ContainerInterface;
1516
use Symfony\Component\Config\FileLocator;
1617
use Symfony\Component\DependencyInjection\Alias;
1718
use Symfony\Component\DependencyInjection\Argument\ServiceLocatorArgument;
@@ -129,19 +130,41 @@ public function testProcessInlinesWhenThereAreMultipleReferencesButFromTheSameDe
129130
$this->assertFalse($container->hasDefinition('c'), 'Service C was not inlined.');
130131
}
131132

132-
public function testCanDecorateServiceSubscriber()
133+
public function testCanDecorateServiceSubscriberUsingBinding()
133134
{
134135
$container = new ContainerBuilder();
135136
$container->register(ServiceSubscriberStub::class)
136137
->addTag('container.service_subscriber')
137138
->setPublic(true);
138139

139140
$container->register(DecoratedServiceSubscriber::class)
141+
->setProperty('inner', new Reference(DecoratedServiceSubscriber::class.'.inner'))
140142
->setDecoratedService(ServiceSubscriberStub::class);
141143

142144
$container->compile();
143145

144146
$this->assertInstanceOf(DecoratedServiceSubscriber::class, $container->get(ServiceSubscriberStub::class));
147+
$this->assertInstanceOf(ServiceSubscriberStub::class, $container->get(ServiceSubscriberStub::class)->inner);
148+
$this->assertInstanceOf(ServiceLocator::class, $container->get(ServiceSubscriberStub::class)->inner->container);
149+
}
150+
151+
public function testCanDecorateServiceSubscriberReplacingArgument()
152+
{
153+
$container = new ContainerBuilder();
154+
$container->register(ServiceSubscriberStub::class)
155+
->setArguments([new Reference(ContainerInterface::class)])
156+
->addTag('container.service_subscriber')
157+
->setPublic(true);
158+
159+
$container->register(DecoratedServiceSubscriber::class)
160+
->setProperty('inner', new Reference(DecoratedServiceSubscriber::class.'.inner'))
161+
->setDecoratedService(ServiceSubscriberStub::class);
162+
163+
$container->compile();
164+
165+
$this->assertInstanceOf(DecoratedServiceSubscriber::class, $container->get(ServiceSubscriberStub::class));
166+
$this->assertInstanceOf(ServiceSubscriberStub::class, $container->get(ServiceSubscriberStub::class)->inner);
167+
$this->assertInstanceOf(ServiceLocator::class, $container->get(ServiceSubscriberStub::class)->inner->container);
145168
}
146169

147170
public function testCanDecorateServiceLocator()
@@ -515,6 +538,13 @@ public function testTaggedServiceLocatorWithDefaultIndex()
515538

516539
class ServiceSubscriberStub implements ServiceSubscriberInterface
517540
{
541+
public $container;
542+
543+
public function __construct(ContainerInterface $container)
544+
{
545+
$this->container = $container;
546+
}
547+
518548
public static function getSubscribedServices(): array
519549
{
520550
return [];
@@ -523,6 +553,7 @@ public static function getSubscribedServices(): array
523553

524554
class DecoratedServiceSubscriber
525555
{
556+
public $inner;
526557
}
527558

528559
class DecoratedServiceLocator implements ServiceProviderInterface

0 commit comments

Comments
 (0)