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.

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

[DI] Reference tagged services in config #22200

Merged
merged 1 commit into from Sep 28, 2017

Conversation

@ro0NL
Contributor

ro0NL commented Mar 28, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12269
License MIT
Doc PR symfony/symfony-docs#8404

This is a proof of concept to reference a sequence of tagged services.

The problem bugs me for some time, and at first i thought the solution was to have some super generic compiler pass. If it could replace a lot of compilers in core.. perhaps worth it, but eventually each tag comes with it's own logic, including how to deal with tag attributes.

However, writing the passes over and over again becomes tedious for the most basic usecase. So given the recent developments, this idea came to mind.

services:
    a:
        class: stdClass
        properties: { a: true }
        tags: [foo]

    b:
        class: stdClass
        properties: { b: true }
        tags: [foo]

    c:
        class: stdClass
        properties:
            #stds: !tagged foo (see #22198)
            stds: !tagged
                foo
dump(iterator_to_array($this->get('c')->stds));
array:2 [▼
  0 => {#5052 ▼
    +"a": true
  }
  1 => {#4667 ▼
    +"b": true
  }
]

Given the basic example at https://symfony.com/doc/current/service_container/tags.html, this could replace that.

Any thoughts?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Mar 28, 2017

Member

you don't have a XML syntax for this

Member

stof commented Mar 28, 2017

you don't have a XML syntax for this

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Mar 28, 2017

Contributor

Also no tests.. it's a proof of concept :) but will do that.. wanted to propose it first.

Perhaps introduces a few things to discuss...

  • may require tag attributes to be blank dont think this is needed as were only interested in collecting services at this point
  • could support the priority attribute built in
  • could leverage expression language for basic atribute support (in terms of filtering)
  • may require something that does iterator_to_array to satisfy any array constraints not related to this PR

I think the latter is actually an important one...

Contributor

ro0NL commented Mar 28, 2017

Also no tests.. it's a proof of concept :) but will do that.. wanted to propose it first.

Perhaps introduces a few things to discuss...

  • may require tag attributes to be blank dont think this is needed as were only interested in collecting services at this point
  • could support the priority attribute built in
  • could leverage expression language for basic atribute support (in terms of filtering)
  • may require something that does iterator_to_array to satisfy any array constraints not related to this PR

I think the latter is actually an important one...

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Mar 29, 2017

Contributor

Tagged scalars won't be supported in yaml before 4.0. So you can either use a different syntax, or you can whitelist just one tag using:

diff --git a/src/Symfony/Component/Yaml/Inline.php b/src/Symfony/Component/Yaml/Inline.php
index e8d5faa..82112f0 100644
--- a/src/Symfony/Component/Yaml/Inline.php
+++ b/src/Symfony/Component/Yaml/Inline.php
@@ -710,8 +710,10 @@ class Inline
 
         // Is followed by a scalar
         if (!isset($value[$nextOffset]) || !in_array($value[$nextOffset], array('[', '{'), true)) {
-            // Manage scalars in {@link self::evaluateScalar()}
-            return;
+            if (!in_array($tag, $whitelist = array('foo'))) {
+                // Manage scalars in {@link self::evaluateScalar()}
+                return;
+            }
         }
 
         // Built-in tags
diff --git a/src/Symfony/Component/Yaml/Parser.php b/src/Symfony/Component/Yaml/Parser.php
index 7b572e1..bbb1531 100644
--- a/src/Symfony/Component/Yaml/Parser.php
+++ b/src/Symfony/Component/Yaml/Parser.php
@@ -642,6 +642,8 @@ class Parser
             if ('' !== $matches['tag']) {
                 if ('!!binary' === $matches['tag']) {
                     return Inline::evaluateBinaryScalar($data);
+                } elseif (in_array($matches['tag'], $whitelist = array('!foo'))) {
+                    return new TaggedValue(substr($matches['tag'], 1), $data);
                 } elseif ('!' !== $matches['tag']) {
                     @trigger_error(sprintf('Using the custom tag "%s" for the value "%s" is deprecated since version 3.3. It will be replaced by an instance of %s in 4.0.', $matches['tag'], $data, TaggedValue::class), E_USER_DEPRECATED);
                 }
Contributor

GuilhemN commented Mar 29, 2017

Tagged scalars won't be supported in yaml before 4.0. So you can either use a different syntax, or you can whitelist just one tag using:

diff --git a/src/Symfony/Component/Yaml/Inline.php b/src/Symfony/Component/Yaml/Inline.php
index e8d5faa..82112f0 100644
--- a/src/Symfony/Component/Yaml/Inline.php
+++ b/src/Symfony/Component/Yaml/Inline.php
@@ -710,8 +710,10 @@ class Inline
 
         // Is followed by a scalar
         if (!isset($value[$nextOffset]) || !in_array($value[$nextOffset], array('[', '{'), true)) {
-            // Manage scalars in {@link self::evaluateScalar()}
-            return;
+            if (!in_array($tag, $whitelist = array('foo'))) {
+                // Manage scalars in {@link self::evaluateScalar()}
+                return;
+            }
         }
 
         // Built-in tags
diff --git a/src/Symfony/Component/Yaml/Parser.php b/src/Symfony/Component/Yaml/Parser.php
index 7b572e1..bbb1531 100644
--- a/src/Symfony/Component/Yaml/Parser.php
+++ b/src/Symfony/Component/Yaml/Parser.php
@@ -642,6 +642,8 @@ class Parser
             if ('' !== $matches['tag']) {
                 if ('!!binary' === $matches['tag']) {
                     return Inline::evaluateBinaryScalar($data);
+                } elseif (in_array($matches['tag'], $whitelist = array('!foo'))) {
+                    return new TaggedValue(substr($matches['tag'], 1), $data);
                 } elseif ('!' !== $matches['tag']) {
                     @trigger_error(sprintf('Using the custom tag "%s" for the value "%s" is deprecated since version 3.3. It will be replaced by an instance of %s in 4.0.', $matches['tag'], $data, TaggedValue::class), E_USER_DEPRECATED);
                 }
@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Mar 29, 2017

Contributor

For XML, the syntax could be:

<service>
    <argument type="tagged-services" tag="foo" />
</service>
Contributor

GuilhemN commented Mar 29, 2017

For XML, the syntax could be:

<service>
    <argument type="tagged-services" tag="foo" />
</service>
@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Mar 29, 2017

Contributor

Above works currently (tagged scalar on a new line), so the syntax only improves as of 4.0.

perhaps to compromise it could allow !tagged_services [tagN, ...] as well.. to benefit from the allowed oneliner :) and multiple tag support of course.

Contributor

ro0NL commented Mar 29, 2017

Above works currently (tagged scalar on a new line), so the syntax only improves as of 4.0.

perhaps to compromise it could allow !tagged_services [tagN, ...] as well.. to benefit from the allowed oneliner :) and multiple tag support of course.

@ro0NL ro0NL changed the base branch from master to 3.4 Jul 13, 2017

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 13, 2017

Contributor

Little update from my side;

  • renamed to TaggedIteratorArgument
  • yaml support !tagged_iterator tag-name
  • xml support <argument/property type="tagged_iterator" tag="tag-name" />
  • order tags by priority
  • initial tests
  • no multiple tag support, no automatic to array expansion

Open for any cosmetic changes / better naming etc :)

Contributor

ro0NL commented Jul 13, 2017

Little update from my side;

  • renamed to TaggedIteratorArgument
  • yaml support !tagged_iterator tag-name
  • xml support <argument/property type="tagged_iterator" tag="tag-name" />
  • order tags by priority
  • initial tests
  • no multiple tag support, no automatic to array expansion

Open for any cosmetic changes / better naming etc :)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 13, 2017

Member

Could that be used to replace any existing passes? If yes, that'd be a good sign of the usefulness of it and should be done in the same PR if possible.

Member

nicolas-grekas commented Jul 13, 2017

Could that be used to replace any existing passes? If yes, that'd be a good sign of the usefulness of it and should be done in the same PR if possible.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 13, 2017

Contributor

Technically yes. Now the priority attribute is handled it could replace some 🤞

