Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@sroze
Copy link
Contributor

QA
Branch?master
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsø
LicenseMIT
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)

Simperfit reacted with thumbs up emoji
@xabbuhxabbuh added this to the4.0 milestoneNov 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?

@srozesrozeforce-pushed thesingle-typed-argument-can-be-applied-on-multiple-parameters branch from2e11874 tobf7eeefCompareNovember 16, 2017 23:07
@sroze
Copy link
ContributorAuthor

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

@srozesrozeforce-pushed thesingle-typed-argument-can-be-applied-on-multiple-parameters branch from24173cf tod512654CompareNovember 17, 2017 08:42
@sroze
Copy link
ContributorAuthor

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

@stofstof changed the base branch frommaster to3.4November 17, 2017 14:25
@stofstof modified the milestones:4.0,3.4Nov 17, 2017
@stof
Copy link
Member

stof commentedNov 17, 2017•
edited
Loading

Thank you@nicolas-grekas &@sroze.

@stofstof merged commitd512654 intosymfony:3.4Nov 17, 2017
stof added a commit that referenced this pull requestNov 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 typehintedbf7eeef Prove that change is working with tests2176be7 [DI] Fix by-type args injection
This was referencedNov 21, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof approved these changes

+1 more reviewer

@SimperfitSimperfitSimperfit approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@sroze@nicolas-grekas@stof@Simperfit@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp