Skip to content

[RFC] Simplify wiring of service locators #29203

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

Jump to bottom
Closed
zmitic opened this issue Nov 13, 2018 · 14 comments
Closed

[RFC] Simplify wiring of service locators #29203

zmitic opened this issue Nov 13, 2018 · 14 comments
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@zmitic
Copy link

zmitic commented Nov 13, 2018 •
edited by nicolas-grekas
Loading

@nicolas-grekas on Twitter asked me to create RFC. And when Symfony core member makes a call, you answer that call 😄

Is it possible to have autowiring for Service Locator? Example:

interface CustomizedServiceNameInterface
{
    public static function getName(): string;
}

interface TagCollectionInterface
{
    public static function getTagName(): string;
}

Because components need custom name instead of FQCN, programmer has to implement interface (otherwise, it works as usual FQCN as name):

class HeaderComponent implements CustomizedServiceNameInterface, ComponentInterface
{
    public static function getName(): string
    {
        return 'header';
    }
}

so when I make class that implements TagCollectionInterface:

class MyCollectionOfTaggedServices extends ServiceLocator implements TagCollectionInterface
{
    public static function getTagName(): string
    {
        return ComponentInterface::class;
    }
}

$factories is populated as

array [
  HeaderComponent::getName() => new Reference(HeaderComponent::class),
 // just an example, I know these are callables
];

instead of

array [
  HeaderComponent::class => new Reference(HeaderComponent::class),
];

Right now I have to write a compiler pass:

image

It is pretty messy code, but the thing is that I make array of components where key is read from ComponentInterface::getName() static method. The rest (like $map variable) is irrelevant to Symfony.

Anyway, tell me what you think. This idea will be scraped, I have better idea now (using annotations on controller + kernel.view event), define route tree just like NG and it will be good to go. It will not change the speed but will clean the code.
@chalasr chalasr added DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Nov 14, 2018
@Tobion
Copy link
Contributor

Tobion commented Nov 17, 2018

Sorry but your description is not understandable to me. What kind of feedback are you asking for in this RFC?

@zmitic
Copy link
Author

zmitic commented Nov 17, 2018

@Tobion You are right, I just re-read it and it does sound confusing. I wanted to explain how this site works and why it is so fast; I did it because @nicolas-grekas was interested in that and I don't have a blog.

So I am closing it and when I make this as a bundle, I might create new RFC with link only.

However I think the idea of autowired service locators (for tagged services) is still something very usable and would like to see in the core.

@zmitic zmitic closed this as completed Nov 17, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 17, 2018 •
edited
Loading

