feature #36390 [DI] remove restriction and allow mixing "parent" and …

…instanceof-conditionals/defaults/bindings (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[DI] remove restriction and allow mixing "parent" and instanceof-conditionals/defaults/bindings

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

We added these limitations because we didn't know how to merge all those dimensions. Yet, the code is perfectly able to handle all of them together.

This PR makes `parent` definitions first-class citizens again - no need to split them in a dedicated file anymore.

Commits
-------

6229253 [DI] remove restriction and allow mixing "parent" and instanceof-conditionals/defaults/bindings

src/Symfony/Component/DependencyInjection/CHANGELOG.md

@@ -8,6 +8,7 @@ CHANGELOG
  * added support to autowire public typed properties in php 7.4
  * added support for defining method calls, a configurator, and property setters in `InlineServiceConfigurator`
  * added possibility to define abstract service arguments
+ * allowed mixing "parent" and instanceof-conditionals/defaults/bindings
  * updated the signature of method `Definition::setDeprecated()` to `Definition::setDeprecation(string $package, string $version, string $message)`
  * updated the signature of method `Alias::setDeprecated()` to `Alias::setDeprecation(string $package, string $version, string $message)`
  * updated the signature of method `DeprecateTrait::deprecate()` to `DeprecateTrait::deprecation(string $package, string $version, string $message)`

src/Symfony/Component/DependencyInjection/ChildDefinition.php

@@ -11,7 +11,6 @@
 
 namespace Symfony\Component\DependencyInjection;
 
-use Symfony\Component\DependencyInjection\Exception\BadMethodCallException;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 use Symfony\Component\DependencyInjection\Exception\OutOfBoundsException;
 
@@ -105,20 +104,4 @@ public function replaceArgument($index, $value)
 
         return $this;
     }
-
-    /**
-     * @internal
-     */
-    public function setAutoconfigured(bool $autoconfigured): self
-    {
-        throw new BadMethodCallException('A ChildDefinition cannot be autoconfigured.');
-    }
-
-    /**
-     * @internal
-     */
-    public function setInstanceofConditionals(array $instanceof): self
-    {
-        throw new BadMethodCallException('A ChildDefinition cannot have instanceof conditionals set on it.');
-    }
 }

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

@@ -36,10 +36,6 @@ public function process(ContainerBuilder $container)
         }
 
         foreach ($container->getDefinitions() as $id => $definition) {
-            if ($definition instanceof ChildDefinition) {
-                // don't apply "instanceof" to children: it will be applied to their parent
-                continue;
-            }
             $container->setDefinition($id, $this->processDefinition($container, $id, $definition));
         }
     }