Will look into that.

edit: from https://symfony.com/doc/current/service_container/tags.html looks like twig.extension is a candidate...

Contributor

ro0NL commented Jul 13, 2017

Technically yes. Now the priority attribute is handled it could replace some 🤞

Will look into that.

edit: from https://symfony.com/doc/current/service_container/tags.html looks like twig.extension is a candidate...

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 13, 2017

Contributor

Furthermore if we ever allow to map a !iterator against an adder it could benefit from that by passing along tag attributes. Making everything available in runtime (sounds cool at least :))

Contributor

ro0NL commented Jul 13, 2017

Furthermore if we ever allow to map a !iterator against an adder it could benefit from that by passing along tag attributes. Making everything available in runtime (sounds cool at least :))

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 15, 2017

Contributor

@nicolas-grekas would it make sense to merge ServiceLocator and RewindableGenerator? Making IteratorArgument obsolete.. as we have something (ServiceLocatorArgument probably) that can be used for almost anything, i.e. typehints against iterable or ContainerInterface or array (with #22204).

That would end the iterator vs map thingy. And allows this PR to move forward with a single !tagged_service_locator again :) (we should expose !service_locator as well - #22649 (comment), and for #23454 it can be !service_closure).

Could that be used to replace any existing passes? If yes, that'd be a good sign of the usefulness of it and should be done in the same PR if possible.

I guess the problem is most rely on array API, so the auto expansion could actually help here to satisfy those and have easier migration.

Also thinks like https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml#L59 can benefit i guess.

Contributor

ro0NL commented Jul 15, 2017

@nicolas-grekas would it make sense to merge ServiceLocator and RewindableGenerator? Making IteratorArgument obsolete.. as we have something (ServiceLocatorArgument probably) that can be used for almost anything, i.e. typehints against iterable or ContainerInterface or array (with #22204).

That would end the iterator vs map thingy. And allows this PR to move forward with a single !tagged_service_locator again :) (we should expose !service_locator as well - #22649 (comment), and for #23454 it can be !service_closure).

Could that be used to replace any existing passes? If yes, that'd be a good sign of the usefulness of it and should be done in the same PR if possible.

I guess the problem is most rely on array API, so the auto expansion could actually help here to satisfy those and have easier migration.

Also thinks like https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml#L59 can benefit i guess.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 15, 2017

Member

Dunno, we could try... :)

Member

nicolas-grekas commented Jul 15, 2017

Dunno, we could try... :)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 15, 2017

Contributor

See ro0NL@1360bab for where im leaning to.. Argument\IteratorArg could become Argument\ServiceLocArg allowing to deprecate DI\ServiceLoc. Could work out, feel free to contrib :)

Basically i dont understand why we favor

!service { arguments: { k: '@ref' }, tags: [container.service_loc] }

over (simply)

!service_locator { k: '@ref' }
!service_locator [ '@ref' ] # keys are arbitrary

What !iterator does actually :) it has way better DX, dont you think? But maybe im missing why a tag is needed.

edit: if im correct it creates a new def+alias only to be inlined again afterwards.. lost me :)

Contributor

ro0NL commented Jul 15, 2017

See ro0NL@1360bab for where im leaning to.. Argument\IteratorArg could become Argument\ServiceLocArg allowing to deprecate DI\ServiceLoc. Could work out, feel free to contrib :)

Basically i dont understand why we favor

!service { arguments: { k: '@ref' }, tags: [container.service_loc] }

over (simply)

!service_locator { k: '@ref' }
!service_locator [ '@ref' ] # keys are arbitrary

What !iterator does actually :) it has way better DX, dont you think? But maybe im missing why a tag is needed.

edit: if im correct it creates a new def+alias only to be inlined again afterwards.. lost me :)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 18, 2017

Contributor

@nicolas-grekas final proposal; im leaning to roll out my own yaml tag(s) otherwise.

Looking at my own usecases i could definitely use a tagged iterator as well as a tagged service locator *.

We can keep leveraging iterator arg and simply dont support tagged service locators, but to me thats half the feature. Not sure if worth it then.

So we could do !tagged_services tag-name; giving you an iterator. Or !tagged_services {tag: tag-name, index_by: id|class|tag-attr }; giving you a service locator.

*) Problem with tagged service locator is we cant get the full map, it's not iterable nor exposes getKeys. Which may be limiting, and therefor im not sure whats best way to introduce it in core; hence im leaning to roll out my own and keep things as is in core.

Contributor

ro0NL commented Jul 18, 2017

@nicolas-grekas final proposal; im leaning to roll out my own yaml tag(s) otherwise.

Looking at my own usecases i could definitely use a tagged iterator as well as a tagged service locator *.

We can keep leveraging iterator arg and simply dont support tagged service locators, but to me thats half the feature. Not sure if worth it then.

So we could do !tagged_services tag-name; giving you an iterator. Or !tagged_services {tag: tag-name, index_by: id|class|tag-attr }; giving you a service locator.

*) Problem with tagged service locator is we cant get the full map, it's not iterable nor exposes getKeys. Which may be limiting, and therefor im not sure whats best way to introduce it in core; hence im leaning to roll out my own and keep things as is in core.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 18, 2017

Member

If you know the name of the tagged services, you don't need to tag them.
And of you don't know their name, you cannot "get" them from the service locator.
Which means to me only an iterator is useful... esp.since if you have an iterator, you can easily turn that into a map with iterator_to_array.
I also prefer tagged_services instead of tagged_iterator.
You did not deprecate any existing compiler pass, does that mean none can be replaced with this generic implem?
What's missing?

Member

nicolas-grekas commented Jul 18, 2017

If you know the name of the tagged services, you don't need to tag them.
And of you don't know their name, you cannot "get" them from the service locator.
Which means to me only an iterator is useful... esp.since if you have an iterator, you can easily turn that into a map with iterator_to_array.
I also prefer tagged_services instead of tagged_iterator.
You did not deprecate any existing compiler pass, does that mean none can be replaced with this generic implem?
What's missing?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 18, 2017

Contributor

If you know the name of the tagged services, you don't need to tag them.
And of you don't know their name, you cannot "get" them from the service locator.

Yes, so true :) we dont need this 👍

if you have an iterator, you can easily turn that into a map with iterator_to_array.

But there's no key control, you'll just get numeric indexes. But lets forget about maps here ;-)

So, as is, we can at least cover twig.extension, kernel.cache_clear for sure. But we need to update some APIs from iterable to array, that be the cleanest. Or apply iterator_to_array out-of-the-box based on typehint; seems pragmatic but im not sure (well.. i was :P).. wdyt?

Tags like security.voter are a bit more difficult. And things like commands cannot even be done this way i guess. So it needs a per case decision.

For end users it may fit many more cases though.

Contributor

ro0NL commented Jul 18, 2017

If you know the name of the tagged services, you don't need to tag them.
And of you don't know their name, you cannot "get" them from the service locator.

Yes, so true :) we dont need this 👍

if you have an iterator, you can easily turn that into a map with iterator_to_array.

But there's no key control, you'll just get numeric indexes. But lets forget about maps here ;-)

So, as is, we can at least cover twig.extension, kernel.cache_clear for sure. But we need to update some APIs from iterable to array, that be the cleanest. Or apply iterator_to_array out-of-the-box based on typehint; seems pragmatic but im not sure (well.. i was :P).. wdyt?

Tags like security.voter are a bit more difficult. And things like commands cannot even be done this way i guess. So it needs a per case decision.

For end users it may fit many more cases though.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 18, 2017

Member

apply iterator_to_array out-of-the-box based on typehint

I wouldn't recommend doing that :)

we can at least cover twig.extension, kernel.cache_clear

Let's see how it goes then

Member

nicolas-grekas commented Jul 18, 2017

apply iterator_to_array out-of-the-box based on typehint

I wouldn't recommend doing that :)

we can at least cover twig.extension, kernel.cache_clear

Let's see how it goes then

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 22, 2017

Contributor

@nicolas-grekas this is hard :) and im not sure about the best approach.

I think there are 2 viable directions, given

method($mixed, array $array, iterable $iterable, ...$variadic)

