Skip to content

[DependencyInjection] Allow array attributes for service tags #38540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

[DependencyInjection] Allow array attributes for service tags #38540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8494c7c
Allow array attributes for service tags
aschempp Oct 13, 2020
c9ee2dd
Rename methods and arguments
aschempp Jul 14, 2021
3d7d4f8
Fixed recursion and added tests for YamlFileLoader
aschempp Jul 14, 2021
aeb2ce1
Fixed XMLFileLoader and XSD and added test
aschempp Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/Symfony/Component/DependencyInjection/Loader/Configurator/DefaultsConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,7 @@ final public function tag(string $name, array $attributes = []): self
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);

Expand All @@ -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, $attributes, $attribute.'.');
} elseif (!is_scalar($value) && null !== $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));
}
}
}
}
17 changes: 12 additions & 5 deletions src/Symfony/Component/DependencyInjection/Loader/Configurator/Traits/TagTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,21 @@ final public function tag(string $name, array $attributes = []): self
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, $attributes, $attribute.'.');
} elseif (!is_scalar($value) && null !== $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));
}
}
}
}
39 changes: 32 additions & 7 deletions src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,14 @@ private function parseDefinition(\DOMElement $service, string $file, Definition
$tags = $this->getChildren($service, 'tag');

foreach ($tags as $tag) {
$parameters = [];
$tagName = $tag->nodeValue;
$tagName = $tag->hasChildNodes() || '' === $tag->nodeValue ? $tag->getAttribute('name') : $tag->nodeValue;
if ('' === $tagName) {
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something is missing here, that would allow removing this foreach. Right now, this looks like duplication with the new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The foreach loop is to handle attributes on the tag itself, the method handles nested arguments.

Example in the initial comment:

<?xml version="1.0" ?>
<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="foo" class="BarClass" public="true">
        <tag name="foo_tag" some-option="cat">
            <attribute name="array_option">
                <attribute name="key1">lorem</attribute>
                <attribute name="key2">ipsum</attribute>
            </attribute>
        </tag>
    </service>
  </services>
</container>

The foreach would read some-option while the method recursively reads <atttribute …>

if ('name' === $name && '' === $tagName) {
if ('name' === $name) {
continue;
}

Expand All @@ -349,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.', (string) $service->getAttribute('id'), $file));
}

$definition->addTag($tagName, $parameters);
}

Expand Down Expand Up @@ -585,6 +585,31 @@ 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) {
$name = $childNode->getAttribute('name');
if ('' === $name) {
throw new InvalidArgumentException($missingName);
}

if ($this->getChildren($childNode, 'attribute')) {
$parameters[$name] = $this->getTagAttributes($childNode, $missingName);
} else {
if (false !== strpos($name, '-') && false === strpos($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.
*
Expand Down
23 changes: 13 additions & 10 deletions src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,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);
}
}

Expand Down Expand Up @@ -614,11 +610,7 @@ private function parseDefinition(string $id, $service, string $file, array $defa
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);
}
Expand Down Expand Up @@ -960,4 +952,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) && null !== $value) {
throw new InvalidArgumentException(sprintf($message, $prefix.$attribute));
}
}
}
}
18 changes: 12 additions & 6 deletions src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,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">
Expand All @@ -226,6 +225,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" />
Expand Down
14 changes: 14 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services_with_array_tags.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?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="foo" class="Foo">
<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>
2 changes: 1 addition & 1 deletion ...Injection/Tests/Fixtures/yaml/badtag3.yml → ...sts/Fixtures/yaml/tag_array_arguments.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 } }
9 changes: 9 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,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);
Expand Down
16 changes: 7 additions & 9 deletions src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,16 +433,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()
Expand Down