[DependencyInjection] Allow array attributes for service tags #38540

aschempp · 2020-10-13T06:36:26Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #38339
License	MIT
Doc PR	symfony/symfony-docs#...

This adds array attribute support for container service tags as described in #38339. I'm not sure I catched all places this would apply. The most notable change is the additions to the XML schema for services, because that's the current limitation for not using array.

I created this as draft PR because I'm very well aware the docs and tests need to be update. But please let me know if this sounds like a valid feature to add and if the approach seems reasonable or if I missed something 🙃

With this PR, you can now define service tags like this:

PHP:

$container->services()
        ->set('foo', BarClass::class)
            ->public()
            ->tag(
                'foo_tag', 
                [
                    'some_option' => 'cat', 
                    'array_option' => [
                        'key1' => 'lorem', 
                        'key2' => 'ipsum'
                    ]
                ]
            )
;

XML:

<?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>

YAML:

services:
    foo:
        class: BarClass
        tags:
            - { name: 'foo_tag', 'some-option': 'cat', array_option: [key1: lorem, key2: ipsum] }

As described in #38339, our service definition might look like this now:

services:
    Vendor\Bundle\Controller\FooController:
        public: true
        tags:
            - { name: 'contao.page', path: 'foo/{id}', requirements: [ id: '\d+' ] }

aschempp · 2020-10-28T06:10:39Z

any update/feedback/opinions on this PR? 🙃

nicolas-grekas

I like it. What's missing:

corresponding updates to the dumpers
tests

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

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

nicolas-grekas · 2021-01-07T11:13:50Z

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

+                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->getRecursiveTagAttributes($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) {


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

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 …>

nicolas-grekas · 2021-02-11T10:24:36Z

Friendly ping @aschempp

aschempp · 2021-02-17T17:02:44Z

Sorry I didn't have time yet to update this. I'll give it a try in the coming weeks.

aschempp · 2021-07-15T07:37:01Z

I've updated the PR, added some text and fixed major bugs 🙈
The failing PHP8 tests seem unrelated to me.

aschempp · 2021-10-06T14:37:57Z

Friendly ping @nicolas-grekas 😅

nicolas-grekas · 2021-10-15T09:21:41Z

How do the dumpers behave with array as tag attributes? Can you please add some tests for that?

aschempp · 2021-11-05T07:47:46Z

I'm sorry I might not be familiar with that concept. What "Dumpers" are you referring to?

ruudk · 2021-11-06T13:45:04Z

First of all, thank you for working on this feature. It's something that I really struggle with myself and I hope this will be possible one day 🙏

To answer your question, @nicolas-grekas talks about dumpers like Symfony\Component\DependencyInjection\Dumper\XmlDumper. They are used to dump the compiled container to the var/cache directory. (If you would try this on a real application/project you would see an error too)

As far as I can see your PR only makes it possible to have array tags in configurations (read side). But the PR should also fix the write side.

To solve that, add a test case to Symfony\Component\DependencyInjection\Tests\Dumper\XmlDumperTest:

     public function testNonScalarTags()
    {
        $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());
    }

and create a fixture like this:

<?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',
            'baz' => 'foo'
        ]])
;

return $container;

When you run it, you'll see that the Dumper breaks:

1) Symfony\Component\DependencyInjection\Tests\Dumper\XmlDumperTest::testNonScalarTags
TypeError: DOMElement::setAttribute(): Argument #2 ($value) must be of type string, array given

/Users/Ruud/opensource/symfony/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php:147
/Users/Ruud/opensource/symfony/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php:272

That foreach should be altered to support arrays and write the XML in the new format.

Hope it helps 😊

nicolas-grekas · 2022-04-02T13:56:24Z

@aschempp up to finish this PR?

nicolas-grekas · 2022-07-28T08:59:00Z

I'm closing here because this staled and also because on second though I'm not sure it will work smoothly. Also because re-reading the original issue, turning tags into annotations doesn't feel like a good idea to me, at least not without using the autoconfiguration mechanism. registerAttributeForAutoconfiguration() provides a way to do that natively. This provides a hook that would allow serializing complex structures found in the attribute into eg json, so that the description can fit a tag.

Closing as such, thanks for proposing!

aschempp · 2022-07-28T12:38:47Z

Sorry for being stale, I was just about to update that (started yesterday after finishing #42355). I just updated the PR but it seems GH does not update here if the PR is closed.

Not exactly sure what you mean by turning tags into annotations. The change simply allows for recursive tag attributes. A compiler pass can then use this information to do whatever they want.
In our case, we have tags to define a service as a controller, and the route configuration has a name, path and an array of requirements etc., hence the requirement for an array of tag attributes.

nicolas-grekas · 2022-07-28T12:42:18Z

I meant turning annotations into tags sorry (with this).
How do you do right now? Wouldn't my suggestion above work for you?

aschempp · 2022-08-03T05:48:18Z

I meant turning annotations into tags sorry (with this).

Yeah that's basically a predecessor of the PHP8 attributes we've had for a few years now 😎 (although it is obviously deprecated now that we have attributes)

How do you do right now? Wouldn't my suggestion above work for you?

If I remember correctly, both using our annotations as well as our newly added attributes works fine. Even though attributes/annotations are converted to tags, there is no validation on them not being arrays. It just does not work when trying to define the same in YAML or XML.

nicolas-grekas · 2022-08-03T06:47:44Z

Even though attributes/annotations are converted to tags, there is no validation on them not being arrays. It just does not work when trying to define the same in YAML or XML.

Oh, and how does the XML dump look like?

I can't reopen but please resubmit if you want.

aschempp · 2022-08-08T16:13:26Z

XML will look like this 😊
https://github.com/symfony/symfony/pull/38540/files#diff-16371727bd7f17a541f80dec3603214facfb14f0848930e8567c54c0a06eae8b

aschempp · 2022-08-23T09:14:29Z

see #47364 for the reopened PR

…ce tags (aschempp) 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 #38339, #38540 | License | MIT | Doc PR | symfony/symfony-docs#...  This adds array attribute support for container service tags as described in #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 #38540 as new PR since the other one was ("accidentially") closed 😊 /cc `@nicolas`-grekas Commits ------- edd8d77 [DependencyInjection] Allow array attributes for service tags

…ce tags (aschempp) 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

carsonbot added Status: Needs Review Feature labels Oct 13, 2020

aschempp mentioned this pull request Oct 13, 2020

Support for non-scalar tag attributes #38339

Closed

nicolas-grekas added this to the 5.x milestone Oct 13, 2020

derrabus added the DependencyInjection label Oct 21, 2020

nicolas-grekas mentioned this pull request Jan 6, 2021

[Console] Add LazyCommandTrait to help writing lazy commands #39726

Closed

nicolas-grekas reviewed Jan 7, 2021

View reviewed changes

aschempp added 4 commits July 15, 2021 09:25

Allow array attributes for service tags

8494c7c

Rename methods and arguments

c9ee2dd

Fixed recursion and added tests for YamlFileLoader

3d7d4f8

Fixed XMLFileLoader and XSD and added test

aeb2ce1

aschempp force-pushed the feature/tag-array-attributes branch from 24eae57 to aeb2ce1 Compare July 15, 2021 07:26

aschempp marked this pull request as ready for review July 15, 2021 07:36

nicolas-grekas added DependencyInjection and removed DependencyInjection labels Oct 14, 2021

carsonbot changed the title ~~Allow array attributes for service tags~~ [DependencyInjection] Allow array attributes for service tags Oct 14, 2021

nicolas-grekas modified the milestones: 5.4, 6.1 Nov 12, 2021

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas closed this Jul 28, 2022

aschempp mentioned this pull request Aug 23, 2022

[DependencyInjection] Allow array attributes for service tags #47364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Allow array attributes for service tags #38540

[DependencyInjection] Allow array attributes for service tags #38540

aschempp commented Oct 13, 2020

aschempp commented Oct 28, 2020

nicolas-grekas left a comment

nicolas-grekas Jan 7, 2021

aschempp Jul 14, 2021

nicolas-grekas commented Feb 11, 2021

aschempp commented Feb 17, 2021

aschempp commented Jul 15, 2021

aschempp commented Oct 6, 2021

nicolas-grekas commented Oct 15, 2021

aschempp commented Nov 5, 2021

ruudk commented Nov 6, 2021 •

edited

Loading

nicolas-grekas commented Apr 2, 2022

nicolas-grekas commented Jul 28, 2022

aschempp commented Jul 28, 2022

nicolas-grekas commented Jul 28, 2022

aschempp commented Aug 3, 2022

nicolas-grekas commented Aug 3, 2022

aschempp commented Aug 8, 2022

aschempp commented Aug 23, 2022

[DependencyInjection] Allow array attributes for service tags #38540

[DependencyInjection] Allow array attributes for service tags #38540

Conversation

aschempp commented Oct 13, 2020

aschempp commented Oct 28, 2020

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Jan 7, 2021

Choose a reason for hiding this comment

aschempp Jul 14, 2021

Choose a reason for hiding this comment

nicolas-grekas commented Feb 11, 2021

aschempp commented Feb 17, 2021

aschempp commented Jul 15, 2021

aschempp commented Oct 6, 2021

nicolas-grekas commented Oct 15, 2021

aschempp commented Nov 5, 2021

ruudk commented Nov 6, 2021 • edited Loading

nicolas-grekas commented Apr 2, 2022

nicolas-grekas commented Jul 28, 2022

aschempp commented Jul 28, 2022

nicolas-grekas commented Jul 28, 2022

aschempp commented Aug 3, 2022

nicolas-grekas commented Aug 3, 2022

aschempp commented Aug 8, 2022

aschempp commented Aug 23, 2022

ruudk commented Nov 6, 2021 •

edited

Loading