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

[DI] [CompilerPass] [AutoWire] : fix order service definition problem#22170

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

Closed
jsamouh wants to merge1 commit intosymfony:2.8fromjsamouh:ticket_22162

Conversation

@jsamouh
Copy link
Contributor

@jsamouhjsamouh commentedMar 26, 2017
edited
Loading

QA
Branch?2.8
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22162
LicenseMIT
Doc PRsymfony/symfony-docs

This is a PR proposal to fix service order problem definition during AutoWirePass.

@jsamouh
Copy link
ContributorAuthor

The principle:

  • Let AutowirePass makes his stuff and logs in an array parameters not found for autowire reference.
  • Browse the array above to try a second time cause all the service have been analysed ,created by AutowirePass
  • If the array still have values, it really means there is a problem

Unit test fails. I will check why but normally, this proposing code has no effect on the existing behavior and fix the problem announced in the issue.

WDYT ?

@jsamouh
Copy link
ContributorAuthor

@dunglas , WDYT about#22162 and this PR ? not sure it's the right way to take or if it's really useful to do it

@stof
Copy link
Member

Not having any test covering this issue makes it impossible to accept this PR

@jsamouh
Copy link
ContributorAuthor

@stof , right. will fix quickly

@nicolas-grekasnicolas-grekas added this to the2.8 milestoneMar 28, 2017
@nicolas-grekas
Copy link
Member

this should be done (and discussed) on 2.8 first

@jsamouhjsamouh changed the base branch frommaster to2.8March 28, 2017 20:32
@jsamouh
Copy link
ContributorAuthor

jsamouh commentedMar 28, 2017
edited by stof
Loading

@nicolas-grekas,@stof , rebase to 2.8 done.
Test unit pass + 2 tests cases of the current issue#22162 added in test unit pass.

First Point:
Development of the method getPriorityToInstantiableParameters. put a priority order to instantiable parameters. With this, you can create a service autowired like this

service_test:class:MyClassautowire:true
class MyClass {publicfunction__construct(InterfaceRepoInterface$RI,Repo$R)}class Repoimplements RepoInterface {}

It treats$R before$RI so it create the autowire service Repo and can deal the parameter$RI

Second Point:
To deal the problem of service definition order discussed in#22162
I created an array of failed checking parameters definitions.
I make a second pass in the process function to make sure that the service has not been created after. If the second pass fails, I throw an exception as before

Tell me WDYT

@nicolas-grekas
Copy link
Member

I think we should be stricter here andnot turn auto-registered services into type-candidates.
See#22254 for such a proposal.

@fabpot
Copy link
Member

Closing in favor of#22254

@fabpotfabpot closed thisApr 3, 2017
fabpot added a commit that referenced this pull requestApr 3, 2017
…andidates (nicolas-grekas)This PR was merged into the 2.8 branch.Discussion----------[DI] Don't use auto-registered services to populate type-candidates| Q             | A| ------------- | ---| Branch?       | 2.8| Bug fix?      | yes| New feature?  | no| BC breaks?    | no (every bug fix is a bc break, isn't it?)| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#22162, ~~#21658~~| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->Alternative to#22170 and ~~#21665~~.The core issue fixed here is that auto-registered services should *not* be used as type-candidates.The culprit is this line:`$this->populateAvailableType($argumentId, $argumentDefinition);`Doing so creates a series of wtf-issues (the linked ones), with no simple fixes (the linked PRs are just dealing with the simplest cases, but the real fix would require a reboot of autowiring for every newly discovered types.)The other changes accommodate for the removal of the population of the `types` property, and fix a few other issues found along the way:- variadic constructors- empty strings injection- tail args removal when they match the existing defaultsCommits-------992c677 [DI] Don't use auto-registered services to populate type-candidates
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees

No one assigned

Projects

None yet

Milestone

2.8

Development

Successfully merging this pull request may close these issues.

5 participants

@jsamouh@stof@nicolas-grekas@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp