Skip to content

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 4, 2017

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

Best reviewed ignoring whitespaces:
https://github.com/symfony/symfony/pull/21873/files?w=1

With this, any "invalid" reference embedded in an autowired definition will trigger the creation of an autoregistered service, in the same way than discovered type hints create them.

This PR provides the same experience than #21859, but in a more focused way, respecting a core principle: if you don't ask explicitly for autowiring, you don't get any :)

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 4, 2017
@nicolas-grekas nicolas-grekas changed the title Di autow refs [DI] Resolve invalid refs embedded in autowired defs by autoregistered services Mar 4, 2017
@nicolas-grekas nicolas-grekas force-pushed the di-autow-refs branch 3 times, most recently from 7415ffc to 208def9 Compare March 5, 2017 00:15
@xabbuh
Copy link
Member

xabbuh commented Mar 5, 2017

I see one inconsistency here which might be confusing: Imagine having two definitions, one being autowired that triggers the auto registration of another service. The second definition is not autowired, but its constructor requires an argument whose type hint matches the note automatically registered definition. Do everything seems to work. But once you remove the autowired definition, the previously registered definition does not exist anymore. Thus letting the container build fail (that's no issue if we don't merge #21871).

@nicolas-grekas
Copy link
Member Author

Is this really related to #21871? I mean: autoregistered services are still valid by-type candidates, thus will trigger the behavior you describe with or without #21871. In fact, is there anything new here? Can't this happen already, with type hints instead of refs?

@nicolas-grekas
Copy link
Member Author

To me, that looks like an acceptable (and already accepted) edge case of autowiring - one situation where you need to make things explicit. Or is there something new I'm missing?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 5, 2017

I'm working today on a better way to provide the target experience - at the loader level.
This PR is still usefull, but should be restricted to typed references as introduced in #21770
Thus, I'm closing here, and merging this one (after a few tweaks), within #21770.
@xabbuh let's continue this discussion on #21770 once I finish updating it, if they still apply there :)

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.

3 participants