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

[DependencyInjection] Anonymous services in PHP DSL #24632

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
10 participants
@unkind
Contributor

unkind commented Oct 19, 2017

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:

$s->anonymous(SendEmailForRegisteredUser::class)->tag('listener');
@unkind

This comment has been minimized.

Contributor

unkind commented Oct 19, 2017

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.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 19, 2017

@chalasr chalasr changed the base branch from master to 3.4 Oct 19, 2017

@unkind unkind changed the title from [DependencyInjection] Anonymous services & decorates() to [DependencyInjection] Anonymous services & decorators Oct 19, 2017

@nicolas-grekas

I think I'm 👎: this will introduce a non trivial amount of complexity (it's broken already), when the current way just works (just give that decorator a name.)

@@ -22,6 +30,25 @@
*/
final public function args(array $arguments)
{
$arguments = array_map(

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 19, 2017

Member

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).

This comment has been minimized.

@unkind

unkind Oct 19, 2017

Contributor

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 19, 2017

Member

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.)

This comment has been minimized.

@unkind

unkind Oct 19, 2017

Contributor

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?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 19, 2017

Member

it should (but I'm still not sure it's worth the added complexity)

This comment has been minimized.

@unkind

unkind Oct 19, 2017

Contributor

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 19, 2017

Member

it's used in function iterator(array $values)

);
}
return new ReferenceConfigurator($decorated[1] ? $decorated[1] : $this->id . '.inner');

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 19, 2017

Member

".inner" is not hardcoded when decorating services, see "renamedId".

This comment has been minimized.

@unkind

unkind Oct 19, 2017

Contributor

I don't get it. $decorated[1] contains value of renamedId. I use .inner suffix when user didn't specify renamedId.

This comment has been minimized.

@nicolas-grekas
@unkind

This comment has been minimized.

Contributor

unkind commented Oct 19, 2017

I think I'm 👎: this will introduce a non trivial amount of complexity (it's broken already), when the current way just works (just give that decorator a name.)

What about just anonymous services without decorators?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 19, 2017

What about just anonymous services without decorators?

I would do it without the "anonymous" helper: ->set(null, Fqcn::class)

@unkind

This comment has been minimized.

Contributor

unkind commented Oct 19, 2017

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 set(SendEmailForRegisteredUserListener::class, ...) for most of them, but they include namespace and service name becomes very long. On other hand, typing set(null, ...) again and again doesn't make me happy as well.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 19, 2017

My fear is that ppl will get confused when reading
->set(Foo::class)
vs
->anonymous(Foo::class)
the difference is not obvious if you're not used to anonymous services (how many of us are, of many new comers...)
->set(null, Foo::class) on the contrary is "more" different than ->set(Foo::class)

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.

@unkind

This comment has been minimized.

Contributor

unkind commented Oct 19, 2017

the difference is not obvious if you're not used to anonymous services
And I'm also fearing that writing actually might become more confusing

Let them learn the difference. They won't hurt themselves, it's not a gun.

(how many of us are, of many new comers...)

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.

@unkind unkind changed the title from [DependencyInjection] Anonymous services & decorators to [DependencyInjection] Anonymous services Oct 20, 2017

@unkind

This comment has been minimized.

Contributor

unkind commented Oct 20, 2017

I've removed decorators, tests are OK except some unrelated ones.

Let's focus on the anonymous() helper. Do you really want to remove it? I'd like to see it as explicit way to register a service, I really don't like nullables, it's almost always better to introduce new straightforward method. I also really doubt it's sort of "forbidden knowledge" that may bring any problems to people. They'd rather discover something new for themselves.

@unkind

This comment has been minimized.

Contributor

unkind commented Oct 20, 2017

Offtopic:

just give that decorator a name

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 game.infrastructure.players_qs_memcache_decorator.inner, it becomes a problem. We also have sometimes 3-4 similar services with decorators, so I copy-paste definitions and trying to find the difference in names which I don't even need, ugh.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 21, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 26, 2017

I'm still really unsold on the topic, if anything, ->set(null, Foo::class) should be the way to IMHO.
Services don't have to be anonymous. Giving them a name is fine (esp. when the name is automatically set as is now.)

@unkind

This comment has been minimized.

Contributor

unkind commented Nov 26, 2017

@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.

->set(null, Foo::class)

Giving a name for this construction is fine, isn't it? ->set(null, Foo::class) is WTF syntax and it's harder to google 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.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 26, 2017

