[DependencyInjection] Add support for named arguments #21383

dunglas · 2017-01-23T22:38:53Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	todo

This PR introduces named arguments for services definitions. It's especially useful to inject parameters in an autowired service. It is (at least partially) an alternative to #21376 and #20738.

Usage:

services:
    _defaults: { autowire: true }
    Acme\NewsletterManager: { $apiKey: "%mandrill_api_key%" }

# Alternative (traditional) syntax
services:
    newsletter_manager:
        class: Acme\NewsletterManager
        arguments:
            $apiKey: "%mandrill_api_key%"
        autowire: true

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

namespace Acme;

class NewsletterManager
{
    private $logger;
    private $em;
    private $apiKey;

    public function __construct(LoggerInterface $logger, EntityManager $em, $apiKey)
    {
        $this->logger = $logger;
        $this->em = $em;
        $this->apiKey = $apiKey;
    }
}

nicolas-grekas · 2017-01-23T22:46:09Z

I like it.
Should be made to work with method calls.
Missing resources tracking for now.
With now two alts to #20738, I think we should close it :)

fabpot · 2017-01-23T22:49:56Z

Note that this conflicts with the implementation of #21313 that was just merged.

ogizanagi · 2017-01-23T22:53:46Z

Note that this conflicts with the implementation of #21313 that was just merged.

Not really (AFAIU). But can't be used along with.

Side-note: I've already seen some projects using arguments keys, despite it had no effect until now (simply for "increasing readability"). Shouldn't it be considered as a BC break?

fabpot · 2017-01-23T22:55:47Z

services:
    App\Foo\Bar: {'@baz', 'foo', apiKey: "%mandril_api_key%"}

dunglas · 2017-01-23T22:56:22Z

@ogizanagi in my opinion no, it was explicitly documented to support only packed arrays (see my changes in Definition).
If it really matters, I can skip named parameters not matching instead of throwing, but it will be harder to debug.

dunglas · 2017-01-23T22:58:34Z

Not really (AFAIU). But can't be used along with.

Indeed, I'll try to adapt my PR to be able to use both syntaxes together.

This would be nice but it actually doesn't work:

services:
    Acme\NewsletterManager: { apiKey: "%mandril_api_key%" }

But this works:

services:
    Acme\NewsletterManager:
        arguments: { apiKey: "%mandril_api_key%" }

dunglas · 2017-01-23T23:03:20Z

@nicolas-grekas

Should be made to work with method calls.

It would be a nice addition, I propose to do it in another PR when this one will be reviewed and merged.

Missing resources tracking for now.

Done, I rely on @weaverryan's implementation for now because conditions to rebuild the container will be the same if we add calls support later.

With now two alts to #20738, I think we should close it :)

Done!

dunglas · 2017-01-23T23:21:46Z

