Skip to content

[DI] Remove skipping magic for autowired methods #22030

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
Merged
merged 1 commit into from
Mar 19, 2017
Merged

[DI] Remove skipping magic for autowired methods #22030

merged 1 commit into from
Mar 19, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 17, 2017 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no (master only)
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Wildcard based autowiring made it required to auto-skip methods that were not wireable.
Now that things need to be explicit (ie via the @required annotation, or via configuration), this "automagic" behavior is not required anymore.
Since it can lead to wtf moments ("I did put that @required annotation, why is it ignored by autowiring?"), I think we should remove it.
This also fixes another issue where configured method calls had their optional arguments wired, while we want only the constructor's to behave as such.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 17, 2017
@weaverryan
Copy link
Member

For reference, here are the original decisions we made about how to try to autowire setter methods, but not fail as often / consistently when compared with constructor autowiring - items 2, 3 and 4 in my comment: #17608 (comment)

I think this PR is wonderful! The @required completely removes the need for us to try to make this decision for the user.

@nicolas-grekas the only thing I noticed when trying this was that if I had @required above a method with zero arguments, that method wasn't called. With this philosophy, it now feels like this method should also be called. However, if I have a method with a single optional argument that cannot be autowired, it feels like that one should not be called (which is how it works now). But those are kind of the same situation (i.e. should we call a method when we have nothing to pass to it), so I'm not totally sure :).

Thanks!

@fabpot
Copy link
Member

fabpot commented Mar 17, 2017

@weaverryan As @required is added by the user, he knows what he wants. So to me, the methods marked with @required must always be called, regardless of their arguments.

@weaverryan
Copy link
Member

Nicolas and I were talking off-GitHub, and we agree - we should always call methods with @required, unless they cannot be called due to non-autowireable args (then we should throw an exception).

What I forgot was that a method with no arguments is getter-injected. In other words, it's incorrect for me to assume that I could add @required above a method with no args (e.g. configure()) and expect it to be called. This is a little magic - that we're deciding between setter and getter injection based on the presence/absence of at least one arg -, but it seems like it needs to be done that way (and Nicolas and I discussed how we should perhaps throw an exception if there is an @required method with no args, but also not return type - so at least the user will get a clear error).

@fabpot
Copy link
Member

fabpot commented Mar 17, 2017

I think throwing an exception when there is a @required annotation without arguments is a good idea indeed.

@nicolas-grekas
Copy link
Member Author

Exceptions added, all code related to skipping logic removed.
I also reverted #21602: abstract getter don't need to be autowired anymore, we'd better encourage using the annotation instead.

fabpot
fabpot reviewed Mar 18, 2017
View reviewed changes
src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
$isConstructor = $reflectionMethod->isConstructor();

if (!$isConstructor && !$arguments && !$reflectionMethod->getNumberOfRequiredParameters()) {
throw new RuntimeException(sprintf('Cannot autowire service "%s": method %s::%s() has only optional arguments, thus must be wired explicitly.', $this->currentId, $reflectionMethod->class, $reflectionMethod->name));
Copy link
Member

Choose a reason for hiding this comment

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

This can also be hit when there is no args at all, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

a method with no argument at all is considered as a getter, thus can't hit this method, see
https://github.com/symfony/symfony/pull/22030/files#diff-62df969ae028c559d33ffd256de1ac49R219

@fabpot
Copy link
Member

fabpot commented Mar 18, 2017

minor comment, 👍

@nicolas-grekas nicolas-grekas merged commit a6bfe1c into symfony:master Mar 19, 2017
nicolas-grekas added a commit that referenced this pull request Mar 19, 2017
@nicolas-grekas
feature #22030 [DI] Remove skipping magic for autowired methods (nico…
e41064f 
…las-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Remove skipping magic for autowired methods

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (master only)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Wildcard based autowiring made it required to auto-skip methods that were not wireable.
Now that things need to be explicit (ie via the `@required` annotation, or via configuration), this "automagic" behavior is not required anymore.
Since it can lead to wtf moments ("*I* did *put that `@required` annotation, why is it ignored by autowiring?*"), I think we should remove it.
This also fixes another issue where configured method calls had their optional arguments wired, while we want only the constructor's to behave as such.

Commits
-------

a6bfe1c [DI] Remove skipping magic for autowired methods
@nicolas-grekas nicolas-grekas deleted the di-autow-fix branch March 19, 2017 21:45
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 21, 2017
Merged
nicolas-grekas added a commit that referenced this pull request Mar 21, 2017
@nicolas-grekas
bug #22084 [DI] Restore skipping logic when autowiring getters (nicol…
6643fe2 
…as-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Restore skipping logic when autowiring getters

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Partial revert for #22030: the skipping logic is part of the getter injection experience, which provides laziness, thus shouldn't bother you until you actually call the getter, if you do.

Commits
-------

b3f494f [DI] Restore skipping logic when autowiring getters
@fabpot fabpot mentioned this pull request May 1, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot left review comments

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolas-grekas @weaverryan @fabpot @carsonbot