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] Added support for variadics in named arguments #24937

Closed
wants to merge 20 commits into
base: master
from

Conversation

@PabloKowalczyk
Contributor

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

This comment has been minimized.

Contributor

iltar commented Nov 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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 13, 2017

Member

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

This comment has been minimized.

@stof

stof Nov 13, 2017

Member

should be removed

@chalasr chalasr added this to the 4.1 milestone Nov 13, 2017

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Nov 13, 2017

@iltar should i open new PR?

@iltar

This comment has been minimized.

Contributor

iltar commented Nov 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.

@PabloKowalczyk PabloKowalczyk changed the base branch from 3.3 to master Nov 14, 2017

@@ -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)) {

This comment has been minimized.

@xabbuh

xabbuh Nov 15, 2017

Member

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

This comment has been minimized.

@xabbuh

xabbuh Nov 15, 2017

Member

not needed

@ro0NL

i think we should also test this in ContainerBuilderTest and PhpDumperTest as well

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Nov 19, 2017

@ro0NL Could you give me more details, please? What methods should i test?

class NamedArgumentsVariadicsDummy
{
public function __construct(...$variadics)

This comment has been minimized.

@sroze

sroze Nov 19, 2017

Member

for the tests, could you add another parameter before the variadic one so we show it works?

@sroze

This comment has been minimized.

Member

sroze commented Nov 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

This comment has been minimized.

Contributor

ro0NL commented Nov 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 👍

@fabpot

fabpot approved these changes Dec 1, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 1, 2017

Thank you @PabloKowalczyk.

@fabpot fabpot closed this Dec 1, 2017

fabpot added a commit that referenced this pull request Dec 1, 2017

feature #24937 [DependencyInjection] Added support for variadics in n…
…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

@PabloKowalczyk PabloKowalczyk deleted the PabloKowalczyk:feature/added-support-for-variadics-in-named-arguments branch Dec 1, 2017

@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