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

nicolas-grekas · 2017-04-03T15:12:15Z

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

stof · 2017-04-03T15:17:01Z

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

+                    if (isset($arguments[$index])) {
+                        continue;
+                    }
+
                    // no default value? Then fail
                    if (!$parameter->isOptional()) {


shouldn't this check isDefaultValueAvailable and allowsNull instead ?

I added a check for isDefaultValueAvailable - but none for allowsNull because unless I'm wrong, we don't need it.

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

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

theofidry · 2017-04-03T15:31:59Z

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 :)

nicolas-grekas · 2017-04-03T15:39:40Z

@theofidry it is directly related: #22162 uses MyRepository as an auto-registered type-hint, which depending on the order of declarations might or might not be used as the implementation for RepositoryInterface, 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 :).)

stof · 2017-04-03T15:49:49Z

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

GuilhemN · 2017-04-03T16:43:01Z

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

@@ -252,13 +262,11 @@ private function createAutowiredDefinition(\ReflectionClass $typeHint, $id)
            throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". No services were found matching this %s and it cannot be auto-registered.', $typeHint->name, $id, $classOrInterface));
        }

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


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

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 by https://github.com/symfony/symfony/pull/22254/files#diff-62df969ae028c559d33ffd256de1ac49R135)

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

GuilhemN · 2017-04-03T16:48:16Z

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


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


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.

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

this is committed behavior, we can't change now

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

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

stof

👍

dunglas

👍, 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?

nicolas-grekas · 2017-04-03T20:56:12Z

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

fabpot · 2017-04-03T22:10:08Z

Thank you @nicolas-grekas.

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

enumag · 2017-04-06T10:31:47Z

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 passed
in .../app/cache/dev/appDevDebugProjectContainer.php on line 2516 and exactly 3 expected

nicolas-grekas · 2017-04-11T15:12:33Z

Last comment fixed in #22311

nicolas-grekas added this to the 2.8 milestone Apr 3, 2017

nicolas-grekas requested a review from dunglas 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

nicolas-grekas force-pushed the di-autow28 branch 2 times, most recently from 4a0d0bd to 881b2e8 Compare 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

GuilhemN reviewed Apr 3, 2017

View reviewed changes

stof approved these changes Apr 3, 2017

View reviewed changes

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

992c677

nicolas-grekas force-pushed the di-autow28 branch from 881b2e8 to 992c677 Compare April 3, 2017 18:02

dunglas approved these changes Apr 3, 2017

View reviewed changes

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

fabpot merged commit 992c677 into symfony:2.8 Apr 3, 2017

nicolas-grekas deleted the di-autow28 branch April 4, 2017 08:37

This was referenced Apr 5, 2017

Release v2.8.19 #22284

Merged

Release v3.2.7 #22293

Merged

nicolas-grekas mentioned this pull request Apr 6, 2017

Autowiring is not working properly in Symfony 3.2.7 #22309

Closed

weaverryan mentioned this pull request Apr 6, 2017

[DI] Fix second auto-registration #22311

Merged

TomasVotruba mentioned this pull request Jun 8, 2017

[DI] use Symfony and PSR-11 container ApiGen/ApiGen#880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nicolas-grekas commented Apr 3, 2017 •

edited

Loading

stof Apr 3, 2017

nicolas-grekas Apr 3, 2017

nicolas-grekas Apr 3, 2017

nicolas-grekas Apr 3, 2017 •

edited

Loading

theofidry commented Apr 3, 2017

nicolas-grekas commented Apr 3, 2017 •

edited

Loading

stof commented Apr 3, 2017

GuilhemN Apr 3, 2017

nicolas-grekas Apr 3, 2017 •

edited

Loading

GuilhemN Apr 3, 2017

GuilhemN Apr 3, 2017

stof Apr 3, 2017

nicolas-grekas Apr 3, 2017

GuilhemN Apr 3, 2017

dunglas Apr 3, 2017

stof left a comment

dunglas left a comment •

edited

Loading

nicolas-grekas commented Apr 3, 2017

fabpot commented Apr 3, 2017

enumag commented Apr 6, 2017 •

edited

Loading

nicolas-grekas commented Apr 11, 2017

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

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

Conversation

nicolas-grekas commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

theofidry commented Apr 3, 2017

nicolas-grekas commented Apr 3, 2017 • edited Loading

stof commented Apr 3, 2017

Choose a reason for hiding this comment

nicolas-grekas Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof left a comment

Choose a reason for hiding this comment

dunglas left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Apr 3, 2017

fabpot commented Apr 3, 2017

enumag commented Apr 6, 2017 • edited Loading

nicolas-grekas commented Apr 11, 2017

nicolas-grekas commented Apr 3, 2017 •

edited

Loading

nicolas-grekas Apr 3, 2017 •

edited

Loading

nicolas-grekas commented Apr 3, 2017 •

edited

Loading

nicolas-grekas Apr 3, 2017 •

edited

Loading

dunglas left a comment •

edited

Loading

enumag commented Apr 6, 2017 •

edited

Loading