Skip to content

[DI] Fix by-type args injection #24978

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

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

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

Three fixes:

  • arguments should have higher priority than bindings
  • $named arguments should have higher priority than By\TypeHint arguments
  • doubled same-type should work, not only the first should be wired

@sroze I might need your help for adding tests, would be awesome!

@sroze
Copy link
Contributor

sroze commented Nov 16, 2017

@nicolas-grekas can you give me a PHP class example that showcases what you try to solve? I can't reproduce the last issue 🤔

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
@stof
Copy link
Member

stof commented Nov 17, 2017

@nicolas-grekas do we still need the pass order change ?

@keradus
Copy link
Member

keradus commented Nov 19, 2017

any test showing the fix would be nice as well ;)

@nicolas-grekas
Copy link
Member Author

Tests have been added as part of #24991

@nicolas-grekas nicolas-grekas deleted the di-named-types branch November 19, 2017 18:20
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.

5 participants