feature #22530 Making tags under _defaults always apply (weaverryan)

This PR was merged into the 3.3-dev branch. Discussion ---------- Making tags under _defaults always apply | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22497 | License | MIT | Doc PR | n/a Obviously, things under `_defaults` are applied to all services in that file. However, tags was an exception: it was *not* applied unless you have `inherit_tags`. The correct behavior is subjective, but after talking about it today, we (mostly) decided that tags *should* always apply. This does exactly that. One side-effect (explained in the commit message) is that if you have a parent and child service both in the same file, the tag from `_defaults` is applied twice. I think that makes sense, and at some point, we can't protect the users from their own configuration :). This kind of "weird" behavior is likely not a problem, as compiler passes now handle multiple tags well AND it only affects a case where the user has added tags to `_defaults` *and* is using parent-child definitions. That's quite a strange mixture of conditions :). Cheers! Commits ------- 037a782 Making tags under _defaults always apply and removing inherit_tags entirely
symfony · May 1, 2017 · 2d88787 · 2d88787
2 parents 7010a7a + 037a782
commit 2d88787
Show file tree

Hide file tree

Showing 16 changed files with 10 additions and 184 deletions.
diff --git a/src/Symfony/Component/DependencyInjection/CHANGELOG.md b/src/Symfony/Component/DependencyInjection/CHANGELOG.md
@@ -22,8 +22,7 @@ CHANGELOG
  * added "iterator" argument type for lazy iteration over a set of values and services
  * added "closure-proxy" argument type for turning services' methods into lazy callables
  * added file-wide configurable defaults for service attributes "public", "tags",
-   "autowire" and "inherit-tags"
- * added "inherit-tags" service attribute to control tags' inheritance from parent context
+   "autowire" and "autoconfigure"
  * made the "class" attribute optional, using the "id" as fallback
  * using the `PhpDumper` with an uncompiled `ContainerBuilder` is deprecated and
    will not be supported anymore in 4.0

diff --git a/src/Symfony/Component/DependencyInjection/ChildDefinition.php b/src/Symfony/Component/DependencyInjection/ChildDefinition.php
@@ -23,7 +23,6 @@
 class ChildDefinition extends Definition
 {
     private $parent;
-    private $inheritTags = false;
 
     /**
      * @param string $parent The id of Definition instance to decorate
@@ -57,30 +56,6 @@ public function setParent($parent)
         return $this;
     }
 
-    /**
-     * Sets whether tags should be inherited from the parent or not.
-     *
-     * @param bool $boolean
-     *
-     * @return $this
-     */
-    public function setInheritTags($boolean)
-    {
-        $this->inheritTags = (bool) $boolean;
-
-        return $this;
-    }
-
-    /**
-     * Returns whether tags should be inherited from the parent or not.
-     *
-     * @return bool
-     */
-    public function getInheritTags()
-    {
-        return $this->inheritTags;
-    }
-
     /**
      * Gets an argument to pass to the service constructor/factory method.
      *

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php b/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
@@ -43,7 +43,6 @@ public function __construct()
             100 => array(
                 $resolveClassPass = new ResolveClassPass(),
                 new ResolveInstanceofConditionalsPass(),
-                new ResolveTagsInheritancePass(),
             ),
         );
 

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
@@ -77,7 +77,7 @@ private function processDefinition(ContainerBuilder $container, $id, Definition
             foreach ($instanceofDefs as $key => $instanceofDef) {
                 /** @var ChildDefinition $instanceofDef */
                 $instanceofDef = clone $instanceofDef;
-                $instanceofDef->setAbstract(true)->setInheritTags(false)->setParent($parent ?: 'abstract.instanceof.'.$id);
+                $instanceofDef->setAbstract(true)->setParent($parent ?: 'abstract.instanceof.'.$id);
                 $parent = 'instanceof.'.$interface.'.'.$key.'.'.$id;
                 $container->setDefinition($parent, $instanceofDef);
                 $instanceofTags[] = $instanceofDef->getTags();

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveTagsInheritancePass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveTagsInheritancePass.php
diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -178,9 +178,6 @@ private function getServiceDefaults(\DOMDocument $xml, $file)
         if ($defaultsNode->hasAttribute('public')) {
             $defaults['public'] = XmlUtils::phpize($defaultsNode->getAttribute('public'));
         }
-        if ($defaultsNode->hasAttribute('inherit-tags')) {
-            $defaults['inherit-tags'] = XmlUtils::phpize($defaultsNode->getAttribute('inherit-tags'));
-        }
         if ($defaultsNode->hasAttribute('autoconfigure')) {
             $defaults['autoconfigure'] = XmlUtils::phpize($defaultsNode->getAttribute('autoconfigure'));
         }
@@ -225,13 +222,6 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults =
             }
 
             $definition = new ChildDefinition($parent);
