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] Allow to choose an index for tagged collection #29598

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@deguif
Copy link
Contributor

deguif commented Dec 13, 2018

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

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.

!tagged {name: 'tag_name', index_by: 'tag_attribute_name', default_index_method: 'static_method'}

Tasks

  • Support PHP loader
  • Support YAML loader
  • Support XML loader (update XSD)
  • Add tests
@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

really nice :) This will work for iterators. The next step would be to make it work for locators also, using eg:
!service_locator !tagged {name: foo, index_by: bar}

Show resolved Hide resolved ...ny/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php Outdated
Show resolved Hide resolved ...ny/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php Outdated
if (\is_string($argument) && $argument) {
return new TaggedIteratorArgument($argument);
} elseif (\is_array($argument) && isset($argument['name']) && $argument['name']) {
return new TaggedIteratorArgument($argument['name'], $argument['index_by'] ?? null, $argument['default_index_method'] ?? null);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

to ease spotting typos, we should validate that there are no extra keys

continue;
}
if (null === $defaultIndexMethod) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

when no $defaultIndexMethod is provided, we should imho derivate a conventional one from the attribute name.
My suggestion would be to camel-case the attribute name "foo_bar" and build something like "getDefaultFooBarName":
$defaultIndexMethod = 'getDefault'.str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $indexAttribute))).'Name';

This comment has been minimized.

@deguif

deguif Dec 14, 2018

Contributor

I see the interest in the default method fallback.
But how can be handled the fact that sometime I don't want to fallback on a method and always require a tag attribute to be defined, should I pass an empty string eg. '' to handle this case?

For example: !tagged {name: 'tag_name', index_by: 'tag_attribute', default_index_method: ''}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

I'm not sure this use case exists. Why would a tagged iterator care? That's not its job to me.

This comment has been minimized.

@deguif

deguif Dec 14, 2018

Contributor

Ok what you mean, is that there should always be a fallback method, and this fallback method is by default auto generated from the above strategy.
What I can do is only override this default by providing one explicitly?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

correct - in case the convention doesn't suit you

This comment has been minimized.

@deguif

deguif Dec 14, 2018

Contributor

Behaviour is now updated

@nicolas-grekas nicolas-grekas changed the title Allow to choose an index for tagged collection [DI] Allow to choose an index for tagged collection Dec 14, 2018

@deguif deguif force-pushed the deguif:custom-indexable-tagged-iterator branch from a2041eb to b750077 Dec 14, 2018

continue;
}
if (isset($attributes[0][$indexAttribute])) {

This comment has been minimized.

@ro0NL

ro0NL Dec 14, 2018

Contributor

here we potentially ignore a possible key from subsequent attributes, e.g. attributes[n].<index_by>, is it expected?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

That's how many passes already behave, including AddConsoleCommandPass and all is fine, nobody asked for something different. We shouldn't invent a use case when none has been reported :)

This comment has been minimized.

@ro0NL

ro0NL Dec 14, 2018

Contributor

fair 👍

{
$services = array();
if (null === $indexAttribute && null !== $defaultIndexMethod) {

This comment has been minimized.

@ro0NL

ro0NL Dec 14, 2018

Contributor

is this really required? i might want to "key by method" as a default approach

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 14, 2018

Member

It is: one must have a way to configure the key they want using configuration. The command example is again enlightening: it must be possible to ignore the default command name and let configuration take over the real command name. Same here.

This comment has been minimized.

@ro0NL

ro0NL Dec 14, 2018

Contributor

i figured :)

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 14, 2018

alternatively

!tagged
  name: tag_name,
  index_by: null | attribute_or_method_name
  is_method: true | false

if index_by is null we follow the convention mentioned above to find a default method (is_method=true).

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 14, 2018

@ro0NL nope, really, see above comment.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 14, 2018

Wait, got ya ... an alias attribute locally overrides Class::getAliasName() 👍

@deguif deguif force-pushed the deguif:custom-indexable-tagged-iterator branch from b750077 to 1b5b1f8 Dec 14, 2018

@deguif deguif force-pushed the deguif:custom-indexable-tagged-iterator branch from 1b5b1f8 to 5006043 Jan 11, 2019

@deguif deguif force-pushed the deguif:custom-indexable-tagged-iterator branch from 5006043 to f40903e Jan 18, 2019

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