NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DI] Don't use auto-registered services to populate type-candidates#22254

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:2.8fromnicolas-grekas:di-autow28

Apr 3, 2017

Merged

[DI] Don't use auto-registered services to populate type-candidates#22254

fabpot merged 1 commit intosymfony:2.8fromnicolas-grekas:di-autow28

Apr 3, 2017

Conversation

Copy link

Member

nicolas-grekas commentedApr 3, 2017•
edited
Loading

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

Alternative to#22170 and~~#21665~~.

The core issue fixed here is that auto-registered services shouldnot 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 thetypes 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

nicolas-grekas added this to the2.8 milestone

Apr 3, 2017

nicolas-grekas requested a review fromdunglas

April 3, 2017 15:12

carsonbot added Status: Needs Review DependencyInjection Bug labels

Apr 3, 2017

nicolas-grekas mentioned this pull request

Apr 3, 2017

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

Closed

stof reviewed

Apr 3, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

		}

		// no default value? Then fail
		if (!$parameter->isOptional()) {

Copy link

Member

stofApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

shouldn't this checkisDefaultValueAvailable andallowsNull instead ?

Copy link

MemberAuthor

nicolas-grekasApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I added a check forisDefaultValueAvailable - but none forallowsNull because unless I'm wrong, we don't need it.

Copy link

MemberAuthor

nicolas-grekasApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

and I just reverted this change, because there is nothing wrong here: at this stage, an optional arg must have a default value, (but an arg with a default value is not necessarily optional)
so all good to me as is

Copy link

MemberAuthor

nicolas-grekasApr 3, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the current behavior is a tested one, so I'll adjust on master to fit named args, nothing to do here to me

Copy link

Contributor

theofidry commentedApr 3, 2017

Correct me if I'm wrong, but[DI] Successful construction of DIC depends on order of service definitions when depending on an interface which implementation is auto-registered #22162 has nothing to do with discovered services does it?

auto-registered services should not be used as type-candidates

How does that translate concretely in the end for the users? I'm not sure to understand the real consequences of that :)

Copy link

MemberAuthor

nicolas-grekas commentedApr 3, 2017•
edited
Loading

@theofidry it is directly related:#22162 usesMyRepository as an auto-registered type-hint, which depending on the order of declarations might or might not be used asthe implementation forRepositoryInterface, found in another constructor.

The concrete consequence of this PR is that instead of failing to autowire in some order-related situations, it will make it to always fail (#22170 tries to make it always work, but I'd prefer to get rid of the existing 💩 instead :).)

Copy link

Member

stof commentedApr 3, 2017

@nicolas-grekas I prefer your approach rather than adding more complex magic requiring a 2-step processing of definitions.

nicolas-grekas force-pushed thedi-autow28 branch 2 times, most recently from4a0d0bd to881b2e8Compare

April 3, 2017 16:02

nicolas-grekas mentioned this pull request

Apr 3, 2017

[DI] Reduce complexity of autowiring#22256

Merged

GuilhemN approved these changes

Apr 3, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

		}

		$argumentId =sprintf('autowired.%s',$typeHint->name);
		$this->autowired[$typeHint->name] =$argumentId =sprintf('autowired.%s',$typeHint->name);

Copy link

Contributor

GuilhemNApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can't we populate just one type instead? e.g.$this->types[$typeHint->name] = $argumentId

Copy link

MemberAuthor

nicolas-grekasApr 3, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I looked into this, and didn't find much benefit in doing so, counting LOC - also conceptually that's no exactly the same (as hinted byhttps://github.com/symfony/symfony/pull/22254/files#diff-62df969ae028c559d33ffd256de1ac49R135)

Copy link

Contributor

GuilhemNApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, as there is thefalse support I don't see much benefit too.
Fine for me as is then 👍

GuilhemN reviewed

Apr 3, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

		$arguments =$definition->getArguments();
		foreach ($constructor->getParameters()as$index =>$parameter) {
		foreach ($parametersas$index =>$parameter) {
		if (array_key_exists($index,$arguments) &&'' !==$arguments[$index]) {

Copy link

Contributor

GuilhemNApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder if we actually need this, it makes one more thing to learn while we could achieve the same using manual indexes or named arguments.

Copy link

Member

stofApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

named arguments are a 3.3 feature. This PR targets 2.8. So we need to keep this feature in 2.8

Copy link

MemberAuthor

nicolas-grekasApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this is committed behavior, we can't change now

Copy link

Contributor

GuilhemNApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed, that's a comment for 3.3, I'll open a different PR to discuss this.

Copy link

Member

dunglasApr 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This behavior is still useful in XML (but not in YAML IMO).

stof approved these changes

Apr 3, 2017

View reviewed changes

Copy link

Member

stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

[DI] Don't use auto-registered services to populate type-candidates

992c677

nicolas-grekas force-pushed thedi-autow28 branch from881b2e8 to992c677Compare

April 3, 2017 18:02

dunglas approved these changes

Apr 3, 2017

View reviewed changes

Copy link

Member

dunglas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍, it should have been the default since the beginning. The only thing annoying to me is that this change may break a few apps. Maybe should this "new" behavior mentioned in the CHANGELOG?

Copy link

MemberAuthor

nicolas-grekas commentedApr 3, 2017

Not sure where this should be added. It will be in the changelog as usual of course.

dunglas mentioned this pull request

Apr 3, 2017

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention#22060

Merged

Copy link

Member

fabpot commentedApr 3, 2017

Thank you@nicolas-grekas.

fabpot merged commit992c677 intosymfony:2.8

Apr 3, 2017

fabpot added a commit that referenced this pull request

Apr 3, 2017

bug#22254[DI] Don't use auto-registered services to populate type-c…

ee10bf2

…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

nicolas-grekas deleted the di-autow28 branch

April 4, 2017 08:37

This was referencedApr 5, 2017

Release v2.8.19#22284

Merged

Release v3.2.7#22293

Merged

Copy link

Contributor

enumag commentedApr 6, 2017•
edited
Loading

BC breaks? no (every bug fix is a bc break, isn't it?)

Not entirely sure if it was caused by this change or some other commit but the fact is that in Symfony 3.2.7 autowiring doesn't work for me at all (3.2.6 is fine). In the compiled container many services are created with no arguments at all instead of their autowired dependencies.

Type error: Too few arguments to function AppBundle\Util\Formatter::__construct(), 0 passedin .../app/cache/dev/appDevDebugProjectContainer.php on line 2516 and exactly 3 expected