Variant A - type based:

'$mixed': !tagged-services tag # pass a lazy iterable
'$array': !tagged-services tag # pass an array
'$iterable': !tagged-services tag # pass a lazy iterable
'$variadic': !tagged-services tag # pass a service + append subsequent services as arg

Variadic for numeric arg doesnt resolve subsequent arguments, as it implies subsequent args come from config (though not sure), i.e;

- [mixed]
- [array]
- [iterable]
- !tagged-services tag # pass a lazy iterable
- x # pass scalar for subsequent variadic

Variant B - tag based:

'$mixed': !tagged-services tag # pass an array
'$array': !tagged-services tag # pass an array
'$iterable': !tagged-services tag # pass an array
'$iterable': !iterator { values: !tagged-services tag } # pass a lazy iterable
'$variadic': !variadic { values: !tagged-services tag } # pass a service + append subsequent services as arg

I guess this variant allows for the same approach between numeric and named args.

So im leaning to tag based, but it requires refactoring here and there. Though type based looks nice.. i think explicitness is the way to go.

Basically we introduce a little DSL with !iterator !tagged-services foo.

Contributor

ro0NL commented Jul 22, 2017

@nicolas-grekas this is hard :) and im not sure about the best approach.

I think there are 2 viable directions, given

method($mixed, array $array, iterable $iterable, ...$variadic)

Variant A - type based:

'$mixed': !tagged-services tag # pass a lazy iterable
'$array': !tagged-services tag # pass an array
'$iterable': !tagged-services tag # pass a lazy iterable
'$variadic': !tagged-services tag # pass a service + append subsequent services as arg

Variadic for numeric arg doesnt resolve subsequent arguments, as it implies subsequent args come from config (though not sure), i.e;

- [mixed]
- [array]
- [iterable]
- !tagged-services tag # pass a lazy iterable
- x # pass scalar for subsequent variadic

Variant B - tag based:

'$mixed': !tagged-services tag # pass an array
'$array': !tagged-services tag # pass an array
'$iterable': !tagged-services tag # pass an array
'$iterable': !iterator { values: !tagged-services tag } # pass a lazy iterable
'$variadic': !variadic { values: !tagged-services tag } # pass a service + append subsequent services as arg

I guess this variant allows for the same approach between numeric and named args.

So im leaning to tag based, but it requires refactoring here and there. Though type based looks nice.. i think explicitness is the way to go.

Basically we introduce a little DSL with !iterator !tagged-services foo.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 18, 2017

Contributor

Ready for review :) updated to tagged keyword; thus !tagged, type="tagged" and future $c->tagged(). Datatype is now iterator by design, and closes (IMHO) the map vs. collection debate, and anything related to that.

cc @GuilhemN yaml is patched, might need a trigger from DI as in YAML "tagged" is just a magic string. But works for me :)

Contributor

ro0NL commented Sep 18, 2017

Ready for review :) updated to tagged keyword; thus !tagged, type="tagged" and future $c->tagged(). Datatype is now iterator by design, and closes (IMHO) the map vs. collection debate, and anything related to that.

cc @GuilhemN yaml is patched, might need a trigger from DI as in YAML "tagged" is just a magic string. But works for me :)

@ro0NL ro0NL closed this Sep 18, 2017

@ro0NL ro0NL reopened this Sep 18, 2017

@nicolas-grekas

