Skip to content

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

jsamouh
Copy link
Contributor

@jsamouh jsamouh commented Mar 26, 2017

Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22162
License MIT
Doc PR symfony/symfony-docs

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

@jsamouh
Copy link
Contributor Author

jsamouh commented Mar 26, 2017

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
Contributor Author

jsamouh commented Mar 27, 2017

@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

stof commented Mar 28, 2017

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

@jsamouh
Copy link
Contributor Author

jsamouh commented Mar 28, 2017

@stof , right. will fix quickly

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Mar 28, 2017
@nicolas-grekas
Copy link
Member

this should be done (and discussed) on 2.8 first

@jsamouh jsamouh changed the base branch from master to 2.8 March 28, 2017 20:32
@jsamouh
Copy link
Contributor Author

jsamouh commented Mar 28, 2017

@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: MyClass
    autowire: true
class MyClass {
  public function __construct(Interface RepoInterface $RI, Repo $R)
}
class Repo implements 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 and not turn auto-registered services into type-candidates.
See #22254 for such a proposal.

@fabpot
Copy link
Member

fabpot commented Apr 3, 2017

Closing in favor of #22254

@fabpot fabpot closed this Apr 3, 2017
fabpot added a commit that referenced this pull request Apr 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 defaults

Commits
-------

992c677 [DI] Don't use auto-registered services to populate type-candidates
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