You're creating service locators using a convention to generate its keys. It's pretty easy right now to get collections of services without writting any DI pass (eg with !tagged), but it doesn't work for locators. The deep reason for this current impossibility is that we don't know which keys we should give to the services in the collection (for !tagged, we don't need special keys - an increasing number is enough.)

So, here, you're seeking for a way to express how the keys should be generated. A custom tag + DI pass does the job, but you're wondering if some more generic mechanism could allow reducing the need for custom code.

At least that's what I understood so far :)

@zmitic
Copy link
Author

zmitic commented Nov 17, 2018

@nicolas Exactly, thanks, I didn't know how to ask correctly. If I used tagged services, all would be instantiated and I would loose speed. In real site, having 50+ components is likely expected.

Even if $factories can be autowired as $id => new Reference($id) , it would be enough. The best solution is probably using @param like this issue but I see it is closed.

@nicolas-grekas
Copy link
Member

I removed the first part in your description because I think it's not needed to explain your point. Reopening.

@yceruto
Copy link
Member

yceruto commented Nov 17, 2018 •
edited
Loading

I recently asked @ro0NL about this possibility that !tagged returned the collection indexed by the id of the service and make ServiceLocatorTagPass to support TaggedIteratorArgument.

Basically:

[...] wondering if some more generic mechanism could allow reducing the need for custom code. [...]

I would like something like this:

services:
    task_worker_locator:
        class: Symfony\Component\DependencyInjection\ServiceLocator
        arguments: ['!tagged task_worker']
        tags: ['container.service_locator']

instead of actual:

class WorkerCompilerPass implements CompilerPassInterface
{
    use PriorityTaggedServiceTrait;

    public function process(ContainerBuilder $container)
    {
        $definition = $container->findDefinition('task_worker_locator');

        $services = [];
        foreach ($this->findAndSortTaggedServices('task_worker', $container) as $reference) {
            $services[(string) $reference] = $reference;
        }

        $definition->replaceArgument(0, $services);
    }
}

later we can do this $this->workerLocator->get(TaggedTaskWorkerSrv::class)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 17, 2018 •
edited
Loading

Also: foo_locator: !service_locator !tagged foo_tag. BUT I'm wondering if indexing by service id is legitimate: it looks like another form of "by convention" binding - ie something that works just "by chance" because the configuration happens to be OK. How can a consumer then know about the keys that exist in such a locator? FQCN is the only sensible way right now. But service ids don't have to be FQCN, so that !tagged might gather non-FQCN-identified services.
We should figure out this before moving anything forward IMHO.

@ciaranmcnulty
Copy link
Contributor

ciaranmcnulty commented Nov 29, 2018 •
edited
Loading

This would be useful for me too, I'm doing something like:

  my.user_repositories:
    class: Symfony\Component\DependencyInjection\ServiceLocator
    arguments:
    -
      memory: '@My\InMemory\UserRepository'
      mysql: '@My\Doctrine\UserRepository'
    tags: ['container.service_locator']

(A factory uses this to select service at runtime using an env DSN)

So in my case I want to get a specific key for each service that isn't just the ID. Maybe there's a convention we can use like:

  My\InMemory\UserRepository:
    tags:
      - { name: 'my.user.repository', service-key: 'memory'}

  my.user_repositories:
    class: Symfony\Component\DependencyInjection\ServiceLocator
    arguments: ['!tagged-keys my.user.repository']
    tags: ['container.service_locator']

?

Then we could do something app-specific with Autoconfiguration to generate those keys

@nicolas-grekas
Copy link
Member

@ciaranmcnulty that's almost what we're working on with @deguif, to be submitted at SFCon's hack day :)

@ciaranmcnulty
Copy link
Contributor

@nicolas-grekas Damn I don't think I'm at the hack day but let's talk about it :-)

@deguif deguif mentioned this issue Dec 13, 2018
Closed
4 tasks
@nicolas-grekas
Copy link
Member

See #29598

@XuruDragon XuruDragon mentioned this issue Feb 14, 2019
Closed
4 tasks
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 14, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
f9133a6 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader
* [x]  Support YAML loader
* [x]  Support XML loader (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 14, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
d6b3908 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader
* [x]  Support YAML loader
* [x]  Support XML loader (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 14, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
0d99ffc 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 14, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
f8b2815 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 15, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
3b53adb 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 15, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
5333e9d 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
@XuruDragon XuruDragon mentioned this issue Feb 15, 2019
Merged
5 tasks
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 15, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
c84f9b9 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
XuruDragon pushed a commit to XuruDragon/symfony that referenced this issue Feb 15, 2019
[DI] change name to tag + add XMl support + adding yaml/xml tests
d18fd9a 
| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yno
| Tests pass?   | yes
| Fixed tickets | symfony#29203
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This is the continuity of the PR #

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged { tag: 'foo'', index_by: 'tag_attribute_name', default_index_method: 'static_method'}
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dump (update XSD too)
* [x]  Add tests
@nicolas-grekas
Copy link
Member

Implemented in #30257, please have a look.

@zmitic
Copy link
Author

zmitic commented Feb 16, 2019

This is amazing! Just checked the code and I really like that method name is specified in TaggedIteratorArgument.
Thank you all.

nicolas-grekas added a commit that referenced this issue Feb 22, 2019
@nicolas-grekas
feature #30257 [DependencyInjection] Allow to choose an index for tag…
b4f6c34 
…ged collection (deguif, XuruDragon)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DependencyInjection] Allow to choose an index for tagged collection

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

This is the continuity of the PR #29598

Add a way to specify an index based on a tag attribute when injecting a tag collection into services, but also a a way to fallback to a static method on the service class.

```yaml
services:
  foo_service:
    class: Foo
    tags:
      - foo
  foo_service_tagged:
    class: Bar
    arguments:
      - !tagged
          tag: 'foo'
          index_by: 'tag_attribute_name'
          default_index_method: 'static_method'
```
```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 http://symfony.com/schema/dic/services/services-1.0.xsd">
  <services>
    <service id="foo" class="Foo">
        <tag name="foo_tag" />
    </service>
    <service id="foo_tagged_iterator" class="Bar" public="true">
      <argument type="tagged" tag="foo_tag" index-by="tag_attribute_name" default-index-method="static_method" />
    </service>
  </services>
</container>
```

Tasks

* [x]  Support PHP loader/dumper
* [x]  Support YAML loader/dumper
* [x]  Support XML loader/dumper (and update XSD too)
* [x]  Add tests
* [x]  Documentation

Commits
-------

101bfd7 [DI] change name to tag + add XMl support + adding yaml/xml tests
845d3a6 Allow to choose an index for tagged collection
@XuruDragon XuruDragon mentioned this issue Mar 15, 2019
Merged
5 tasks
@nicolas-grekas
Copy link
Member

#30348 is now merged, time to try it.

@zmitic zmitic mentioned this issue Jan 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@ciaranmcnulty @nicolas-grekas @Tobion @zmitic @yceruto @chalasr