Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Added support for variadics in named arguments#24937
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
linaori commentedNov 13, 2017
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think a better implementation would hook into the existing foreach loop on line 56-57.
We just need aif (\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); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed
PabloKowalczyk commentedNov 13, 2017
@iltar should i open new PR? |
linaori commentedNov 14, 2017
@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 to0219b3bCompare| foreach ($parametersas$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Symfony 4.1 will require PHP 7. So this can be simplified.
| } | ||
| /** | ||
| * @requires PHP 5.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not needed
ro0NL left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i think we should also test this inContainerBuilderTest andPhpDumperTest as well
PabloKowalczyk commentedNov 19, 2017
@ro0NL Could you give me more details, please? What methods should i test? |
| class NamedArgumentsVariadicsDummy | ||
| { | ||
| publicfunction__construct(...$variadics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
for the tests, could you add another parameter before the variadic one so we show it works?
sroze commentedNov 19, 2017
@ro0NL what exactly are you willing to test? I'm not convinced yet of the value of more tests for these changes :) |
ro0NL commentedNov 19, 2017
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 to27754efCompare…use NamedArgumentsDummy to this test.
27754ef toca5453eComparefabpot commentedDec 1, 2017
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 | MITCommits-------b5c0e89 [DependencyInjection] Added support for variadics in named arguments
Uh oh!
There was an error while loading.Please reload this page.