@kasparsklavins we are trying to find the best way to inject parameters when using autowiring and service auto-registration (#21289). It will be a brand new way to use Symfony that will change drastically the first experience for new users and allow to develop faster.
For now, there is no way to do it. Please consider the big picture. Btw, you can already define a service using XML, YAML, PHP, your own loader, autowiring and so on...

dunglas · 2017-01-23T23:22:34Z

@fabpot @ogizanagi I've tried something to support the short syntax: b8c0e7c WDYT?

ogizanagi · 2017-01-23T23:27:59Z

Looks good to me, as soon as we warn in the documentation no argument should be named the same as one of the definition keywords, otherwise it won't work.
I don't know if we can do better for now :)

ogizanagi · 2017-01-24T06:10:54Z

Re-thinking about it, it also means introducing a new keyword someday means a potential BC break for someone using the short syntax along with named arguments 😕
So maybe this tradeoff isn't worth it and the short syntax should be kept for most simple usages.

nicolas-grekas · 2017-01-24T06:16:01Z

Enforce a leading $, eg $apiKey: '%foo%'?
About method calls, is it that a big change ? Because that feels incomplete to me to not have it right now...

linaori · 2017-01-24T07:12:11Z

One minor issue I see with this: when you rename a variable in php, it will now crash, but this won't be detected with applications (or bundles) with only unit-tests.

dunglas · 2017-01-24T08:34:28Z

@iltar you're right but it's already the case for traditional definitions (if you add or remove an argument it will fail) and for calls definitions (if you rename the method it will fail).

It's why unit testing is not enough and apps need to have a pyramid of tests including some functional and UI ones.

linaori · 2017-01-24T08:38:32Z

@dunglas correct, however, there's a bunch of refactoring tools and find/replaces that might modify the name of a variable. Correctly removing one without causing parse errors is a lot more complex, hence this issue is faster to present itself.

dunglas · 2017-01-24T08:48:26Z

PR updated to make named arguments starting with a $ as suggested by @nicolas-grekas. It solves problems raided by @ogizanagi (no conflict possible now) and partially the one solved by @iltar (the $ makes it explicit that there is a special handling, and IDEs can add support for this new convention to allow fluent refactoring).

nicolas-grekas · 2017-01-24T09:21:03Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php

+        }
+
+        if ($originalArguments !== $arguments) {
+            $value->setArguments($arguments);


ksort($arguments) before set?

nicolas-grekas · 2017-01-24T09:27:45Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

+    private function isUsingShortSyntax(array $service)
+    {
+        foreach ($service as $key => $value) {
+            if (!is_int($key) && '$' !== substr($key, 0, 1)) {


should be an || instead of &&?
I'd write it as if (!isset($key[0]) || '$' !== $key[0]) {

should be an || instead of &&?

No it's correct (only int and keys starting with a $ are allowed when using short syntax)

nicolas-grekas · 2017-01-24T09:28:39Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -245,7 +261,7 @@ private function parseDefinition($id, $service, $file, array $defaults)
            return;
        }

-        if (is_array($service) && array_values($service) === $service) {
+        if (is_array($service) && $this->isUsingShortSyntax($service)) {


note that array_values checked the order of keys, whereas isUsingShortSyntax doesn't anymore, isn't it?

Indeed, is it a problem?

nicolas-grekas · 2017-01-24T10:36:44Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

+    private function isUsingShortSyntax(array $service)
+    {
+        foreach ($service as $key => $value) {
+            if (!is_int($key) && (isset($key[0]) && '$' !== $key[0])) {


the empty string case should also return false, isn't it? thus:
if (is_string($key) && ('' === $key || '$' !== $key[0])) {

Why? If it's an empty, string, it's not a short definition.

that's exactly what I'm saying :)

javiereguiluz · 2017-01-24T15:13:04Z

I don't like the $ at all. It makes the YAML file look a mix of PHP and YAML contents. Example:

arguments:
    $apiKey: "%mandrill_api_key%"

zanbaldwin · 2017-01-24T16:24:13Z

I think the $ is appropriate. It makes it much more obvious that the keys are directly referencing PHP variables/parameters (as defined in the method definition) rather than some non-trivial structure defined by something like the config component.
Making the arguments configuration seem a little less "magic" to newer developers (meaning a lower learning curve) is, to me personally, more important than clean looking configuration files.

fabpot · 2017-02-13T14:33:00Z

@weaverryan Can we close #21376?

fabpot · 2017-02-13T14:37:57Z

@dunglas Looks like another one where a doc PR is needed :)

fabpot · 2017-02-13T14:38:22Z

Thank you @dunglas.

…(dunglas, nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Add support for named arguments | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | todo This PR introduces named arguments for services definitions. It's especially useful to inject parameters in an autowired service. It is (at least partially) an alternative to #21376 and #20738. Usage: ```yml services: _defaults: { autowire: true } Acme\NewsletterManager: { $apiKey: "%mandrill_api_key%" } # Alternative (traditional) syntax services: newsletter_manager: class: Acme\NewsletterManager arguments: $apiKey: "%mandrill_api_key%" autowire: true ``` ```php use Doctrine\ORM\EntityManager; use Psr\Log\LoggerInterface; namespace Acme; class NewsletterManager { private $logger; private $em; private $apiKey; public function __construct(LoggerInterface $logger, EntityManager $em, $apiKey) { $this->logger = $logger; $this->em = $em; $this->apiKey = $apiKey; } } ``` Commits ------- 8a126c8 [DI] Deprecate string keys in arguments 2ce36a6 [DependencyInjection] Add a new pass to check arguments validity 6e50129 [DependencyInjection] Add support for named arguments

weaverryan · 2017-02-13T17:05:03Z

I've at least got a docs issue opened for this! symfony/symfony-docs#7482

... and I closed my other PR - #21376

teohhanhui · 2017-02-23T12:36:26Z

The $ prefix is unfortunate. PHP-ism seeping into YAML.

This breaks on Symfony 3.3.0-Beta1 because of [changes in DI](symfony/symfony#21383 (comment)). Named keys have never been officially supported.

Tobion · 2017-05-31T01:42:28Z

src/Symfony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php

+     *
+     * @throws InvalidArgumentException
+     *
+     * @return array


Should be hinted as \ReflectionParameter[] so the following calls make more sense

This method has been removed during a subsequent refactoring.

Tobion · 2017-05-31T01:46:33Z

src/Symfony/Component/DependencyInjection/ChildDefinition.php

@@ -213,12 +213,14 @@ public function getArgument($index)
     */
    public function replaceArgument($index, $value)


the phpdoc of $index needs to be updated to allow string

also ChildDefinition::getArgument doesn't work with string yet. whereas you changed the parent Definition::getArgument.

@Tobion

…ition (dunglas) This PR was squashed before being merged into the 3.3 branch (closes #22981). Discussion ---------- [DependencyInjection] Fix named args support in ChildDefinition | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Following @Tobion's review of #21383. Commits ------- 1ab3e41 [DependencyInjection] Fix named args support in ChildDefinition

alcohol · 2017-07-27T14:09:35Z

For reasons unknown to me, my currently client had tons of service definitions that looked like:

service.id:
  class: Foo
  arguments:
    foo: something
    bar: somethingelse

<?php

class Foo {
  public function __construct($foo, $bar) { 
    // $foo would be something
    // $bar would be somethingelse
  }
}

All of those broke with the introduction of these changes in 3.3.

Invalid key "X" found in arguments of method "X" for service "X": only integer or $named arguments are allowed.

Was this intentional? Was their use-case undocumented and thus not supported? Cause it did work exactly as expected before 3.3...

linaori · 2017-07-28T06:27:01Z

Afaik, the keys were unused and therefore treated by sequence. Writing it down like this wasn't a supported feature either.

alcohol · 2017-07-28T09:33:38Z

That's what I figured. They were using the keys themselves though in some cases, by retrieving the arguments of the service definition, in some CompilerPass related stuff. Took me a bit to sort everything out 😅

dunglas requested review from weaverryan and nicolas-grekas January 23, 2017 22:38

carsonbot added Status: Needs Review DependencyInjection Feature labels Jan 23, 2017

dunglas mentioned this pull request Jan 23, 2017

[WIP] Annotation support for Services #21376

Closed

nicolas-grekas reviewed Jan 24, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Jan 24, 2017

dunglas force-pushed the di/named_arguments branch from 2e5cabb to 0841285 Compare January 24, 2017 12:13

dunglas added 2 commits February 13, 2017 10:03

[DependencyInjection] Add support for named arguments

6e50129

[DependencyInjection] Add a new pass to check arguments validity

2ce36a6

dunglas force-pushed the di/named_arguments branch from dd345af to 8305385 Compare February 13, 2017 09:04

[DI] Deprecate string keys in arguments

8a126c8

dunglas force-pushed the di/named_arguments branch from 86effb8 to 8a126c8 Compare February 13, 2017 09:06

fabpot merged commit 8a126c8 into symfony:master Feb 13, 2017

weaverryan mentioned this pull request Feb 13, 2017

[DI] Document supplying named args for services symfony/symfony-docs#7482

Closed

sstok mentioned this pull request Apr 19, 2017

[Feat] DI - auto wire container params using annotation #22467

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

alister mentioned this pull request May 2, 2017

Removed unsupported named argument for service azine/AzineMailgunWebhooksBundle#4

Merged

dunglas deleted the di/named_arguments branch May 9, 2017 10:03

dunglas mentioned this pull request May 9, 2017

[DI] Fixed index args bug with ResolveNamedArgumentsPass #22677

Merged

Tobion reviewed May 31, 2017

View reviewed changes

dunglas mentioned this pull request May 31, 2017

[DependencyInjection] Fix named args support in ChildDefinition #22981

Closed

wesleylancel mentioned this pull request Jun 2, 2017

[AdminBundle] Remove key for ConsoleExceptionListener argument Kunstmaan/KunstmaanBundlesCMS#1516

Merged

ogizanagi mentioned this pull request Jun 27, 2017

Autowire error with Symfony Flex when configuring argument by name #23308

Closed

ninsuo mentioned this pull request Jul 14, 2017

Probable invalid DI setup on newer symfony/guzzle hwi/HWIOAuthBundle#1240

Closed

theofidry mentioned this pull request Jul 31, 2017

[RFC] Proposal to simplify the autowiring of container parameters #23718

Closed

		@@ -213,12 +213,14 @@ public function getArgument($index)
		*/
		public function replaceArgument($index, $value)

[DependencyInjection] Add support for named arguments #21383

[DependencyInjection] Add support for named arguments #21383

Conversation

dunglas commented Jan 23, 2017 • edited by nicolas-grekas Loading

nicolas-grekas commented Jan 23, 2017

fabpot commented Jan 23, 2017

ogizanagi commented Jan 23, 2017 • edited Loading

fabpot commented Jan 23, 2017

dunglas commented Jan 23, 2017

dunglas commented Jan 23, 2017 • edited Loading

dunglas commented Jan 23, 2017 • edited Loading

dunglas commented Jan 23, 2017 • edited Loading

dunglas commented Jan 23, 2017

ogizanagi commented Jan 23, 2017 • edited Loading

ogizanagi commented Jan 24, 2017 • edited Loading

nicolas-grekas commented Jan 24, 2017

linaori commented Jan 24, 2017

dunglas commented Jan 24, 2017

linaori commented Jan 24, 2017

dunglas commented Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Jan 24, 2017

zanbaldwin commented Jan 24, 2017

fabpot commented Feb 13, 2017

fabpot commented Feb 13, 2017

fabpot commented Feb 13, 2017

weaverryan commented Feb 13, 2017

teohhanhui commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcohol commented Jul 27, 2017 • edited Loading

linaori commented Jul 28, 2017

alcohol commented Jul 28, 2017

dunglas commented Jan 23, 2017 •

edited by nicolas-grekas

Loading

ogizanagi commented Jan 23, 2017 •

edited

Loading

dunglas commented Jan 23, 2017 •

edited

Loading

dunglas commented Jan 23, 2017 •

edited

Loading

dunglas commented Jan 23, 2017 •

edited

Loading

ogizanagi commented Jan 23, 2017 •

edited

Loading

ogizanagi commented Jan 24, 2017 •

edited

Loading

dunglas commented Jan 24, 2017 •

edited

Loading

nicolas-grekas Jan 24, 2017 •

edited

Loading

alcohol commented Jul 27, 2017 •

edited

Loading