Straightforward, nice :)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services9.xml
Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services9.yml
@@ -774,8 +774,8 @@ private static function parseTag($value, &$i, $flags)
$nextOffset += strspn($value, ' ', $nextOffset);
// Is followed by a scalar
if (!isset($value[$nextOffset]) || !in_array($value[$nextOffset], array('[', '{'), true)) {
// Manage scalars in {@link self::evaluateScalar()}
if ((!isset($value[$nextOffset]) || !in_array($value[$nextOffset], array('[', '{'), true)) && 'tagged' !== $tag) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 18, 2017

Member

opting out from a deprecation in the specific required case (!tagged foo was a string before this, not yaml compliant so deprecated anyway)

@nicolas-grekas

nicolas-grekas Sep 18, 2017

Member

opting out from a deprecation in the specific required case (!tagged foo was a string before this, not yaml compliant so deprecated anyway)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 26, 2017

Member

@ro0NL could you rebase please?

Member

nicolas-grekas commented Sep 26, 2017

@ro0NL could you rebase please?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 26, 2017

Contributor

Done.

edit: looking at tests.

Contributor

ro0NL commented Sep 26, 2017

Done.

edit: looking at tests.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 26, 2017

Contributor

Status: needs work. Cant get tests to work.

Need some help to both fix

DependencyInjection/Tests/Loader/PhpFileLoaderTest.php --filter testConfig 
DependencyInjection/Tests/Dumper/PhpDumperTest.php --filter testAddService
Contributor

ro0NL commented Sep 26, 2017

Status: needs work. Cant get tests to work.

Need some help to both fix

DependencyInjection/Tests/Loader/PhpFileLoaderTest.php --filter testConfig 
DependencyInjection/Tests/Dumper/PhpDumperTest.php --filter testAddService
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

Now green fabbot failures are false-positives.
PR ready, still 👍 on my side.

Member

nicolas-grekas commented Sep 28, 2017

Now green fabbot failures are false-positives.
PR ready, still 👍 on my side.

@ogizanagi

👍

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 28, 2017

Member

Thank you @ro0NL.

Member

nicolas-grekas commented Sep 28, 2017

Thank you @ro0NL.

@nicolas-grekas nicolas-grekas merged commit 979e58f into symfony:3.4 Sep 28, 2017

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nicolas-grekas added a commit that referenced this pull request Sep 28, 2017

feature #22200 [DI] Reference tagged services in config (ro0NL)
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Reference tagged services in config

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12269
| License       | MIT
| Doc PR        | symfony/symfony-docs#8404

This is a proof of concept to reference a sequence of tagged services.

The problem bugs me for some time, and at first i thought the solution was to have some super generic compiler pass. If it could replace a lot of compilers in core.. perhaps worth it, but eventually each tag comes with it's own logic, including how to deal with tag attributes.

However, writing the passes over and over again becomes tedious for the most basic usecase. So given the recent developments, this idea came to mind.

```yml
services:
    a:
        class: stdClass
        properties: { a: true }
        tags: [foo]

    b:
        class: stdClass
        properties: { b: true }
        tags: [foo]

    c:
        class: stdClass
        properties:
            #stds: !tagged_services foo (see #22198)
            stds: !tagged_services
                foo
```

```
dump(iterator_to_array($this->get('c')->stds));
```

```
array:2 [▼
  0 => {#5052 ▼
    +"a": true
  }
  1 => {#4667 ▼
    +"b": true
  }
]
```

Given the _basic_ example at https://symfony.com/doc/current/service_container/tags.html, this could replace that.

Any thoughts?

Commits
-------

979e58f [DI] Reference tagged services in config

@ro0NL ro0NL deleted the ro0NL:di/tagged-services branch Sep 28, 2017

ogizanagi added a commit that referenced this pull request Sep 28, 2017

bug #24365 [DependencyInjection] fix tests (xabbuh)
This PR was merged into the 4.0-dev branch.

Discussion
----------

[DependencyInjection] fix tests

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | adapt tests from #22200
| License       | MIT
| Doc PR        |

Commits
-------

08deb37 [DependencyInjection] fix tests
@wachterjohannes

This comment has been minimized.

Show comment
Hide comment
@wachterjohannes

wachterjohannes Oct 6, 2017

@ro0NL @nicolas-grekas with this PR is it possible to use an alias as array index?

wachterjohannes commented Oct 6, 2017

@ro0NL @nicolas-grekas with this PR is it possible to use an alias as array index?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Oct 6, 2017

Contributor

Nope :) just a sequence collection. We had a long debate about that.

You might infer alias/key from get_class($service) perhaps.

Contributor

ro0NL commented Oct 6, 2017

Nope :) just a sequence collection. We had a long debate about that.

You might infer alias/key from get_class($service) perhaps.

@wachterjohannes

This comment has been minimized.

Show comment
Hide comment
@wachterjohannes

wachterjohannes Oct 6, 2017

@ro0NL we from sulu (https://github.com/sulu/sulu) uses the tagged services and also aliased because we want to allow overriding this services. so this would be an awesome additional feature for us

wachterjohannes commented Oct 6, 2017

@ro0NL we from sulu (https://github.com/sulu/sulu) uses the tagged services and also aliased because we want to allow overriding this services. so this would be an awesome additional feature for us

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 6, 2017

Member

Well done @ro0NL! :)

Member

chalasr commented Oct 6, 2017

Well done @ro0NL! :)

@@ -61,6 +61,7 @@ public function __construct()
new AutowireRequiredMethodsPass(),
new ResolveBindingsPass(),
new AutowirePass(false),
new ResolveTaggedIteratorArgumentPass(),

This comment has been minimized.

@stof

stof Oct 6, 2017

Member

this should be before the DecoratorServicePass, so that tagging a service and then decorating it creates the reference with the original id (and so gets the decorated service rather than the inner one). It should be at the same place than the ServiceLocatorTagPass (which is here for the same reason)

@stof

stof Oct 6, 2017

Member

this should be before the DecoratorServicePass, so that tagging a service and then decorating it creates the reference with the original id (and so gets the decorated service rather than the inner one). It should be at the same place than the ServiceLocatorTagPass (which is here for the same reason)

This comment has been minimized.

@ro0NL

ro0NL Oct 6, 2017

Contributor

@stof debugged this real quick, and AFAIK for a decorated service we already inherit tags via ResolveChildDefinitionsPass, which runs sooner.

Thus ContainerBuilder::findTaggedServiceIds() returns expected service id's already. Am i missing something?

@ro0NL

ro0NL Oct 6, 2017

Contributor

@stof debugged this real quick, and AFAIK for a decorated service we already inherit tags via ResolveChildDefinitionsPass, which runs sooner.

Thus ContainerBuilder::findTaggedServiceIds() returns expected service id's already. Am i missing something?

This comment has been minimized.

@stof

stof Oct 9, 2017

Member

child definitions are not about decorated services. Child definitions are about service definition inheritance (which is precisely why we renamed DecoratorDefinition to ChildDefinition as the previous name was totally misleading, especially after we implemented service decoration)

@stof

stof Oct 9, 2017

Member

child definitions are not about decorated services. Child definitions are about service definition inheritance (which is precisely why we renamed DecoratorDefinition to ChildDefinition as the previous name was totally misleading, especially after we implemented service decoration)

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 6, 2017

Contributor

@ro0NL to clarify, can we use variadic's then?

Contributor

kbond commented Oct 6, 2017

@ro0NL to clarify, can we use variadic's then?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 6, 2017

Member

nope, it injects an iterator. Variadics argument would not be compatible with IteratorArgument's lazy-loading.
Variadic argument would require creating multiple arguments in the definition, which is much more complex to implement here (this feature has nothing to do with autowiring btw, it does not inspect the signature).

Member

stof commented Oct 6, 2017

nope, it injects an iterator. Variadics argument would not be compatible with IteratorArgument's lazy-loading.
Variadic argument would require creating multiple arguments in the definition, which is much more complex to implement here (this feature has nothing to do with autowiring btw, it does not inspect the signature).

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 6, 2017

Contributor

Thanks for the clarification and explanation.

Contributor

kbond commented Oct 6, 2017

Thanks for the clarification and explanation.

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Oct 13, 2017

feature #8404 Docs for referencing tagged services in config (ro0NL, …
…javiereguiluz)

This PR was merged into the 3.4 branch.

Discussion
----------

Docs for referencing tagged services in config

See symfony/symfony#22200

Curious how it looks :) also a bit related to #8403

Commits
-------

ed70659 Update tags.rst
2e5c87f Update tags.rst
564b5ea Update tags.rst
a2fd23f Update tags.rst
000b801 Minor reword and fixes
0aaf48b Update tags.rst
71158f8 Update tags.rst
61c74da Update tags.rst
da034d2 Docs for referencing tagged services in config

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment