Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Allow to choose an index for tagged collection#29598
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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}
src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| if (\is_string($argument) &&$argument) { | ||
| returnnewTaggedIteratorArgument($argument); | ||
| }elseif (\is_array($argument) &&isset($argument['name']) &&$argument['name']) { | ||
| returnnewTaggedIteratorArgument($argument['name'],$argument['index_by'] ??null,$argument['default_index_method'] ??null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to ease spotting typos, we should validate that there are no extra keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| continue; | ||
| } | ||
| if (null ===$defaultIndexMethod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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: ''}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this use case exists. Why would a tagged iterator care? That's not its job to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
nicolas-grekasDec 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
correct - in case the convention doesn't suit you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Behaviour is now updated
a2041eb tob750077Comparesrc/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| { | ||
| $services =array(); | ||
| if (null ===$indexAttribute &&null !==$defaultIndexMethod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is this really required? i might want to "key by method" as a default approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is: one must have a way to configure the key they want using configuration. The command example is again enlightening: itmust be possible to ignore the default command name and let configuration take over the real command name. Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i figured :)
ro0NL commentedDec 14, 2018
alternatively if index_by is null we follow the convention mentioned above to find a default method (is_method=true). |
nicolas-grekas commentedDec 14, 2018
@ro0NL nope, really, see above comment. |
ro0NL commentedDec 14, 2018
Wait, got ya ... an |
b750077 to1b5b1f8Compare5006043 tof40903eComparekunicmarko20 commentedJan 29, 2019
This is exactly what I am looking for, is it possible to have some kind of fallback and use class name for index_by? |
nicolas-grekas commentedJan 29, 2019
@kunicmarko20 the fallback exists: create the static method covers all use cases and reduces complexity, no need for anything else IMHO. |
kunicmarko20 commentedJan 29, 2019
It isn't really a fallback if I have to create a method and return the class name, but at least better than me creating compiler pass all the time, thank you! great addition! |
f40903e to2b54becCompare2b54bec to17d9faeComparenicolas-grekas commentedFeb 15, 2019
…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#11009This is the continuity of the PR#29598Add 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.```yamlservices: 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/serviceshttp://symfony.com/schema/dic/services/services-1.0.xsd"> <services> <service> <tag name="foo_tag" /> </service> <service 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] DocumentationCommits-------101bfd7 [DI] change name to tag + add XMl support + adding yaml/xml tests845d3a6 Allow to choose an index for tagged collection
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