feature #47364 [DependencyInjection] Allow array attributes for service tags (aschempp)

fabpot · fabpot · commit c10b2247414a · 2022-10-24T14:22:51.000+02:00
This PR was merged into the 6.2 branch. Discussion ---------- [DependencyInjection] Allow array attributes for service tags | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix symfony/symfony#38339, symfony/symfony#38540 | License | MIT | Doc PR | symfony/symfony-docs#...  This adds array attribute support for container service tags as described in symfony/symfony#38339. The most notable change is the additions to the XML schema for services, because that's the current limitation for not using array. This is reopening symfony/symfony#38540 as new PR since the other one was ("accidentially") closed 😊 /cc `@nicolas`-grekas Commits ------- edd8d776a0 [DependencyInjection] Allow array attributes for service tags
diff --git a/Dumper/XmlDumper.php b/Dumper/XmlDumper.php
@@ -137,8 +137,14 @@ private function addService(Definition $definition, ?string $id, \DOMElement $pa
                 } else {
                     $tag->appendChild($this->document->createTextNode($name));
                 }
-                foreach ($attributes as $key => $value) {
-                    $tag->setAttribute($key, $value ?? '');
+
+                // Check if we have recursive attributes
+                if (array_filter($attributes, \is_array(...))) {
+                    $this->addTagRecursiveAttributes($tag, $attributes);
+                } else {
+                    foreach ($attributes as $key => $value) {
+                        $tag->setAttribute($key, $value ?? '');
+                    }
                 }
                 $service->appendChild($tag);
             }
@@ -261,6 +267,22 @@ private function addServices(\DOMElement $parent)
         $parent->appendChild($services);
     }
 
+    private function addTagRecursiveAttributes(\DOMElement $parent, array $attributes)
+    {
+        foreach ($attributes as $name => $value) {
+            $attribute = $this->document->createElement('attribute');
+            $attribute->setAttribute('name', $name);
+
+            if (\is_array($value)) {
+                $this->addTagRecursiveAttributes($attribute, $value);
+            } else {
+                $attribute->appendChild($this->document->createTextNode($value));
+            }
+
+            $parent->appendChild($attribute);
+        }
+    }
+
     private function convertParameters(array $parameters, string $type, \DOMElement $parent, string $keyAttribute = 'key')
     {
         $withKeys = !array_is_list($parameters);
diff --git a/Loader/Configurator/DefaultsConfigurator.php b/Loader/Configurator/DefaultsConfigurator.php
@@ -48,11 +48,7 @@ final public function tag(string $name, array $attributes = []): static
             throw new InvalidArgumentException('The tag name in "_defaults" must be a non-empty string.');
         }
 
-        foreach ($attributes as $attribute => $value) {
-            if (null !== $value && !\is_scalar($value)) {
-                throw new InvalidArgumentException(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type.', $name, $attribute));
-            }
-        }
+        $this->validateAttributes($name, $attributes);
 
         $this->definition->addTag($name, $attributes);
 
@@ -66,4 +62,15 @@ final public function instanceof(string $fqcn): InstanceofConfigurator
     {
         return $this->parent->instanceof($fqcn);
     }
+
+    private function validateAttributes(string $tagName, array $attributes, string $prefix = ''): void
+    {
+        foreach ($attributes as $attribute => $value) {
+            if (\is_array($value)) {
+                $this->validateAttributes($tagName, $value, $attribute.'.');
+            } elseif (!\is_scalar($value ?? '')) {
+                throw new InvalidArgumentException(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type or an array of scalar-type.', $tagName, $prefix.$attribute));
+            }
+        }
+    }
 }
diff --git a/Loader/Configurator/Traits/TagTrait.php b/Loader/Configurator/Traits/TagTrait.php
@@ -26,14 +26,21 @@ final public function tag(string $name, array $attributes = []): static
             throw new InvalidArgumentException(sprintf('The tag name for service "%s" must be a non-empty string.', $this->id));
         }
 
-        foreach ($attributes as $attribute => $value) {
-            if (!\is_scalar($value) && null !== $value) {
-                throw new InvalidArgumentException(sprintf('A tag attribute must be of a scalar-type for service "%s", tag "%s", attribute "%s".', $this->id, $name, $attribute));
-            }
-        }
+        $this->validateAttributes($name, $attributes);
 
         $this->definition->addTag($name, $attributes);
 
         return $this;
     }
+
+    private function validateAttributes(string $tagName, array $attributes, string $prefix = ''): void
+    {
+        foreach ($attributes as $attribute => $value) {
+            if (\is_array($value)) {
+                $this->validateAttributes($tagName, $value, $attribute.'.');
+            } elseif (!\is_scalar($value ?? '')) {
+                throw new InvalidArgumentException(sprintf('A tag attribute must be of a scalar-type or an array of scalar-types for service "%s", tag "%s", attribute "%s".', $this->id, $tagName, $prefix.$attribute));
+            }
+        }
+    }
 }
diff --git a/Loader/XmlFileLoader.php b/Loader/XmlFileLoader.php
@@ -336,10 +336,13 @@ private function parseDefinition(\DOMElement $service, string $file, Definition
         $tags = $this->getChildren($service, 'tag');
 
         foreach ($tags as $tag) {
-            $parameters = [];
-            $tagName = $tag->nodeValue;
+            if ('' === $tagName = $tag->hasChildNodes() || '' === $tag->nodeValue ? $tag->getAttribute('name') : $tag->nodeValue) {
+                throw new InvalidArgumentException(sprintf('The tag name for service "%s" in "%s" must be a non-empty string.', (string) $service->getAttribute('id'), $file));
+            }
+
+            $parameters = $this->getTagAttributes($tag, sprintf('The attribute name of tag "%s" for service "%s" in %s must be a non-empty string.', $tagName, (string) $service->getAttribute('id'), $file));
             foreach ($tag->attributes as $name => $node) {
-                if ('name' === $name && '' === $tagName) {
+                if ('name' === $name) {
                     continue;
                 }
 
@@ -350,10 +353,6 @@ private function parseDefinition(\DOMElement $service, string $file, Definition
                 $parameters[$name] = XmlUtils::phpize($node->nodeValue);
             }
 
-            if ('' === $tagName && '' === $tagName = $tag->getAttribute('name')) {
-                throw new InvalidArgumentException(sprintf('The tag name for service "%s" in "%s" must be a non-empty string.', $service->getAttribute('id'), $file));
-            }
-
             $definition->addTag($tagName, $parameters);
         }
 
@@ -592,6 +591,30 @@ private function getChildren(\DOMNode $node, string $name): array
         return $children;
     }
 
+    private function getTagAttributes(\DOMNode $node, string $missingName): array
+    {
+        $parameters = [];
+        $children = $this->getChildren($node, 'attribute');
+
+        foreach ($children as $childNode) {
+            if ('' === $name = $childNode->getAttribute('name')) {
+                throw new InvalidArgumentException($missingName);
+            }
+
+            if ($this->getChildren($childNode, 'attribute')) {
+                $parameters[$name] = $this->getTagAttributes($childNode, $missingName);
+            } else {
+                if (str_contains($name, '-') && !str_contains($name, '_') && !\array_key_exists($normalizedName = str_replace('-', '_', $name), $parameters)) {
+                    $parameters[$normalizedName] = XmlUtils::phpize($childNode->nodeValue);
+                }
+                // keep not normalized key
+                $parameters[$name] = XmlUtils::phpize($childNode->nodeValue);
+            }
+        }
+
+        return $parameters;
+    }
+
     /**
      * Validates a documents XML schema.
      *
diff --git a/Loader/YamlFileLoader.php b/Loader/YamlFileLoader.php
@@ -297,11 +297,7 @@ private function parseDefaults(array &$content, string $file): array
                     throw new InvalidArgumentException(sprintf('The tag name in "_defaults" must be a non-empty string in "%s".', $file));
                 }
 
-                foreach ($tag as $attribute => $value) {
-                    if (!\is_scalar($value) && null !== $value) {
-                        throw new InvalidArgumentException(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type in "%s". Check your YAML syntax.', $name, $attribute, $file));
-                    }
-                }
+                $this->validateAttributes(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type in "%s". Check your YAML syntax.', $name, '%s', $file), $tag);
             }
         }
 
@@ -605,11 +601,7 @@ private function parseDefinition(string $id, array|string|null $service, string
                 throw new InvalidArgumentException(sprintf('The tag name for service "%s" in "%s" must be a non-empty string.', $id, $file));
             }
 
-            foreach ($tag as $attribute => $value) {
-                if (!\is_scalar($value) && null !== $value) {
-                    throw new InvalidArgumentException(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s", attribute "%s" in "%s". Check your YAML syntax.', $id, $name, $attribute, $file));
-                }
-            }
+            $this->validateAttributes(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s", attribute "%s" in "%s". Check your YAML syntax.', $id, $name, '%s', $file), $tag);
 
             $definition->addTag($name, $tag);
         }
@@ -948,4 +940,15 @@ private function checkDefinition(string $id, array $definition, string $file)
             }
         }
     }
+
+    private function validateAttributes(string $message, array $attributes, string $prefix = ''): void
+    {
+        foreach ($attributes as $attribute => $value) {
+            if (\is_array($value)) {
+                $this->validateAttributes($message, $value, $attribute.'.');
+            } elseif (!\is_scalar($value ?? '')) {
+                throw new InvalidArgumentException(sprintf($message, $prefix.$attribute));
+            }
+        }
+    }
 }
diff --git a/Loader/schema/dic/services/services-1.0.xsd b/Loader/schema/dic/services/services-1.0.xsd
@@ -219,12 +219,11 @@
     <xsd:attribute name="public" type="boolean" />
   </xsd:complexType>
 
-  <xsd:complexType name="tag">
-    <xsd:simpleContent>
-      <xsd:extension base="xsd:string">
-        <xsd:anyAttribute namespace="##any" processContents="lax" />
-      </xsd:extension>
-    </xsd:simpleContent>
+  <xsd:complexType name="tag" mixed="true">
+    <xsd:choice minOccurs="0">
+      <xsd:element name="attribute" type="tag_attribute" maxOccurs="unbounded"/>
+    </xsd:choice>
+    <xsd:anyAttribute namespace="##any" processContents="lax" />
   </xsd:complexType>
 
   <xsd:complexType name="deprecated">
@@ -236,6 +235,13 @@
     </xsd:simpleContent>
   </xsd:complexType>
 
+  <xsd:complexType name="tag_attribute" mixed="true">
+    <xsd:choice minOccurs="0">
+      <xsd:element name="attribute" type="tag_attribute" maxOccurs="unbounded" />
+    </xsd:choice>
+    <xsd:attribute name="name" type="xsd:string" use="required" />
+  </xsd:complexType>
+
   <xsd:complexType name="parameters">
     <xsd:choice minOccurs="1" maxOccurs="unbounded">
       <xsd:element name="parameter" type="parameter" />
diff --git a/Tests/Dumper/XmlDumperTest.php b/Tests/Dumper/XmlDumperTest.php
@@ -288,4 +288,12 @@ public function testDumpServiceWithAbstractArgument()
         $dumper = new XmlDumper($container);
         $this->assertStringEqualsFile(self::$fixturesPath.'/xml/services_with_abstract_argument.xml', $dumper->dump());
     }
+
+    public function testDumpNonScalarTags()
+    {
+        $container = include self::$fixturesPath.'/containers/container_non_scalar_tags.php';
+        $dumper = new XmlDumper($container);
+
+        $this->assertEquals(file_get_contents(self::$fixturesPath.'/xml/services_with_array_tags.xml'), $dumper->dump());
+    }
 }
diff --git a/Tests/Dumper/YamlDumperTest.php b/Tests/Dumper/YamlDumperTest.php
@@ -173,6 +173,14 @@ public function testDumpServiceWithAbstractArgument()
         $this->assertStringEqualsFile(self::$fixturesPath.'/yaml/services_with_abstract_argument.yml', $dumper->dump());
     }
 
+    public function testDumpNonScalarTags()
+    {
+        $container = include self::$fixturesPath.'/containers/container_non_scalar_tags.php';
+        $dumper = new YamlDumper($container);
+
+        $this->assertEquals(file_get_contents(self::$fixturesPath.'/yaml/services_with_array_tags.yml'), $dumper->dump());
+    }
+
     private function assertEqualYamlStructure(string $expected, string $yaml, string $message = '')
     {
         $parser = new Parser();
diff --git a/Tests/Fixtures/containers/container_non_scalar_tags.php b/Tests/Fixtures/containers/container_non_scalar_tags.php
@@ -0,0 +1,20 @@
+<?php
+
+require_once __DIR__.'/../includes/classes.php';
+require_once __DIR__.'/../includes/foo.php';
+
+use Bar\FooClass;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+
+$container = new ContainerBuilder();
+$container
+    ->register('foo', FooClass::class)
+    ->addTag('foo_tag', [
+        'foo' => 'bar',
+        'bar' => [
+            'foo' => 'bar',
+            'bar' => 'foo'
+        ]])
+;
+
+return $container;
diff --git a/Tests/Fixtures/xml/services_with_array_tags.xml b/Tests/Fixtures/xml/services_with_array_tags.xml
@@ -0,0 +1,15 @@
+<?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>
+    <service id="service_container" class="Symfony\Component\DependencyInjection\ContainerInterface" public="true" synthetic="true"/>
+    <service id="foo" class="Bar\FooClass">
+      <tag name="foo_tag">
+        <attribute name="foo">bar</attribute>
+        <attribute name="bar">
+          <attribute name="foo">bar</attribute>
+          <attribute name="bar">foo</attribute>
+        </attribute>
+      </tag>
+    </service>
+  </services>
+</container>
diff --git a/Tests/Fixtures/yaml/services_with_array_tags.yml b/Tests/Fixtures/yaml/services_with_array_tags.yml
@@ -0,0 +1,10 @@
+
+services:
+    service_container:
+        class: Symfony\Component\DependencyInjection\ContainerInterface
+        public: true
+        synthetic: true
+    foo:
+        class: Bar\FooClass
+        tags:
+            - foo_tag: { foo: bar, bar: { foo: bar, bar: foo } }
diff --git a/Tests/Fixtures/yaml/tag_array_arguments.yml b/Tests/Fixtures/yaml/tag_array_arguments.yml
@@ -2,5 +2,5 @@ services:
     foo_service:
         class:    FooClass
         tags:
-          # tag-attribute is not a scalar
+          # tag-attribute is an array
           - { name: foo, bar: { foo: foo, bar: bar } }
diff --git a/Tests/Loader/XmlFileLoaderTest.php b/Tests/Loader/XmlFileLoaderTest.php
@@ -426,6 +426,15 @@ public function testParseServiceClosure()
         $this->assertEquals(new ServiceClosureArgument(new Reference('bar', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)), $container->getDefinition('foo')->getArgument(0));
     }
 
+    public function testParseServiceTagsWithArrayAttributes()
+    {
+        $container = new ContainerBuilder();
+        $loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml'));
+        $loader->load('services_with_array_tags.xml');
+
+        $this->assertEquals(['foo_tag' => [['foo' => 'bar', 'bar' => ['foo' => 'bar', 'bar' => 'foo']]]], $container->getDefinition('foo')->getTags());
+    }
+
     public function testParseTagsWithoutNameThrowsException()
     {
         $this->expectException(InvalidArgumentException::class);
diff --git a/Tests/Loader/YamlFileLoaderTest.php b/Tests/Loader/YamlFileLoaderTest.php
@@ -426,16 +426,14 @@ public function testNameOnlyTagsAreAllowedAsString()
         $this->assertCount(1, $container->getDefinition('foo_service')->getTag('foo'));
     }
 
-    public function testTagWithAttributeArrayThrowsException()
+    public function testTagWithAttributeArray()
     {
-        $loader = new YamlFileLoader(new ContainerBuilder(), new FileLocator(self::$fixturesPath.'/yaml'));
-        try {
-            $loader->load('badtag3.yml');
-            $this->fail('->load() should throw an exception when a tag-attribute is not a scalar');
-        } catch (\Exception $e) {
-            $this->assertInstanceOf(InvalidArgumentException::class, $e, '->load() throws an InvalidArgumentException if a tag-attribute is not a scalar');
-            $this->assertStringStartsWith('A "tags" attribute must be of a scalar-type for service "foo_service", tag "foo", attribute "bar"', $e->getMessage(), '->load() throws an InvalidArgumentException if a tag-attribute is not a scalar');
-        }
+        $container = new ContainerBuilder();
+        $loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
+        $loader->load('tag_array_arguments.yml');
+
+        $definition = $container->getDefinition('foo_service');
+        $this->assertEquals(['foo' => [['bar' => ['foo' => 'foo', 'bar' => 'bar']]]], $definition->getTags());
     }
 
     public function testLoadYamlOnlyWithKeys()

Original file line number	Diff line number	Diff line change
`@@ -297,11 +297,7 @@ private function parseDefaults(array &$content, string $file): array`
`297`	`297`	`throw new InvalidArgumentException(sprintf('The tag name in "_defaults" must be a non-empty string in "%s".', $file));`
`298`	`298`	`}`
`299`	`299`
`300`		`- foreach ($tag as $attribute => $value) {`
`301`		`- if (!\is_scalar($value) && null !== $value) {`
`302`		`- throw new InvalidArgumentException(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type in "%s". Check your YAML syntax.', $name, $attribute, $file));`
`303`		`- }`
`304`		`- }`
	`300`	`+ $this->validateAttributes(sprintf('Tag "%s", attribute "%s" in "_defaults" must be of a scalar-type in "%s". Check your YAML syntax.', $name, '%s', $file), $tag);`
`305`	`301`	`}`
`306`	`302`	`}`
`307`	`303`
`@@ -605,11 +601,7 @@ private function parseDefinition(string $id, array\|string\|null $service, string`
`605`	`601`	`throw new InvalidArgumentException(sprintf('The tag name for service "%s" in "%s" must be a non-empty string.', $id, $file));`
`606`	`602`	`}`
`607`	`603`
`608`		`- foreach ($tag as $attribute => $value) {`
`609`		`- if (!\is_scalar($value) && null !== $value) {`
`610`		`- throw new InvalidArgumentException(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s", attribute "%s" in "%s". Check your YAML syntax.', $id, $name, $attribute, $file));`
`611`		`- }`
`612`		`- }`
	`604`	`+ $this->validateAttributes(sprintf('A "tags" attribute must be of a scalar-type for service "%s", tag "%s", attribute "%s" in "%s". Check your YAML syntax.', $id, $name, '%s', $file), $tag);`
`613`	`605`
`614`	`606`	`$definition->addTag($name, $tag);`
`615`	`607`	`}`
`@@ -948,4 +940,15 @@ private function checkDefinition(string $id, array $definition, string $file)`
`948`	`940`	`}`
`949`	`941`	`}`
`950`	`942`	`}`
	`943`	`+`
	`944`	`+ private function validateAttributes(string $message, array $attributes, string $prefix = ''): void`
	`945`	`+ {`
	`946`	`+ foreach ($attributes as $attribute => $value) {`
	`947`	`+ if (\is_array($value)) {`
	`948`	`+ $this->validateAttributes($message, $value, $attribute.'.');`
	`949`	`+ } elseif (!\is_scalar($value ?? '')) {`
	`950`	`+ throw new InvalidArgumentException(sprintf($message, $prefix.$attribute));`
	`951`	`+ }`
	`952`	`+ }`
	`953`	`+ }`
`951`	`954`	`}`
Original file line number	Diff line number	Diff line change
`@@ -288,4 +288,12 @@ public function testDumpServiceWithAbstractArgument()`
`288`	`288`	`$dumper = new XmlDumper($container);`
`289`	`289`	`$this->assertStringEqualsFile(self::$fixturesPath.'/xml/services_with_abstract_argument.xml', $dumper->dump());`
`290`	`290`	`}`
	`291`	`+`
	`292`	`+ public function testDumpNonScalarTags()`
	`293`	`+ {`
	`294`	`+ $container = include self::$fixturesPath.'/containers/container_non_scalar_tags.php';`
	`295`	`+ $dumper = new XmlDumper($container);`
	`296`	`+`
	`297`	`+ $this->assertEquals(file_get_contents(self::$fixturesPath.'/xml/services_with_array_tags.xml'), $dumper->dump());`
	`298`	`+ }`
`291`	`299`	`}`
Original file line number	Diff line number	Diff line change
`@@ -173,6 +173,14 @@ public function testDumpServiceWithAbstractArgument()`
`173`	`173`	`$this->assertStringEqualsFile(self::$fixturesPath.'/yaml/services_with_abstract_argument.yml', $dumper->dump());`
`174`	`174`	`}`
`175`	`175`
	`176`	`+ public function testDumpNonScalarTags()`
	`177`	`+ {`
	`178`	`+ $container = include self::$fixturesPath.'/containers/container_non_scalar_tags.php';`
	`179`	`+ $dumper = new YamlDumper($container);`
	`180`	`+`
	`181`	`+ $this->assertEquals(file_get_contents(self::$fixturesPath.'/yaml/services_with_array_tags.yml'), $dumper->dump());`
	`182`	`+ }`
	`183`	`+`
`176`	`184`	`private function assertEqualYamlStructure(string $expected, string $yaml, string $message = '')`
`177`	`185`	`{`
`178`	`186`	`$parser = new Parser();`