Oh! :)
could be would need a reason. When doing ->set(Foo::class) is actually fine, I really don't see benefit of doing ->anonymous(Foo::class). The first providing a name is a side effect that benefits everyone when needing a name, and doesn't hurt anyone when a name is actually not needed. That's my reasoning. Just let SF give a name for you, and ignore it if you don't need it.
The only case where the name is actually boilerplate if when you need several services implemented by the same class. Which is a case where you have to type some configuration anyway, and doing ->set(null, Foo::class) would just work and not look like impossible boilerplate IMHO.

@unkind

This comment has been minimized.

Contributor

unkind commented Nov 26, 2017

@nicolas-grekas while ->set(Foo::class) works for many cases, I think this is just a coincidence. You presume that service is a singleton, but it isn't the same as anonymous service: it can be referenced directly (but you probably want explicitly say that listener shouldn't be injected directly), it actually has a name. Moreover, you have to keep in the mind every time whether a service is a singleton or not to choose between set(Foo:class) and set(null, Foo:class).

By doing ->anonymous(Foo::class) you explicitly say that service shouldn't be referenced directly and you don't care how much services of the class exist. It is easier, more straightforward and more google-friendly.

I really don't see benefit of doing

Benefit is clarity of intent. Let's say we want to make a coffee machine for several types of coffee including latte. You say that "you can make a latte by pressing buttons 'espresso', 'steamed milk' and 'milk foam' sequentially, but if X is true, you have to press them at the same time". I propose to add "make a latte" button, this is perfectly fine keyword for coffee machine bounded context.

Another minor technical benefit is a size of name.

The first providing a name is a side effect

Exactly, it is a side-effect and not primary function. In other words, you reach the pseudo-anonymous effect coincidentally in specific set of cases, and you have to use another syntax for the same effect in other cases. It is complicated.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 26, 2017

Ok, thanks for the arguments. I'll let others contribute to the discussion, as my personal pov doesn't specifically matter (and I might just be missing the point...)

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 4, 2017

In other words, introducing new keyword is about ubiquitous language. "Service without explicit name (null)" is worse than "anonymous". It is less descriptive and people don't talk like that in discussions. People are not robots. null is technical detail, implementation. We don't "set name of User", we rename him, we don't "set status to 'Blocked' and set 'banned_at' to the current timestamp", we block violators.

However, if majority of deciders think other way, I'm ready to make ->set(null, Foo::class).

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2017

ping @symfony/deciders I gave my pov, yours welcome now :)

*
* @return string
*/
public function generateServiceName($prefix)

This comment has been minimized.

@Tobion

Tobion Dec 8, 2017

Member

should be moved as a private method to the ServicesConfigurator as there is no point in exposing such a method

This comment has been minimized.

@unkind

unkind Dec 9, 2017

Contributor

I think it's possible to implement the same feature for other configurations. ServicesConfigurator is for fluent-format only, isn't it?

@Tobion

This comment has been minimized.

Member

Tobion commented Dec 8, 2017

->anonymous(Foo::class) is much better than ->set(null, Foo::class)

@weaverryan

This comment has been minimized.

Member

weaverryan commented Dec 9, 2017

Haha, well, it's one of those things that is tough to agree on.

I think the exact opposite. I think that ->set(null, Foo::class) is better than ->anonymous(Foo::class).

But there's more to it. I don't love ->set(null, Foo::class), but when we add a new function (e.g. anonymous), it introduces another auto-complete option, and so makes the 99% use-cases just a little bit less findable. That's a real cost, especially when the purpose of the PHP format is to help people naturally find their way.

That's why I prefer ->set(null, Foo::class): it's less discoverable (that's a +) and if you do find it, it's more descriptive. I don't know what an anonymous service is. But I can clearly see from this code that I'm creating a service without an id. No need to introduce new terminology.

tl;dr; +1 for ->set(null, Foo::class)

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 9, 2017

but when we add a new function (e.g. anonymous), it introduces another auto-complete option, and so makes the 99% use-cases just a little bit less findable

I prefer ->set(null, Foo::class): it's less discoverable (that's a +)

anonymous(Foo::class) makes definition "less findable" and ->set(null, Foo:class) is "less discoverable"? I'm confused. 😕

By the way, how do you search? Regex? I usually search by usages of the Foo::class.

What do you think about ->setNameless(Foo::class)?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 10, 2017

Thanks @weaverryan, we're on par.

@unkind ->setNameless(Foo:class) is longer than ->set(null, Foo:class) (if that matters) and has the same drawbacks as "anonymous": (from Ryan): "it introduces another auto-complete option, and so makes the 99% use-cases just a little bit less findable. That's a real cost"

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 10, 2017

Note that decisions for what we think should make it into core don't bind you to not have autocompletion:
It should be quite easy to define your own DSL, by creating a different set of configurator classes, and type-hinting for them in the closure.

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 10, 2017