@@ -59,11 +55,12 @@ private function processDefinition(ContainerBuilder $container, string $id, Defi
         $conditionals = $this->mergeConditionals($autoconfiguredInstanceof, $instanceofConditionals, $container);
 
         $definition->setInstanceofConditionals([]);
-        $parent = $shared = null;
+        $shared = null;
         $instanceofTags = [];
         $instanceofCalls = [];
         $instanceofBindings = [];
         $reflectionClass = null;
+        $parent = $definition instanceof ChildDefinition ? $definition->getParent() : null;
 
         foreach ($conditionals as $interface => $instanceofDefs) {
             if ($interface !== $class && !(null === $reflectionClass ? $reflectionClass = ($container->getReflectionClass($class, false) ?: false) : $reflectionClass)) {
@@ -100,12 +97,14 @@ private function processDefinition(ContainerBuilder $container, string $id, Defi
         if ($parent) {
             $bindings = $definition->getBindings();
             $abstract = $container->setDefinition('.abstract.instanceof.'.$id, $definition);
-
-            // cast Definition to ChildDefinition
             $definition->setBindings([]);
             $definition = serialize($definition);
-            $definition = substr_replace($definition, '53', 2, 2);
-            $definition = substr_replace($definition, 'Child', 44, 0);
+
+            if (Definition::class === \get_class($abstract)) {
+                // cast Definition to ChildDefinition
+                $definition = substr_replace($definition, '53', 2, 2);
+                $definition = substr_replace($definition, 'Child', 44, 0);
+            }
             /** @var ChildDefinition $definition */
             $definition = unserialize($definition);
             $definition->setParent($parent);

src/Symfony/Component/DependencyInjection/Loader/Configurator/ServiceConfigurator.php

@@ -11,7 +11,6 @@
 
 namespace Symfony\Component\DependencyInjection\Loader\Configurator;
 
-use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 
@@ -62,11 +61,6 @@ public function __destruct()
         parent::__destruct();
 
         $this->container->removeBindings($this->id);
-
-        if (!$this->definition instanceof ChildDefinition) {
-            $this->container->setDefinition($this->id, $this->definition->setInstanceofConditionals($this->instanceof));
-        } else {
-            $this->container->setDefinition($this->id, $this->definition);
-        }
+        $this->container->setDefinition($this->id, $this->definition->setInstanceofConditionals($this->instanceof));
     }
 }

src/Symfony/Component/DependencyInjection/Loader/Configurator/ServicesConfigurator.php

@@ -72,8 +72,6 @@ final public function instanceof(string $fqcn): InstanceofConfigurator
     final public function set(?string $id, string $class = null): ServiceConfigurator
     {
         $defaults = $this->defaults;
-        $allowParent = !$defaults->getChanges() && empty($this->instanceof);
-
         $definition = new Definition();
 
         if (null === $id) {
@@ -92,7 +90,7 @@ final public function set(?string $id, string $class = null): ServiceConfigurato
         $definition->setBindings($defaults->getBindings());
         $definition->setChanges([]);
 
-        $configurator = new ServiceConfigurator($this->container, $this->instanceof, $allowParent, $this, $definition, $id, $defaults->getTags(), $this->path);
+        $configurator = new ServiceConfigurator($this->container, $this->instanceof, true, $this, $definition, $id, $defaults->getTags(), $this->path);
 
         return null !== $class ? $configurator->class($class) : $configurator;
     }
@@ -114,9 +112,7 @@ final public function alias(string $id, string $referencedId): AliasConfigurator
      */
     final public function load(string $namespace, string $resource): PrototypeConfigurator
     {
-        $allowParent = !$this->defaults->getChanges() && empty($this->instanceof);
-
-        return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, $allowParent);
+        return new PrototypeConfigurator($this, $this->loader, $this->defaults, $namespace, $resource, true);
     }
 
     /**
@@ -126,10 +122,9 @@ final public function load(string $namespace, string $resource): PrototypeConfig
      */
     final public function get(string $id): ServiceConfigurator
     {
-        $allowParent = !$this->defaults->getChanges() && empty($this->instanceof);
         $definition = $this->container->getDefinition($id);
 
-        return new ServiceConfigurator($this->container, $definition->getInstanceofConditionals(), $allowParent, $this, $definition, $id, []);
+        return new ServiceConfigurator($this->container, $definition->getInstanceofConditionals(), true, $this, $definition, $id, []);
     }
 
     /**

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/AutoconfigureTrait.php

@@ -11,7 +11,6 @@
 
 namespace Symfony\Component\DependencyInjection\Loader\Configurator\Traits;
 
-use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 
 trait AutoconfigureTrait
@@ -25,9 +24,6 @@ trait AutoconfigureTrait
      */
     final public function autoconfigure(bool $autoconfigured = true): self
     {
-        if ($autoconfigured && $this->definition instanceof ChildDefinition) {
-            throw new InvalidArgumentException(sprintf('The service "%s" cannot have a "parent" and also have "autoconfigure". Try disabling autoconfiguration for the service.', $this->id));
-        }
         $this->definition->setAutoconfigured($autoconfigured);
 
         return $this;

src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/ParentTrait.php

@@ -31,10 +31,6 @@ final public function parent(string $parent): self
 
         if ($this->definition instanceof ChildDefinition) {
             $this->definition->setParent($parent);
-        } elseif ($this->definition->isAutoconfigured()) {
-            throw new InvalidArgumentException(sprintf('The service "%s" cannot have a "parent" and also have "autoconfigure". Try disabling autoconfiguration for the service.', $this->id));
-        } elseif ($this->definition->getBindings()) {
-            throw new InvalidArgumentException(sprintf('The service "%s" cannot have a "parent" and also "bind" arguments.', $this->id));
         } else {
             // cast Definition to ChildDefinition
             $definition = serialize($this->definition);

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php

@@ -147,7 +147,7 @@ protected function setDefinition($id, Definition $definition)
             }
             $this->instanceof[$id] = $definition;
         } else {
-            $this->container->setDefinition($id, $definition instanceof ChildDefinition ? $definition : $definition->setInstanceofConditionals($this->instanceof));
+            $this->container->setDefinition($id, $definition->setInstanceofConditionals($this->instanceof));
         }
     }
 

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

@@ -226,45 +226,23 @@ private function parseDefinition(\DOMElement $service, string $file, array $defa
         if ($this->isLoadingInstanceof) {
             $definition = new ChildDefinition('');
         } elseif ($parent = $service->getAttribute('parent')) {
-            if (!empty($this->instanceof)) {
-                throw new InvalidArgumentException(sprintf('The service "%s" cannot use the "parent" option in the same file where "instanceof" configuration is defined as using both is not supported. Move your child definitions to a separate file.', $service->getAttribute('id')));
-            }
-
-            foreach ($defaults as $k => $v) {
-                if ('tags' === $k) {
-                    // since tags are never inherited from parents, there is no confusion
-                    // thus we can safely add them as defaults to ChildDefinition
-                    continue;
-                }
-                if ('bind' === $k) {
-                    if ($defaults['bind']) {
-                        throw new InvalidArgumentException(sprintf('Bound values on service "%s" cannot be inherited from "defaults" when a "parent" is set. Move your child definitions to a separate file.', $service->getAttribute('id')));
-                    }
-
-                    continue;
-                }
-                if (!$service->hasAttribute($k)) {
-                    throw new InvalidArgumentException(sprintf('Attribute "%s" on service "%s" cannot be inherited from "defaults" when a "parent" is set. Move your child definitions to a separate file or define this attribute explicitly.', $k, $service->getAttribute('id')));
-                }
-            }
-
             $definition = new ChildDefinition($parent);
         } else {
             $definition = new Definition();
+        }
 
-            if (isset($defaults['public'])) {
-                $definition->setPublic($defaults['public']);
-            }
-            if (isset($defaults['autowire'])) {
-                $definition->setAutowired($defaults['autowire']);
-            }
-            if (isset($defaults['autoconfigure'])) {
-                $definition->setAutoconfigured($defaults['autoconfigure']);
-            }
-
-            $definition->setChanges([]);
+        if (isset($defaults['public'])) {
+            $definition->setPublic($defaults['public']);
+        }
+        if (isset($defaults['autowire'])) {
+            $definition->setAutowired($defaults['autowire']);
+        }
+        if (isset($defaults['autoconfigure'])) {
+            $definition->setAutoconfigured($defaults['autoconfigure']);
         }
 
+        $definition->setChanges([]);
+
         foreach (['class', 'public', 'shared', 'synthetic', 'abstract'] as $key) {
             if ($value = $service->getAttribute($key)) {
                 $method = 'set'.$key;
@@ -284,11 +262,7 @@ private function parseDefinition(\DOMElement $service, string $file, array $defa
         }
 
         if ($value = $service->getAttribute('autoconfigure')) {
-            if (!$definition instanceof ChildDefinition) {
-                $definition->setAutoconfigured(XmlUtils::phpize($value));
-            } elseif ($value = XmlUtils::phpize($value)) {
-                throw new InvalidArgumentException(sprintf('The service "%s" cannot have a "parent" and also have "autoconfigure". Try setting autoconfigure="false" for the service.', $service->getAttribute('id')));
-            }
+            $definition->setAutoconfigured(XmlUtils::phpize($value));
         }
 
         if ($files = $this->getChildren($service, 'file')) {

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -378,45 +378,27 @@ private function parseDefinition(string $id, $service, string $file, array $defa
         if ($this->isLoadingInstanceof) {
             $definition = new ChildDefinition('');
         } elseif (isset($service['parent'])) {
-            if (!empty($this->instanceof)) {
-                throw new InvalidArgumentException(sprintf('The service "%s" cannot use the "parent" option in the same file where "_instanceof" configuration is defined as using both is not supported. Move your child definitions to a separate file.', $id));
-            }
-
-            foreach ($defaults as $k => $v) {
-                if ('tags' === $k) {
-                    // since tags are never inherited from parents, there is no confusion
-                    // thus we can safely add them as defaults to ChildDefinition
-                    continue;
-                }
-                if ('bind' === $k) {
-                    throw new InvalidArgumentException(sprintf('Attribute "bind" on service "%s" cannot be inherited from "_defaults" when a "parent" is set. Move your child definitions to a separate file.', $id));
-                }
-                if (!isset($service[$k])) {
-                    throw new InvalidArgumentException(sprintf('Attribute "%s" on service "%s" cannot be inherited from "_defaults" when a "parent" is set. Move your child definitions to a separate file or define this attribute explicitly.', $k, $id));
-                }
-            }
-
             if ('' !== $service['parent'] && '@' === $service['parent'][0]) {
                 throw new InvalidArgumentException(sprintf('The value of the "parent" option for the "%s" service must be the id of the service without the "@" prefix (replace "%s" with "%s").', $id, $service['parent'], substr($service['parent'], 1)));
             }
 
             $definition = new ChildDefinition($service['parent']);
         } else {
             $definition = new Definition();
+        }
 
-            if (isset($defaults['public'])) {
-                $definition->setPublic($defaults['public']);
-            }
-            if (isset($defaults['autowire'])) {
-                $definition->setAutowired($defaults['autowire']);
-            }
-            if (isset($defaults['autoconfigure'])) {
-                $definition->setAutoconfigured($defaults['autoconfigure']);
-            }
-
-            $definition->setChanges([]);
+        if (isset($defaults['public'])) {
+            $definition->setPublic($defaults['public']);
+        }
+        if (isset($defaults['autowire'])) {
+            $definition->setAutowired($defaults['autowire']);
+        }
+        if (isset($defaults['autoconfigure'])) {
+            $definition->setAutoconfigured($defaults['autoconfigure']);
         }
 
+        $definition->setChanges([]);
+
         if (isset($service['class'])) {
             $definition->setClass($service['class']);
         }
@@ -612,11 +594,7 @@ private function parseDefinition(string $id, $service, string $file, array $defa
         }
 
         if (isset($service['autoconfigure'])) {
-            if (!$definition instanceof ChildDefinition) {
-                $definition->setAutoconfigured($service['autoconfigure']);
-            } elseif ($service['autoconfigure']) {
-                throw new InvalidArgumentException(sprintf('The service "%s" cannot have a "parent" and also have "autoconfigure". Try setting "autoconfigure: false" for the service.', $id));
-            }
+            $definition->setAutoconfigured($service['autoconfigure']);
         }
 
         if (\array_key_exists('namespace', $service) && !\array_key_exists('resource', $service)) {

src/Symfony/Component/DependencyInjection/Tests/ChildDefinitionTest.php

@@ -127,17 +127,20 @@ public function testGetArgumentShouldCheckBounds()
         $def->getArgument(1);
     }
 
-    public function testCannotCallSetAutoconfigured()
+    public function testAutoconfigured()
     {
-        $this->expectException('Symfony\Component\DependencyInjection\Exception\BadMethodCallException');
         $def = new ChildDefinition('foo');
         $def->setAutoconfigured(true);
+
+        $this->assertTrue($def->isAutoconfigured());
     }
 
-    public function testCannotCallSetInstanceofConditionals()
+    public function testInstanceofConditionals()
     {
-        $this->expectException('Symfony\Component\DependencyInjection\Exception\BadMethodCallException');
+        $conditionals = ['Foo' => new ChildDefinition('')];
         $def = new ChildDefinition('foo');
-        $def->setInstanceofConditionals(['Foo' => new ChildDefinition('')]);
+        $def->setInstanceofConditionals($conditionals);
+
+        $this->assertSame($conditionals, $def->getInstanceofConditionals());
     }
 }

src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services_instanceof_with_parent.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
     <services>
-        <instanceof id="FooInterface" autowire="true" />
+        <instanceof id="stdClass" autowire="true" />
 
         <service id="parent_service" class="stdClass" public="true"/>
         <service id="child_service" class="stdClass" parent="parent_service" />

src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_instanceof_with_parent.yml

@@ -1,6 +1,6 @@
 services:
     _instanceof:
-        FooInterface:
+        stdClass:
             autowire: true
 
     parent_service:

src/Symfony/Component/DependencyInjection/Tests/Loader/PhpFileLoaderTest.php

@@ -79,15 +79,15 @@ public function provideConfig()
         yield ['lazy_fqcn'];
     }
 
-    public function testAutoConfigureAndChildDefinitionNotAllowed()
+    public function testAutoConfigureAndChildDefinition()
     {
-        $this->expectException('Symfony\Component\DependencyInjection\Exception\InvalidArgumentException');
-        $this->expectExceptionMessage('The service "child_service" cannot have a "parent" and also have "autoconfigure". Try disabling autoconfiguration for the service.');
         $fixtures = realpath(__DIR__.'/../Fixtures');
         $container = new ContainerBuilder();
         $loader = new PhpFileLoader($container, new FileLocator());
         $loader->load($fixtures.'/config/services_autoconfigure_with_parent.php');
         $container->compile();
+
+        $this->assertTrue($container->getDefinition('child_service')->isAutoconfigured());
     }
 
     public function testFactoryShortNotationNotAllowed()