-
-            if ($value = $service->getAttribute('inherit-tags')) {
-                $definition->setInheritTags(XmlUtils::phpize($value));
-            } elseif (isset($defaults['inherit-tags'])) {
-                $definition->setInheritTags($defaults['inherit-tags']);
-            }
-            $defaults = array();
         } else {
             $definition = new Definition();
 
@@ -318,13 +308,7 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults =
 
         $tags = $this->getChildren($service, 'tag');
 
-        if (empty($defaults['tags'])) {
-            // no-op
-        } elseif (!$value = $service->getAttribute('inherit-tags')) {
-            if (!$tags) {
-                $tags = $defaults['tags'];
-            }
-        } elseif (XmlUtils::phpize($value)) {
+        if (!empty($defaults['tags'])) {
             $tags = array_merge($tags, $defaults['tags']);
         }
 

diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
@@ -51,7 +51,6 @@ class YamlFileLoader extends FileLoader
         'configurator' => 'configurator',
         'calls' => 'calls',
         'tags' => 'tags',
-        'inherit_tags' => 'inherit_tags',
         'decorates' => 'decorates',
         'decoration_inner_name' => 'decoration_inner_name',
         'decoration_priority' => 'decoration_priority',
@@ -74,7 +73,6 @@ class YamlFileLoader extends FileLoader
         'configurator' => 'configurator',
         'calls' => 'calls',
         'tags' => 'tags',
-        'inherit_tags' => 'inherit_tags',
         'autowire' => 'autowire',
         'autoconfigure' => 'autoconfigure',
     );
@@ -93,7 +91,6 @@ class YamlFileLoader extends FileLoader
     private static $defaultsKeywords = array(
         'public' => 'public',
         'tags' => 'tags',
-        'inherit_tags' => 'inherit_tags',
         'autowire' => 'autowire',
         'autoconfigure' => 'autoconfigure',
     );
@@ -365,12 +362,6 @@ private function parseDefinition($id, $service, $file, array $defaults)
             }
 
             $definition = new ChildDefinition($service['parent']);
-
-            $inheritTag = isset($service['inherit_tags']) ? $service['inherit_tags'] : (isset($defaults['inherit_tags']) ? $defaults['inherit_tags'] : null);
-            if (null !== $inheritTag) {
-                $definition->setInheritTags($inheritTag);
-            }
-            $defaults = array();
         } else {
             $definition = new Definition();
 
@@ -458,13 +449,7 @@ private function parseDefinition($id, $service, $file, array $defaults)
             throw new InvalidArgumentException(sprintf('Parameter "tags" must be an array for service "%s" in %s. Check your YAML syntax.', $id, $file));
         }
 
