Skip to content

[DI] Deprecate/remove autowiring "auto-create" Definition functionality #23350

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.

Sign up for GitHub

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

Jump to bottom
Closed
weaverryan opened this issue Jul 2, 2017 · 7 comments
Closed

[DI] Deprecate/remove autowiring "auto-create" Definition functionality #23350

weaverryan opened this issue Jul 2, 2017 · 7 comments
Labels
DependencyInjection Deprecation
Milestone
3.4

Comments

@weaverryan
Copy link
Member

Q A
Bug report? No
Feature request? No
BC Break report? no
RFC? No
Symfony version 3.4.0

The current autowiring behavior is:

If there is not a service whose id exactly matches the type AND there are 0 services in the container that have the type, a new, private, autowired service is auto-registered in the container and used for that argument.

In #22295, @nicolas-grekas proposed removing this. But, we decided not to, because it meant that all services would need to be explicitly wired. But with the PSR-4 service loader, that's not true anymore.

We should deprecate this "auto-registration" functionality from AutowirePass (

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

Lines 422 to 461 in d7992dc

/**
* Registers a definition for the type if possible or throws an exception.
*
* @param string $type
*
* @return TypedReference|null A reference to the registered definition
*/
private function createAutowiredDefinition($type)
{
if (!($typeHint = $this->container->getReflectionClass($type)) || !$typeHint->isInstantiable()) {
return;
}
$currentId = $this->currentId;
$this->currentId = $type;
$this->autowired[$type] = $argumentId = sprintf('autowired.%s', $type);
$argumentDefinition = new Definition($type);
$argumentDefinition->setPublic(false);
$argumentDefinition->setAutowired(true);
try {
$originalThrowSetting = $this->throwOnAutowiringException;
$this->throwOnAutowiringException = true;
$this->processValue($argumentDefinition, true);
$this->container->setDefinition($argumentId, $argumentDefinition);
} catch (AutowiringFailedException $e) {
$this->autowired[$type] = false;
$this->lastFailure = $e->getMessage();
$this->container->log($this, $this->lastFailure);
return;
} finally {
$this->throwOnAutowiringException = $originalThrowSetting;
$this->currentId = $currentId;
}
$this->container->log($this, sprintf('Type "%s" has been auto-registered for service "%s".', $type, $this->currentId));
return new TypedReference($argumentId, $type);
}
) and throw an exception in 4.0.

This "auto-registration" in AutowirePass is problematic because we can't apply instanceof to those definitions... which is a bit unexpected. Removing it also simplifies the autowiring logic: there's one less case to explain. I chatted with @dunglas about this already (https://twitter.com/dunglas/status/881557361042894849).
@nicolas-grekas
Copy link
Member

👍

2 similar comments
@dunglas
Copy link
Member

dunglas commented Jul 3, 2017

👍

@xabbuh
Copy link
Member

xabbuh commented Jul 3, 2017

👍

@stof
Copy link
Member

stof commented Jul 3, 2017

Yeah, it also creates WTF debugging moments, when you get error messages related to an autocreated services sometimes while you expected your own service: #23325

👍 from me.

@fabpot
Copy link
Member

fabpot commented Jul 3, 2017

@weaverryan I think you can safely work on a PR :)

@weaverryan
Copy link
Member Author

Haha, indeed... except I was hoping someone else would beat me to it ;). But if not, I'll get to it!

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Jul 11, 2017 •
edited
Loading

This gave me some WTF moments, so 👍 PSR-4 is just fine

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 28, 2017
@nicolas-grekas nicolas-grekas mentioned this issue Jul 28, 2017
Closed
@GuilhemN GuilhemN mentioned this issue Jul 29, 2017
Closed
nicolas-grekas added a commit that referenced this issue Aug 3, 2017
@nicolas-grekas
feature #23712 [DependencyInjection] Deprecate autowiring service aut…
2454a4f 
…o-registration (GuilhemN)

This PR was squashed before being merged into the 3.4 branch (closes #23712).

Discussion
----------

[DependencyInjection] Deprecate autowiring service auto-registration

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | yes <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #23350
| License       | MIT
| Doc PR        |

Fix #23350, to make autowiring more predictable.

Commits
-------

969a207 [DependencyInjection] Deprecate autowiring service auto-registration
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this issue Aug 3, 2017
@nicolas-grekas
feature #23712 [DependencyInjection] Deprecate autowiring service aut…
6a81578 
…o-registration (GuilhemN)

This PR was squashed before being merged into the 3.4 branch (closes #23712).

Discussion
----------

[DependencyInjection] Deprecate autowiring service auto-registration

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | yes <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#23350
| License       | MIT
| Doc PR        |

Fix symfony/symfony#23350, to make autowiring more predictable.

Commits
-------

969a207 [DependencyInjection] Deprecate autowiring service auto-registration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DependencyInjection Deprecation
Projects
None yet
Milestone
3.4
Development

No branches or pull requests
8 participants
@fabpot @dunglas @javiereguiluz @weaverryan @nicolas-grekas @stof @TomasVotruba @xabbuh