[DependencyInjection] Added support for variadics in named arguments #24937
Conversation
PabloKowalczyk
commented
Nov 12, 2017
•
| Q | A |
|---|---|
| Branch? | 4.1 |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #24935 |
| License | MIT |
|
As this is a new feature, it should target the master branch (which is 4.1). 3.4 is closed for new features and you're currently targeting 3.3, which will only receive bug fixes. |
| method_exists('ReflectionParameter', 'isVariadic') | ||
| ) { | ||
| try { | ||
| $reflection = $this->getReflectionMethod($value, $method); |
There was a problem hiding this comment.
I think a better implementation would hook into the existing foreach loop on line 56-57.
We just need a if (\PHP_VERSION_ID >= 50600 && $p->isVariadic() check inside the loop.
No need for enforcing this is the "last argument", PHP already did so.
And the key of the added params must be explicitly set ($resolvedArguments[$j++] instead of $resolvedArguments[], which can be wrong)
| @@ -0,0 +1,12 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
|
@iltar should i open new PR? |
|
@PabloKowalczyk you'll have to change your branch to be based on the master, both locally and in in this PR, no need to create a new PR. |
013f7fd to
0219b3b
Compare
| @@ -55,7 +55,13 @@ protected function processValue($value, $isRoot = false) | |||
| if (isset($key[0]) && '$' === $key[0]) { | |||
| foreach ($parameters as $j => $p) { | |||
| if ($key === '$'.$p->name) { | |||
| $resolvedArguments[$j] = $argument; | |||
| if (\PHP_VERSION_ID >= 50600 && $p->isVariadic() && \is_array($argument)) { | |||
There was a problem hiding this comment.
Symfony 4.1 will require PHP 7. So this can be simplified.
| @@ -125,6 +126,37 @@ public function testTypedArgument() | |||
|
|
|||
| $this->assertEquals(array(new Reference('foo'), '123'), $definition->getArguments()); | |||
| } | |||
|
|
|||
| /** | |||
| * @requires PHP 5.6 | |||
|
@ro0NL Could you give me more details, please? What methods should i test? |
|
|
||
| class NamedArgumentsVariadicsDummy | ||
| { | ||
| public function __construct(...$variadics) |
There was a problem hiding this comment.
for the tests, could you add another parameter before the variadic one so we show it works?
|
@ro0NL what exactly are you willing to test? I'm not convinced yet of the value of more tests for these changes :) |
|
in general, you would test the usecase from a compiled/dumped container; asserting a real constructed service instead of a definition. But i agree the current test implies it should work 👍 |
9658d69 to
27754ef
Compare
…use NamedArgumentsDummy to this test.
27754ef to
ca5453e
Compare
|
Thank you @PabloKowalczyk. |
…amed arguments (PabloKowalczyk) This PR was squashed before being merged into the 4.1-dev branch (closes #24937). Discussion ---------- [DependencyInjection] Added support for variadics in named arguments | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24935 | License | MIT Commits ------- b5c0e89 [DependencyInjection] Added support for variadics in named arguments