NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DependencyInjection] Fix named args support in ChildDefinition#22981

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

Jump to bottom

Closed

dunglas wants to merge2 commits intosymfony:3.3fromdunglas:named_args_review

Closed

[DependencyInjection] Fix named args support in ChildDefinition#22981

dunglas wants to merge2 commits intosymfony:3.3fromdunglas:named_args_review

Conversation

Copy link

Member

dunglas commentedMay 31, 2017

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.

carsonbot added Status: Needs Review DependencyInjection Bug labels

May 31, 2017

[DependencyInjection] Fix named args support in ChildDefinition

70105d1

dunglas force-pushed thenamed_args_review branch fromec191ba to70105d1Compare

May 31, 2017 06:53

xabbuh mentioned this pull request

May 31, 2017

Exception thrown right after updating to 3.3.0: Invalid key "X" found in arguments of method "__construct()", only integer or $named arguments are allowed#22979

Closed

xabbuh added this to the3.3 milestone

May 31, 2017

Copy link

Member

xabbuh commentedMay 31, 2017

I would also update the exception message. "$index must be an integer." is not really right anymore.

dunglas force-pushed thenamed_args_review branch from5881fea to9245400Compare

May 31, 2017 11:10

Copy link

MemberAuthor

dunglas commentedMay 31, 2017

@xabbuh updated,ChildDefinition now delegates the common behavior to the parent class.

Copy link

Member

xabbuh commentedMay 31, 2017

@dunglas I meant the one inreplaceArgument():https://github.com/dunglas/symfony/blob/9245400aa0a3467d89565ecdda8bd1d5315bbe21/src/Symfony/Component/DependencyInjection/ChildDefinition.php#L102

Update error message

91985fb

dunglas force-pushed thenamed_args_review branch from9245400 to91985fbCompare

May 31, 2017 12:15

Copy link

MemberAuthor

dunglas commentedMay 31, 2017

@xabbuh updated

snamor mentioned this pull request

May 31, 2017

Error with Symfony 3.3 and SQS providerRiverline/worker-bundle#6

Closed

Copy link

Contributor

Tobion commentedMay 31, 2017

The exception message inDefinition::replaceArgument

The index "%d" is not in the range [0, %d]

is also not correct anymore in case an argument exists as a string. Then you can't actually access the argument with the given range.

Copy link

yukoff commentedMay 31, 2017

Seems false alarm, I'll remove confusing comment to avoid indexing and leading others to wrong place ;)

Copy link

yukoff commentedMay 31, 2017

Despitemy false alarm in#22979 ;) still doesn't work with PR applied locally:

PHP Fatal error:  Uncaught Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Invalid key "collaboration" found in arguments of method "__construct()" for service "sulu_admin.js_config_pool": only integer or $named arguments are allowed. in D:\sandbox\vendor\symfony\symfony\src\Symfony\Component\DependencyInjection\Compiler\ResolveNamedArgumentsPass.php:47Stack trace:#0 D:\sandbox\vendor\symfony\symfony\src\Symfony\Component\DependencyInjection\Compiler\AbstractRecursivePass.php(57): Symfony\Component\DependencyInjection\Compiler\ResolveNamedArgumentsPass->processValue(Object(Symfony\Component\DependencyInjection\Definition), false)#1 D:\sandbox\vendor\symfony\symfony\src\Symfony\Component\DependencyInjection\Compiler\ResolveNamedArgumentsPass.php(30): Symfony\Component\DependencyInjection\Compiler\AbstractRecursivePass->processValue(Array, false)#2 D:\sandbox\vendor\symfony\sym in D:\sandbox\vendor\symfony\symfony\src\Symfony\Component\DependencyInjection\Compiler\ResolveNamedArgumentsPass.php on line 47

Service description is quite trivial

        <serviceid="sulu_admin.js_config_pool"class="%sulu_admin.js_config_pool.class%"/>

nicolas-grekas approved these changes

Jun 2, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

Copy link

Member

nicolas-grekas commentedJun 2, 2017

@yukoff named args must start with a$

Copy link

yukoff commentedJun 2, 2017•
edited
Loading

My previous comment is not actual as ofsulu/sulu#3379 - works now.

@nicolas-grekas not quite sure I got to where you're referencing... ;)

Copy link

Member

nicolas-grekas commentedJun 2, 2017

I'm referencing the error message you pasted just above: it looks legit to me.

Copy link

yukoff commentedJun 2, 2017

Oh... Anyway it seems not actual any more, thanks!

nicolas-grekas added the Ready label

Jun 5, 2017

Copy link

Member

fabpot commentedJun 5, 2017

Thank you@dunglas.

fabpot added a commit that referenced this pull request

Jun 5, 2017

bug#22981[DependencyInjection] Fix named args support in ChildDefin…

1272d2a

…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 <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aFollowing@Tobion's review of#21383.Commits-------1ab3e41 [DependencyInjection] Fix named args support in ChildDefinition

fabpot closed this

Jun 5, 2017

fabpot mentioned this pull request

Jun 5, 2017

Release v3.3.1#23068

Merged