I was really talking about anonymous services: ones at the root of the service list, without any "id" attribute.

Ah, I didn't know about it (#21970). Why they were confusing, because of syntax? As for me, XML and YAML are confusing formats by themselves. 🙂 UPD: wait, it looks like inlined service too:

services:
    _instanceof:
        FooInterface:
            arguments:
                - !service
                    class: Bar
                    autowire: true

Defaults and instanceof conditionals aren't applied on anonymous services, as in xml too.

Why?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 10, 2017

Yep, the title of the PR there talks about anonymous services, but that's what we should call inlined ones.

I was more thinking about #22903.

Defaults and instanceof conditionals aren't applied on anonymous services

No definitive answer. When we worked on it, it added a lot of mental overhead, and was technically not obvious either, with many undefined edge cases (eg tags: what's their meaning on an inlined service, etc. ?)
So we applied the "simplest is best" principle and all problems were solved. It prevents no one from doing anything, just makes things easier to reason about.

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 10, 2017

I was more thinking about #22903.

As I can see, the problem was exactly with implicit syntax: they confused <service class="..."> and <service id="...">. And now we fall into the same trap with ->set(null, '...') and ->set('...', null). It's not foolproof.

For XML, for example, I'd prefer <anonymous-service class="Acme\Foo">. id doesn't make sense for <anonymous-service>, for instance. Yeah, it is new terminology, but it helps to think more about what you are doing.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 11, 2017

it is new terminology

And for this reason, I'm 👎
There are enough concepts to grasp already.
All service must have a name in the end (even these anonymous one end up having one in the end - randomly generated)
Asking ppl to always add a name is making everything easier to understand, explain, reference, put in error messages, etc.

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 18, 2017

It should be quite easy to define your own DSL, by creating a different set of configurator classes, and type-hinting for them in the closure.

What do you think of decorators for ContainerConfigurator?

@@ -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);
         }
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 28, 2017

set(null, Foo::class) doesnt work in 4.0 due string $id type :) which is also the phpdoc in 3.4, this is actually consistent with the current loaders right.

Anyway, just walked into this trying to register a event listener twice. Now in a codebase full of set(Fqcn::class) the set(null, Fqcn::class) (if it worked) looks a bit odd... but im really thinking of a good ID value now 🤔 (the "naming is hard" issue also counts a bit IMHO).

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 28, 2017

naming is hard

Especially when you don't need a name (anonymous/lambda functions is a very close example).

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 29, 2017

set(null, Foo::class) doesnt work in 4.0 due string $id type

which is not an issue since the method is final, so can be changed without any BC break

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 29, 2017

Hm looking at failing tests locally

+    :\n
+        class: App\BarService\n
+        public: true\n

that's bad right? :)

@unkind

This comment has been minimized.

Contributor

unkind commented Dec 29, 2017

@ro0NL if you mean my branch, it's possible if you merged with 3.4/master because ContainerBuilderTest was changed (GitHub shows the conflict). I could resolve it, but I don't understand the state of PR.

Do I understand correctly that in order to merge it I have to replace ->anonymous() with ->set(null, ...)?

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 29, 2017

right. I tested on 3.4 :) i thought it worked today already. If we can settle with set(null, Fqcn::class) im fine 👍

@unkind

This comment has been minimized.

Contributor

unkind commented Jan 4, 2018

@nicolas-grekas can you change base branch on master?

@curry684

This comment has been minimized.

Contributor

curry684 commented Jan 4, 2018

You can do it yourself with the edit button at the top of the issue.

@unkind unkind changed the base branch from 3.4 to master Jan 4, 2018

@unkind

This comment has been minimized.

Contributor

unkind commented Jan 4, 2018

@curry684 thanks, didn't know it.

@nicolas-grekas nicolas-grekas changed the title from [DependencyInjection] Anonymous services to [DependencyInjection] Anonymous services in PHP DSL Jan 22, 2018

@nicolas-grekas

(@unlinkd FYI, I pushed some changes on your fork, was quicker for me this way. Ready on my side thank you.)

@unkind

This comment has been minimized.

Contributor

unkind commented Jan 22, 2018

OK, looks good.

@fabpot

fabpot approved these changes Jan 23, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 23, 2018

Cannot be merged as tests are broken.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jan 23, 2018

Tests fixed, should be green in a few minutes.

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 23, 2018

Thank you @unkind.

@fabpot fabpot merged commit ee42276 into symfony:master Jan 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 23, 2018

feature #24632 [DependencyInjection] Anonymous services in PHP DSL (u…
…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
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Jan 23, 2018

Nice one @unkind. Thx super useful :)

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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