[BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services #22295
Conversation
d14cdca to
f190182
Compare
|
👍 |
|
It breaks the main idea of autowiring and the DX improvement it provides: currently: create a Constructing automatically the object graph is what make autowiring amazing to use. But maybe am I missing something? |
|
The list of pros is quite sexy to me, fixes lots of code smell (eg #22282 being the last one.) On the DX side, it feels really good: Yes, "by-id" autowiring will require you to add explicit aliases/services for the services in your namespace. But most bundles will ship with default aliases that will just work (as already done by SF core). For those services in your namespace, the DX is really improved vs full manual wiring:
So, DX wise, "by-id" autowiring is "compiler-assisted explicit wiring". I already experienced it, and I want it everywhere :)
To me, the most important promise of autowiring is: tell me you need a LoggerInterface, I'll ship you one.
It feels it works, but it can also break, because of ambiguities, which are waiting around the corner. Then DX is badly hurt.
That would certainly be a bad idea indeed! |
I'm not arguing against the What I want when using autowiring is to only write PHP. It's basically a tool for prototyping and RAD. What I want is creating a Symfony app as fast possible without having to write config files. The steps you describe are really interesting in some cases, but it breaks this "config-less" experience we already have (when using ActionBundle fi). IMO it's a regression. Having both modes is better because it lets the developer choose how he want to code its app instead of forcing it to do some work that is currently not necessary.
Yes, and it's intended when using the standard autowiring. It cannot breaks at runtime, only at compile time, and when it breaks, it's time to move to explicit (or - now - Btw, I would suggest to change |
|
Big 👍 for no-questions-asked config, i.e. Given that.. we could still improve a single type match right, creating the alias automatically when |
|
@nicolas-grekas if our typehint is a class rather than an interface, would it still require registering a service explicitly ? This is what makes the experience harder for simple cases like the Slugger above. All your explanations about by-id wiring always talk abut interface typehints. @ro0NL creating the alias automatically when there is no ambiguity is what happens in the by-type matching (among other things). And this is what breaks often: as soon as you register a second service matching the type (for a totally unrelated feature), the automatically-added alias is not added anymore and things blow up. And this second service might come from elsewhere than your own code (adding a new bundle, etc...). |
Yes; in case we fallback to the process described by @nicolas-grekas #22295 (comment) I understand it sort of brings back the original issue; but on the other hand, it's truly convenient :) most project src dirs i've seen practically contain nothing but unique type services :) |
|
@ro0NL it's why I'm against dropping the original feature :) |
|
Then again... Guess letting the user updating some service id to a FQCN is a better approach, compared to SF creating an alias for your "FQCN to id convention".. which i've also seen thousands of :) causing very cluttered configs. |
yes, that's what I mean by being side-effect free (and call "auto-registration" this "by-type" behavior). take this We could implement something in between if this is the corner stone of the discussion.
That would not be a strict as I'd like it to be - but at least that would fix most of the issues we have with "by-type" (but it would still be "magic" by requiring some container-wide introspection - meaning will lead to unwanted side-effects at some point). |
Give me some time to double-think about it, but it looks like a nice compromise. Will this behavior be recursive (e.g, will FooController -> MyApiClient -> MyRestClient works with no config if there is only 1 type available in the hierarchy for each class)? |
|
As I see it, this would be recursive yes, same as today (ie these would private + autowired). |
|
Oh, I see just one difference that I'd like to put in the compromise: as soon as a type is optional, we should not auto-register. |
|
I'd always use explicit binding aka current |
Ok for me, if there is no alias defined,
I agree, and we need to make it clear in the doc that autowiring is for simple cases only. For advanced uses, you must use explicit wiring (this feature as always been designed with this in mind). |
f190182 to
4631ff8
Compare
|
PR updated, autoregistration is back, with the following logic:
|
4631ff8 to
f2fed47
Compare
…ainst all existing services
f2fed47 to
cc5e582
Compare
|
Personally, I only use "by-id" for two of the reasons @nicolas-grekas already pointed out, and therefore have no issue with the removal of "by-type" completely:
The middle-ground approach recently discussed is also fine, but again, I will only ever use "by-id" myself. ;-) |
|
Thank you @nicolas-grekas. |
…g reflection against all existing services (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | yes | BC breaks? | yes - compile time only | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - (patch best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22295/files?w=1).) "By-id" autowiring, as introduced in #22060 is free from all the issues that "by-type" autowiring has: - it has no magic and requires explicit type<>id matching (*vs* using reflection on all services to cherry-pick *the* one that matches some type-hint *at that time*, which is fragile) - it is free from any ambiguities (*vs* the Damocles' sword of breaking config just by enabling some unrelated bundle) - it is easily introspected: just look at DI config files (*vs* inspecting the type-hierarchy of all services + their type-hints) - ~~it is side-effect free, thus plain predictable (*vs* auto-registration of discovered types as services)~~ - it plays nice with deprecated services or classes (see #22282) - *etc.* Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities *means* adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-) For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't *silently* break any apps. For all core Symfony services, they will work out of the box thanks to #22098 *et al.* For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously. Commits ------- cc5e582 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services
|
Yes I know that I can fix this by explicitly adding |
|
@dunglas that would only "fix" autoregistered services, but would not change anything to the session_manager example, which requires an alias yes. |
|
@antanas-arvasevicius the verbosity can be removed by renaming the service to the FQCN - and get rid of any alias. |
|
@nicolas-grekas so for every manual registered service, as "session_manager" we'll need to add |
|
yes, the point that "session_manager" is sometimes used not in autowired schema, so using everythere |
|
as I understand the pattern for everything is as such:
to let autowire for these services |
|
or, using the class<>id convention in place in 3.3
class will be set to "id" |
|
Understood, but then it's hard to use good old style manual service registration with proper nice ID with new autowire logic where you must create (or rename) FQCN-X service names or alias with your manually created services to get autowire working. |
|
@antanas-arvasevicius yep, FQCN-ids are longer. But now that autowiring is predictible, you can almost always enable it for all services, and let that boring work be done by the framework (ie use |
|
Hello, I've created small compiler pass to workaround these restrictions. What's your thoughts about this "workaround"? Maybe that should be added to Symfony core? |
|
@antanas-arvasevicius this autoalias pass basically reverts to the behavior we'd like to get rid of (is reintroduces the issue with ambiguities). Maybe you should open a PR on |
|
@nicolas-grekas: This is exactly the same thing I've been fighting against for when service_id==class was implemented. Everybody was assuring me that nothing prevents me from using my own service ids. And now there is a feature that forces me to use exactly this. Where everything I want to have aoutwired is mostly controllers |
|
@nicolas-grekas no, I think "by-id" autowire is great, but it's great only for "bundles" and library vendors, our own internal, non publishable source code won't have these "side-effects" as have had a "bundles" with previous "by-type" implementation. So I think "whitelisting" namespaces which by default would work "by-type" without strict restriction would be OK. |
|
I understand the expectation but I can't find a satisfactory way to circumvent the drawbacks listed in the description of this PR - even for internal code. This would be just allowing things to be fragile again - for your code only yes, but still looking like a foot gun. We should not help build such things IMHO. |
|
I don't see any reasonable use case how we could fire from autowire "by-type" in our internal code. Examples: b) Actually in internal code all wiring between services is done with concrete classes not with an interfaces at all, create interfaces for internal code where only one specialization exists is just over-engineering with no reason. But from "bundles" perspective I agree with "by-id" benefits, because then someone in some new bundle will introduce service which implements some (e.g. |
|
@nicolas-grekas I highly respect your work and I personally think current direction of changes in DIC is really fantastic, however such a huge BC break (and in fact functionality REMOVAL) is unacceptable. Yesterday I created a branch of our enterprise system, very large one, which uses Symfony DependencyInjection component and I'm scarred and terrified how to approach changes made in 3.3. While I think FQCNs are great I cannot just suddenly switch since a lot of services, due to nature of legacy code, are manually fetched from container all over the code and new code gradually moves towards injection. What does it mean for me (and probably a lot of other devs)? I literally need to spend hours and create enormous clutter in config files converting something like this: CacheTagsFactory:
class: Foo\Bar\CacheTagsFactory
#....into CacheTagsFactory:
class: Foo\Bar\CacheTagsFactory
#....
Foo\Bar\CacheTagsFactory:
alias: CacheTagsFactoryAlternative solution you recommended works only in theory, since usually services do exist and thus they will not be autowired now. I will be able to swallow such change if at least adding simple We use tags to denote some internal relations. There's a compiler pass which looks at these tags and determines if service should be available - if not service is removed and information about that is saved. Not going into details using aliases to solve issue created by this PR will also break that part of our system, since container will be created with I'm always positive about changes, we should move forward and make code better, but sometimes we need to take baby steps on certain changes. In my opinion this change should be made as optional in 3.3 and by-type injection should be removed in 4.0. |
|
@kiler129 I don't get the issue with tags, how does that relate to this change? Aliases do not change anything to the existing tags you have, do they? |
|
@nicolas-grekas: No, I just mentioned that as an example where creating alias is not just an easy fix. My main point is about polluting the config and actually making things less obvious - I absolutely have no problem with how new DIC works, but with the fact old way of searching for services was simply ripped off the code with no [clean] way of grace period for migration. |
|
Proposal to replace the BC Break by a regular deprecation in #22384 |
…n (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Replace autowiring BC break by regular deprecation | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - It happens that doing so is really easy now. And as discussed in #22295, this should be more friendly to our users. Commits ------- 0ed087d [DI] Replace autowiring BC break by regular deprecation
|
@nicolas-grekas I thought the whole point of using the new "experimental" tag was to allow for things to break without having to wait for the next major releases? |
|
@robfrawley Only newly introduced features can be marked as experimental and potentially be removed in the future, that's only about future additions. Flagging existing features as experimental must not happen since we did promise to keep BC on them by introducing them without any flag. |
|
I thought the autowiring component was marked as experimental ATM. Must have been wrong, though. |
(patch best reviewed ignoring whitespaces.)
"By-id" autowiring, as introduced in #22060 is free from all the issues that "by-type" autowiring has:
it is side-effect free, thus plain predictable (vs auto-registration of discovered types as services)Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities means adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-)
For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't silently break any apps. For all core Symfony services, they will work out of the box thanks to #22098 et al. For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously.