[DependencyInjection] Anonymous services in PHP DSL #24632
Conversation
|
I didn't fix all the tests yet (is there any script to update all autogenerated DI-fixtures?), need feedback. ping @nicolas-grekas UPD: target branch is incorrect. Anyway, any chance to merge it in 3.4? If so, I'll reopen PR. |
55997aa to
02bf763
Compare
| @@ -22,6 +30,25 @@ | |||
| */ | |||
| final public function args(array $arguments) | |||
| { | |||
| $arguments = array_map( | |||
There was a problem hiding this comment.
Ihe inner can be injected anywhere, not only in the constructor, and can be nested also. The logic should be in AbstractConfigurator::processValue() (but it's not trivial to do so).
There was a problem hiding this comment.
and can be nested also
What do you mean?
The logic should be in AbstractConfigurator::processValue() (but it's not trivial to do so)
It is still in dev branch, later will be much harder to fix it. Probably, it's not the only case when we need Definition object there.
There was a problem hiding this comment.
the inner service doesn't have to be injected as an argument: it can be nested in an array, which itself is passed as argument (constructor, or setter, or property in fact.)
There was a problem hiding this comment.
the inner service doesn't have to be injected as an argument: it can be nested in an array, which itself is passed as argument (constructor, or setter, or property in fact.)
It just requires to tweak AbstractConfigurator::processValue() to make it work, doesn't it?
There was a problem hiding this comment.
it should (but I'm still not sure it's worth the added complexity)
There was a problem hiding this comment.
It there any reason why it is static? As I can see, AbstractConfigurator contains link to the definition. I'm not big fan of it, but it's cheap solution.
There was a problem hiding this comment.
it's used in function iterator(array $values)
| ); | ||
| } | ||
|
|
||
| return new ReferenceConfigurator($decorated[1] ? $decorated[1] : $this->id . '.inner'); |
There was a problem hiding this comment.
".inner" is not hardcoded when decorating services, see "renamedId".
There was a problem hiding this comment.
I don't get it. $decorated[1] contains value of renamedId. I use .inner suffix when user didn't specify renamedId.
What about just anonymous services without decorators? |
I would do it without the "anonymous" helper: |
Well, a lot (probably, most) of my services are anonymous: listeners, command handlers, API controllers, console commands, cache/monitoring/logging decorators. I can use |
|
My fear is that ppl will get confused when reading And I'm also fearing that writing actually might become more confusing: "anonymous" would become a new item on the autocompletion list, which ppl might have a hard time figure out what it means (because it's not that common) - and even scarier: ppl doing the mistake of selecting the wrong set/anonymous choice might have an even harder time figuring out their mistake. |
Let them learn the difference. They won't hurt themselves, it's not a gun.
I think most of us have such services in our codebases, we probably just don't realize it. It's yet another reason to make it explicit in my opinion. |
f304759 to
766ceb3
Compare
|
I've removed decorators, tests are OK except some unrelated ones. Let's focus on the |
|
Offtopic:
This annoys in the same way as requirement to specify name for "anonymous" services: I don't need it. Considering we have name convention for service names like |
|
I'm still really unsold on the topic, if anything, |
|
@nicolas-grekas it looks like a straw man for me: I didn't say that services have to be anonymous. Just many of them could be. Giving name is fine, but sometimes is totally pointless like rituals.
Giving a name for this construction is fine, isn't it? The whole discussion is about explicit vs implicit syntax for the same concept. I don't understand your point why this kind of syntax should be implicit. |
|
Oh! :) |
What do you think of decorators for @@ -23,6 +25,16 @@ use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigura
*/
class PhpFileLoader extends FileLoader
{
+ private $configuratorDecorator;
+
+ public static function withConfiguratorDecorator(ContainerBuilder $container, FileLocatorInterface $locator, callable $configuratorDecorator): PhpFileLoader
+ {
+ $loader = new self($container, $locator);
+ $loader->configuratorDecorator = $configuratorDecorator;
+
+ return $loader;
+ }
+
/**
* {@inheritdoc}
*/
@@ -44,7 +56,13 @@ class PhpFileLoader extends FileLoader
$callback = $load($path);
if ($callback instanceof \Closure) {
- $callback(new ContainerConfigurator($this->container, $this, $this->instanceof, $path, $resource), $this->container, $this);
+ $configurator = new ContainerConfigurator($this->container, $this, $this->instanceof, $path, $resource);
+
+ if ($this->configuratorDecorator) {
+ $configurator = call_user_func($this->configuratorDecorator, $configurator);
+ }
+
+ $callback($configurator, $this->container, $this);
} |
|
Anyway, just walked into this trying to register a event listener twice. Now in a codebase full of |
Especially when you don't need a name (anonymous/lambda functions is a very close example). |
which is not an issue since the method is final, so can be changed without any BC break |
|
Hm looking at failing tests locally that's bad right? :) |
|
@ro0NL if you mean my branch, it's possible if you merged with 3.4/master because Do I understand correctly that in order to merge it I have to replace |
|
right. I tested on 3.4 :) i thought it worked today already. If we can settle with |
766ceb3 to
d709412
Compare
|
@nicolas-grekas can you change base branch on master? |
|
You can do it yourself with the |
|
@curry684 thanks, didn't know it. |
d709412 to
8b6945d
Compare
8b6945d to
2348c11
Compare
|
OK, looks good. |
|
Cannot be merged as tests are broken. |
2348c11 to
ee42276
Compare
|
Tests fixed, should be green in a few minutes. |
|
Thank you @unkind. |
…nkind) This PR was merged into the 4.1-dev branch. Discussion ---------- [DependencyInjection] Anonymous services in PHP DSL | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | enhancement for fluent PHP configs | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24493 | License | MIT | Doc PR | not yet See #24493: ```php $s->anonymous(SendEmailForRegisteredUser::class)->tag('listener'); ``` Commits ------- ee42276 [DI] Add way to register service with implicit name using the PHP DSL
|
Nice one @unkind. Thx super useful :) |
See #24493: