Skip to content

[DependencyInjection] Single typed argument can be applied on multiple parameters #24991

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

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Nov 16, 2017

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ø
License MIT
Doc PR ø

I'm @nicolas-grekas' test writer today. This makes the argument resolution working when injecting the same type multiple times (sub-set of PR #24978)

@xabbuh xabbuh added this to the 4.0 milestone Nov 16, 2017
@nicolas-grekas
Copy link
Member

thank you so much @sroze
would be great to have another test tat ensures that the named arg wins when doing something like
$definition->setArguments(array(CaseSensitiveClass::class => new Reference('foo'), '$class1' => ...);

@nicolas-grekas
Copy link
Member

do you think you coud also include the patch on PassConfig on # #24978, and add a test in ContainerBuilderTest that ensures that named arguments win over bindings?

@sroze sroze force-pushed the single-typed-argument-can-be-applied-on-multiple-parameters branch from 2e11874 to bf7eeef Compare November 16, 2017 23:07
@sroze
Copy link
Contributor Author

sroze commented Nov 16, 2017

@nicolas-grekas good point, I've added the test proving the named vs the typed. But I think the priority of the passes should be in another PR, will issue another tomorrow.

@sroze sroze force-pushed the single-typed-argument-can-be-applied-on-multiple-parameters branch from 24173cf to d512654 Compare November 17, 2017 08:42
@sroze
Copy link
Contributor Author

sroze commented Nov 17, 2017

@nicolas-grekas actually needed this new test fixture class so added the test here as well. It turns out that we don't need the pass order change.

@stof stof changed the base branch from master to 3.4 November 17, 2017 14:25
@stof stof modified the milestones: 4.0, 3.4 Nov 17, 2017
@stof
Copy link
Member

stof commented Nov 17, 2017

Thank you @nicolas-grekas & @sroze.

@stof stof merged commit d512654 into symfony:3.4 Nov 17, 2017
stof added a commit that referenced this pull request Nov 17, 2017
… on multiple parameters (nicolas-grekas, sroze)

This PR was merged into the 3.4 branch.

Discussion
----------

[DependencyInjection] Single typed argument can be applied on multiple parameters

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ø
| License       | MIT
| Doc PR        | ø

I'm @nicolas-grekas' test writer today. This makes the argument resolution working when injecting the same type multiple times (sub-set of PR #24978)

Commits
-------

d512654 Test that named arguments are prioritized over typehinted
bf7eeef Prove that change is working with tests
2176be7 [DI] Fix by-type args injection
This was referenced Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants