Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[DependencyInjection] Fix named args support in ChildDefinition #22981

wants to merge 2 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented May 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.

@dunglas dunglas force-pushed the named_args_review branch from ec191ba to 70105d1 Compare May 31, 2017 06:53
@xabbuh xabbuh mentioned this pull request May 31, 2017
Closed
@xabbuh xabbuh added this to the 3.3 milestone May 31, 2017
@xabbuh
Copy link
Member

xabbuh commented May 31, 2017

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

@dunglas dunglas force-pushed the named_args_review branch from 5881fea to 9245400 Compare May 31, 2017 11:10
@dunglas
Copy link
Member Author

dunglas commented May 31, 2017

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

@xabbuh
Copy link
Member

xabbuh commented May 31, 2017

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

@dunglas dunglas force-pushed the named_args_review branch from 9245400 to 91985fb Compare May 31, 2017 12:15
@dunglas
Copy link
Member Author

dunglas commented May 31, 2017

@xabbuh updated

@snamor snamor mentioned this pull request May 31, 2017
Closed
@Tobion
Copy link
Contributor

Tobion commented May 31, 2017

The exception message in Definition::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.

@yukoff
Copy link

yukoff commented May 31, 2017

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

@yukoff
Copy link

yukoff commented May 31, 2017

Despite my 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:47
Stack 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

        <service id="sulu_admin.js_config_pool" class="%sulu_admin.js_config_pool.class%"/>

nicolas-grekas
nicolas-grekas approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas 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.

👍

@nicolas-grekas
Copy link
Member

@yukoff named args must start with a $

@yukoff
Copy link

yukoff commented Jun 2, 2017 •
edited
Loading

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

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

@nicolas-grekas
Copy link
Member

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

@yukoff
Copy link

yukoff commented Jun 2, 2017

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

@fabpot
Copy link
Member

fabpot commented Jun 5, 2017

Thank you @dunglas.

fabpot added a commit that referenced this pull request Jun 5, 2017
@fabpot
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/a

Following @Tobion's review of #21383.

Commits
-------

1ab3e41 [DependencyInjection] Fix named args support in ChildDefinition
@fabpot fabpot closed this Jun 5, 2017
@fabpot fabpot mentioned this pull request Jun 5, 2017
Merged
@ninsuo ninsuo mentioned this pull request Jul 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection Ready Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
7 participants
@dunglas @xabbuh @Tobion @yukoff @nicolas-grekas @fabpot @carsonbot