-        if (!isset($defaults['tags'])) {
-            // no-op
-        } elseif (!isset($service['inherit_tags'])) {
-            if (!isset($service['tags'])) {
-                $tags = $defaults['tags'];
-            }
-        } elseif ($service['inherit_tags']) {
+        if (isset($defaults['tags'])) {
             $tags = array_merge($tags, $defaults['tags']);
         }
 

diff --git a/src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd b/src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
@@ -103,7 +103,6 @@
     </xsd:choice>
     <xsd:attribute name="public" type="boolean" />
     <xsd:attribute name="autowire" type="boolean" />
-    <xsd:attribute name="inherit-tags" type="boolean" />
     <xsd:attribute name="autoconfigure" type="boolean" />
   </xsd:complexType>
 
@@ -132,7 +131,6 @@
     <xsd:attribute name="decoration-inner-name" type="xsd:string" />
     <xsd:attribute name="decoration-priority" type="xsd:integer" />
     <xsd:attribute name="autowire" type="boolean" />
-    <xsd:attribute name="inherit-tags" type="boolean" />
     <xsd:attribute name="autoconfigure" type="boolean" />
   </xsd:complexType>
 
@@ -169,7 +167,6 @@
     <xsd:attribute name="abstract" type="boolean" />
     <xsd:attribute name="parent" type="xsd:string" />
     <xsd:attribute name="autowire" type="boolean" />
-    <xsd:attribute name="inherit-tags" type="boolean" />
     <xsd:attribute name="autoconfigure" type="boolean" />
   </xsd:complexType>
 

diff --git a/...ny/Component/DependencyInjection/Tests/Compiler/ResolveInstanceofConditionalsPassTest.php b/...ny/Component/DependencyInjection/Tests/Compiler/ResolveInstanceofConditionalsPassTest.php
@@ -15,7 +15,6 @@
 use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\Compiler\ResolveInstanceofConditionalsPass;
 use Symfony\Component\DependencyInjection\Compiler\ResolveDefinitionTemplatesPass;
-use Symfony\Component\DependencyInjection\Compiler\ResolveTagsInheritancePass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 
 class ResolveInstanceofConditionalsPassTest extends TestCase
@@ -35,7 +34,6 @@ public function testProcess()
         $this->assertEmpty($def->getInstanceofConditionals());
         $this->assertInstanceof(ChildDefinition::class, $def);
         $this->assertTrue($def->isAutowired());
-        $this->assertFalse($def->getInheritTags());
         $this->assertSame($parent, $def->getParent());
         $this->assertSame(array('tag' => array(array()), 'baz' => array(array('attr' => 123))), $def->getTags());
 
@@ -121,7 +119,6 @@ public function testProcessUsesAutoconfiguredInstanceof()
             ->setFactory('autoconfigured_factory');
 
         (new ResolveInstanceofConditionalsPass())->process($container);
-        (new ResolveTagsInheritancePass())->process($container);
         (new ResolveDefinitionTemplatesPass())->process($container);
 
         $def = $container->getDefinition('normal_service');

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveTagsInheritancePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveTagsInheritancePassTest.php
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services29.xml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services29.xml
@@ -6,7 +6,7 @@
         </defaults>
 
         <service id="with_defaults" class="Foo" />
-        <service id="no_defaults" class="Foo" public="true" autowire="false" inherit-tags="false">
+        <service id="no_defaults" class="Foo" public="true" autowire="false">
         </service>
     </services>
 </container>
diff --git a/...encyInjection/Tests/Fixtures/yaml/integration/defaults_instanceof_importance/expected.yml b/...encyInjection/Tests/Fixtures/yaml/integration/defaults_instanceof_importance/expected.yml
@@ -16,8 +16,7 @@ services:
             # these 2 are from instanceof
             - { name: foo_tag, tag_option: from_instanceof }
             - { name: bar_tag }
-            # the tag from defaults do NOT cascade (but see #22530)
-            # - { name: from_defaults }
+            - { name: from_defaults }
         # calls from instanceof are kept, but this comes later
         calls:
             # first call is from instanceof

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services28.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services28.yml
@@ -14,7 +14,6 @@ services:
         class: Foo
         public: true
         autowire: ~
-        inherit_tags: false
 
     no_defaults:
         class: Foo

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services31_invalid_tags.yml b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services31_invalid_tags.yml
@@ -4,4 +4,3 @@ services:
 
     Foo\Bar:
         tags: invalid
-        inherit_tags: true
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php
@@ -655,7 +655,7 @@ public function testDefaults()
 
         $this->assertTrue($container->getDefinition('no_defaults')->isPublic());
 
-        $this->assertSame(array(), $container->getDefinition('no_defaults')->getTags());
+        $this->assertSame(array('foo' => array(array())), $container->getDefinition('no_defaults')->getTags());
 
         $this->assertFalse($container->getDefinition('no_defaults')->isAutowired());
     }

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
@@ -412,8 +412,9 @@ public function testDefaults()
         $this->assertTrue($container->getDefinition('with_null')->isPublic());
         $this->assertTrue($container->getDefinition('no_defaults')->isPublic());
 
-        $this->assertSame(array(), $container->getDefinition('with_null')->getTags());
-        $this->assertSame(array(), $container->getDefinition('no_defaults')->getTags());
+        // foo tag is inherited from defaults
+        $this->assertSame(array('foo' => array(array())), $container->getDefinition('with_null')->getTags());
+        $this->assertSame(array('foo' => array(array())), $container->getDefinition('no_defaults')->getTags());
 
         $this->assertTrue($container->getDefinition('with_null')->isAutowired());
         $this->assertFalse($container->getDefinition('no_defaults')->isAutowired());