Skip to content
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] enable improved syntax for defining method calls in Yaml #33779

Merged
merged 1 commit into from Oct 7, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Oct 1, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

I've always found the syntax to write method calls in Yaml cumbersome. This PR allows a new syntax that is way easier to type and get (to me at least):

services:
  App\MyService:
    calls:
      - addStuff: ['@App\Stuff']

for the case where a wither is to be declared, using the same wording as in XML:

services:
  App\MyService:
    calls:
      - withStuff: !returns_clone ['@App\Stuff']

That's what this PR provides (+ additional syntax checks to guard against typos).

Note that deprecating the current syntax wouldn't be nice for the ecosystem in 4.4, but could be considered starting with 5.1.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 1, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-caller-yaml branch from 0302846 to f1dc5ec Oct 1, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-caller-yaml branch from f1dc5ec to 1ea22fb Oct 1, 2019
@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

For this kind of renamings/refactorings, we usually ask the new names/syntax to be much better than the original ones (we say "ten times better", but it's not a scientific measure). Why? Because the effort to update our docs, making other community doc resources "obsolete" and forcing developers to learn a new way of doing things must be worth it.

Looking at the before/after comparison, I think the new way it's better but I'm not sure it's much better. What do others think? Thanks!

services:
  App\MyService:
    calls:
      # Before:
      - { method: 'addStuff', arguments: ['@App\Stuff']
      # After:
      - 'addStuff': ['@App\Stuff']

      # Before:
      - ['withStuff', ['@App\Stuff'], true]
      # After
      - 'withStuff': !returns_clone ['@App\Stuff']
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

@javiereguiluz good point, please have look at the example in https://twitter.com/nicolasgrekas/status/1178763141485912064?s=19
Can you spot the mistake? Can you fix it?

With trivial definitions we miss the point I think but once we get into array in arguments, inline services, etc. the difference is significant to me.

@Cydonia7

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@javiereguiluz About forcing developers to learn a new way, I'm not sure most people remember the current syntax exactly because this feature is not among the most often used. I personnally always look it up in the documentation when I do need it.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@javiereguiluz here is the complete example (we have 2 existing syntaxes for both cases):

services:
  App\MyService:
    calls:
      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'] }
      - ['withStuff', ['@App\Stuff']]
      # After:
      - addStuff: ['@App\Stuff']

      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'], returns_clone: true }
      - ['withStuff', ['@App\Stuff'], true]
      # After
      - withStuff: !returns_clone ['@App\Stuff']
@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

!returns_clone is problematic, as that syntax is reserved for YAML tag, which "represents type information": https://yaml.org/spec/1.2/spec.html#tag//

Also, on its own:

calls:
    - withStuff: !returns_clone ['@App\Stuff']

isn't exactly intuitive either. It gives the impression that the "returns clone" is somehow tied to the method arguments, which it of course is not.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@teohhanhui well, that syntax is precisely using a YAML tag.

but I agree that this syntax is quite confusing, by trying returns clone to arguments. IMO, the existing mapping-based syntax is easier to understand in this case.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@stof:

well, that syntax is precisely using a YAML tag.

Yes, but according to the YAML spec, a tag "represents type information". I'm not sure such an interpretation is possible here.

Perhaps this might work better:

calls:
    - withStuff: { arguments: ['@App\Stuff'], returns_clone: true }

or in expanded formatting:

calls:
    -   withStuff:
            arguments:
                - '@App\Stuff'
            returns_clone: true

In summary, just allow moving method to YAML key.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Yeah, in that case, the usage of a tag is indeed abusing the system (which is also related to the fact that it is confusing).
but if we go back to mapping to configure arguments and returns_clone, I don't see any benefit to moving the method name as a key rather than a value (and we cannot switch back that way only when needing returns_clone as that would be ambiguous).

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

My modified proposal, in full:

services:
  App\MyService:
    calls:
      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'] }
      - ['withStuff', ['@App\Stuff']]
      # After (2 supported ways):
      - addStuff: ['@App\Stuff']
      - addStuff: { arguments: ['@App\Stuff'] }

      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'], returns_clone: true }
      - ['withStuff', ['@App\Stuff'], true]
      # After
      - withStuff: { arguments: ['@App\Stuff'], returns_clone: true }
@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@teohhanhui your proposal makes it very hard to avoid ambiguity, given that arguments are not limited to a simple list of strings.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@stof:

your proposal makes it very hard to avoid ambiguity, given that arguments are not limited to a simple list of strings.

I don't see any possible ambiguity. Or have I missed something? A list of arguments must always be, well, a list. That's already the case now and my proposal changes nothing about that. Complex arguments are still nested in a list.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

@teohhanhui not true. When using autowiring, we support named arguments to specify some arguments while letting others be autowiring. And when using ChildDefinition, we can also have string keys to override some parent arguments. Both of these cases have a mapping for arguments, not a list.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@stof Are those applicable to calls too? Even so, we could tell the difference by the presence of the arguments key, right?

@stof

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

yes, autowiring works with call arguments too.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

Taking the autowiring case into account:

services:
  App\MyService:
    calls:
      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'] }
      - ['withStuff', ['@App\Stuff']]
      # After (2 supported ways):
      - addStuff: ['@App\Stuff']
      - addStuff: { arguments: ['@App\Stuff'] }

      # Before (2 supported ways):
      - { method: 'addStuff', arguments: { $stuff: '@App\Stuff' } }
      - ['withStuff', { $stuff: '@App\Stuff' }]
      # After (2 supported ways):
      - addStuff: { $stuff: '@App\Stuff' }
      - addStuff: { arguments: { $stuff: '@App\Stuff' } }

      # Before (2 supported ways):
      - { method: 'addStuff', arguments: ['@App\Stuff'], returns_clone: true }
      - ['withStuff', ['@App\Stuff'], true]
      # After
      - withStuff: { arguments: ['@App\Stuff'], returns_clone: true }

(If the array has arguments or returns_clone key, it's not the arguments list; otherwise it must be the arguments list.)

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2019

I was about to move !returns_clone before the method, but then that would force one to add brackets around the call definition:
- !returns_clone {foo: [...]}
vs this for regular calls:
- foo: [...]

Therefore I'd prefer to stick with the current syntax. Also because to me, the method should be read before the type of return.

Yaml doesn't have any inherent semantics past its base parsing rules, so we're not "abusing" anything: we define what a specific yaml tree means.

(it'd be great if we could avoid such strong words btw, to keep the discussions open...)

Status: needs review

(I'm going to submit a separate PR to add a test case as @stof requested in #33779 (comment))

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

/cc @symfony/mergers please review this one, it's an important DX improvement (to me at least, I really really don't like the current [method, [...]] syntax.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

We would then have three different notations for configuring method calls:

services:
    App\MyService:
        calls:
            - addStuff: ['@App\Stuff']

vs.

    App\MyService:
        calls:
            method: addStuff
            arguments: ['@App\Stuff']

vs.

    App\MyService:
        calls:
            - [addStuff, ['@App\Stuff']]

IMO that would be too many and I would at least try to get rid of the third one.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

@xabbuh 100% agree, that's why I added this note in the description:

Note that deprecating the current syntax wouldn't be nice for the ecosystem in 4.4, but could be considered starting with 5.1.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@nicolas-grekas works for me 👍

@xabbuh
xabbuh approved these changes Oct 7, 2019
@chalasr
chalasr approved these changes Oct 7, 2019
Copy link
Member

left a comment

Cool :)

@yceruto
yceruto approved these changes Oct 7, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-caller-yaml branch from 1ea22fb to 2b27fb2 Oct 7, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-caller-yaml branch from 2b27fb2 to cdc7446 Oct 7, 2019
@chalasr

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Thank you @nicolas-grekas.

chalasr added a commit that referenced this pull request Oct 7, 2019
…in Yaml (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DI] enable improved syntax for defining method calls in Yaml

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

I've always found the syntax to write method calls in Yaml cumbersome. This PR allows a new syntax that is way easier to type and get (to me at least):

```yaml
services:
  App\MyService:
    calls:
      - addStuff: ['@app\Stuff']
```

for the case where a wither is to be declared, using the same wording as in XML:
```yaml
services:
  App\MyService:
    calls:
      - withStuff: !returns_clone ['@app\Stuff']
```

That's what this PR provides (+ additional syntax checks to guard against typos).

Note that deprecating the current syntax wouldn't be nice for the ecosystem in 4.4, but could be considered starting with 5.1.

Commits
-------

cdc7446 [DI] enable improved syntax for defining method calls in Yaml
@chalasr chalasr merged commit cdc7446 into symfony:4.4 Oct 7, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-caller-yaml branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.