[DI] Introducing autoconfigure: automatic _instanceof configuration #22234
Conversation
|
Would it be possible for 3rd party bundles to extend this functionality? Say my Bundle has an interface that works with a tag as well, it would be very nice for developers to automatically have the tags. |
|
@iltar absolutely - and for that exact reason - auto-tagging by interface for a tag introduced by a third-party bundle :) |
15814d1 to
5ca7865
Compare
b1ddadb to
e87962a
Compare
|
This is ready! I added a note about an issue with the (already-merged) I would love to sneak this in for 3.3 - the current |
| } | ||
|
|
||
| /** | ||
| * @return array |
There was a problem hiding this comment.
Maybe should you add a description to those public methods?
| @@ -515,6 +518,11 @@ private function parseDefinition($id, $service, $file, array $defaults) | ||
| } | ||
| } | ||
|
|
||
| $autoConfigureInstanceof = isset($service['auto_configure_instanceof']) ? $service['auto_configure_instanceof'] : (isset($defaults['auto_configure_instanceof']) ? $defaults['auto_configure_instanceof'] : null); |
There was a problem hiding this comment.
Nested ternaries are hard to read :) Why not using if blocks? In Symfony 4 we'll be able to replace such constructs with ??.
| @@ -515,6 +518,11 @@ private function parseDefinition($id, $service, $file, array $defaults) | ||
| } | ||
| } | ||
|
|
||
| $autoConfigureInstanceof = isset($service['auto_configure_instanceof']) ? $service['auto_configure_instanceof'] : (isset($defaults['auto_configure_instanceof']) ? $defaults['auto_configure_instanceof'] : null); | ||
| if (null !== $autoConfigureInstanceof) { |
There was a problem hiding this comment.
This whole block dans be simplified:
if (isset($service['auto_configure_instanceof'])) {
$definition->setShouldAutoConfigureInstanceofConditionals($service['auto_configure_instanceof']);
} elseif (isset($defaults['auto_configure_instanceof'])) {
$definition->setShouldAutoConfigureInstanceofConditionals($defaults['auto_configure_instanceof']);
}|
I really like this proposal! Just some ideas to go further: Why putting services:
_auto_instanceof: true
# ...And I wonder if this approach should not totally replace the actual |
|
Btw, ActionBundle only allows to configure the tag/interface mapping globally and having the ability to do it per file has never been requested. |
|
👍 for Instead of the defaults. About the replacing of the |
The feature freeze is exactly for this kind of fine tuning :) |
|
Having the option in About the name, what about just using services:
_defaults:
autowire: true
autoconfigure: true |
| * | ||
| * @experimental in version 3.3 | ||
| */ | ||
| public function getShouldAutoConfigureInstanceofConditionals() |
There was a problem hiding this comment.
What about removing the should? It doesn't bring anything imo.
There was a problem hiding this comment.
or just "isAutoconfigured"?
|
I will still update from your comments and finish this - I think this is an easy and big win that makes the new |
|
👍 |
e87962a to
96f3014
Compare
|
This is ready to go! The configuration is now called As we've mentioned in a few other places, this makes it more likely that a tag will be applied to a service twice, but in #22398 I illustrate how the tags are already added in a specific order so that compiler passes can handle (de-dup) this situation. In #22396 (and related changes that are needed to 3.2, once that's merged up), we're fixing individual compiler passes that may not handle multiple tags gracefully yet. Most already handle them perfectly (because multi tags was always a valid situation). Cheers! |
| @@ -19,6 +19,8 @@ | ||
| use Symfony\Component\Config\Loader\LoaderInterface; | ||
| use Symfony\Component\Config\Resource\DirectoryResource; | ||
| use Symfony\Component\Console\Application; | ||
| use Symfony\Component\Config\ResourceCheckerInterface; |
| @@ -27,17 +29,33 @@ | ||
| use Symfony\Component\DependencyInjection\Exception\LogicException; | ||
| use Symfony\Component\DependencyInjection\Reference; | ||
| use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; | ||
| use Symfony\Component\DependencyInjection\ServiceSubscriberInterface; |
| use Symfony\Component\WebLink\HttpHeaderSerializer; | ||
| use Symfony\Component\Translation\Dumper\DumperInterface; |
| * @param string $interface The class or interface to match | ||
| * @param $childDefinition ChildDefinition | ||
| */ | ||
| public function addAutomaticInstanceofDefinition($interface, ChildDefinition $childDefinition) |
There was a problem hiding this comment.
could this return the ChildDefinition instead of this second arg?
that would allow things like $container->addAutomaticInstanceofDefinition(\Twig_ExtensionInterface::class)->addTag('twig.extension')
There was a problem hiding this comment.
about the name, $container->registerForAutoconfiguration()?
| @@ -86,13 +88,15 @@ class YamlFileLoader extends FileLoader | ||
| 'calls' => 'calls', | ||
| 'tags' => 'tags', | ||
| 'autowire' => 'autowire', | ||
| 'autoconfigure' => 'autoconfigure', |
There was a problem hiding this comment.
not sure it makes sense here? if it does, it's missing from the xsd
There was a problem hiding this comment.
Ha, so now we've reversed roles :).
Maybe? I have an IntegrationTest testing the override logic for it, so that's solid. In theory, I could use autoconfigure: true under _defaults, but then set autoconfigure: false for one specific interface (e.g. VoterInterface) because I'm doing something weird and want to control all of my voters entirely myself.
| } | ||
|
|
||
| $childDefinition = new ChildDefinition(''); | ||
| $this->automaticInstanceofDefinitions[$interface][] = $childDefinition; |
There was a problem hiding this comment.
Append, fail or replace that's my only remaining questions
There was a problem hiding this comment.
Definitely appending (which means, merging).
Why?
-
If 2 DI extensions both autoconfigure the same instance (which probably shouldn't happen), we should probably throw an exception or merge them together. Replacing seems completely wrong, since execution order isn't guaranteed. Of course, merging is a bit weird too, since execution order isn't guaranteed. But if they configure different things, merging is great. If they configure the same config... well, they shouldn't do that :).
-
If the end-user wants to auto configure an instanceof globally, why not let them? e.g. suppose they want to add some extra config to all VoterInterface? In this case, throwing an exception does not make sense. So, replacing or merging makes sense. And I think merging is more user-friendly: they're able to inherit all the original auto-config, then override whatever they want. They will get duplicate tags... but since new tags will be added first, they will override the original tags.
So, appending/merging makes sense to me. This actually affects 2 places in the code - calling registerForAutoconfiguration() 2 times on the same builder and merging 2 builders... but I think they should probably act the same.
|
Changes made! |
|
Updated again: the PR description accurately explains the feature. Most recent change is that: A) If you call B) In |
22c71bf to
3d78194
Compare
|
Thank you @weaverryan. |
|
Just wondering, is there a case you don't want this feature on by default? This will only force people to put same lines in every config file they have. |
That's just not how Symfony works. If the feature were on by default it would be "magic", the killer word used to describe "something is happening in my application and I never told it to". Magic can actually be good. Magic can be a conscious choice. Laravel for example is built around magic, and is used by millions of developers madly in love with its magic. Magic makes sure you can develop quick and not have to know everything about what you're doing. Many other professional developers despise magic, because it allows people to have no clue what they're doing and even if they do accidentally introduce the most obscure bugs and unpredictable behavior. Symfony does not do magic. |
|
and having it on by default would impact all existing code, and so would be a BC break. |
|
@curry684 Well, tagging is magic, no doubt about that. This only adds magic over magic. But I rather look from a practical point of view. A use case, when you don't want this feature on by default? To explain better, when this is NOT WANTED in you app ↓ # services.yml
services:
some_command:
class: SomeCommand
tags: [console.command] # no, you don't want this here!Could you give me some? Real examples only, please, not theory. |
|
That's a very easy one actually - when following standard procedure by putting Console commands in the But for example for Security Voters I may have an application which defines 20 voters of which, depending on deployment type or the availability of external integrations, only 15, 17 or 19 are actually going to be enabled. Not tagging them takes them out of use. For Event Subscribers the same goes - I may opt to disable a few low level logging event subscribers in certain deployments for performance reasons, and tag them based on env variables in my bundle compilation. It's not our job to tell the developers what they want, we're just providing the enabling tools.
Tagging is the exact opposite of magic, it's the epitome of explicitly and verbosely stated intent. |
That's just miss leading of another magic. And you do, if you use DI and not write clear extension by yourself.
Why would you register a service you don't use?
"In theory everything is true." Do you actually already do that? This sound like architecture smell in your application. To much coupling and breaking SRP.
That's what frameworks do actually. If you get hammer, everything seem like a nail. And it's their job, it's ok. Just this hammer seems like adding knife and vacuum cleaner to it. Still missing some saint and practical examples. |
Why shouldn't I be able to register a voter I wish to invoke manually and conditionally? The case is also perfectly sane and practical in a bundle to have an extension only tag a number of specific voters if configuration says a specific external authorization component is available.
No, this is definitely not true of frameworks like .NET and Symfony. They provide you with a huge toolbox of things, and then allow you to use not a single one of them, or just the hammer, or just the screwdriver, or both. That's why they're classified as unopinionated, you the developer provide the opinions by opting into functionality you want to be there. Batteries are included but optional and replaceable. Laravel on the other hand is a clear example of an opinionated framework. Their opinions make for fast development of predictable and rigidly structured projects. But if you don't agree with a significant number of their opinions, you shouldn't be developing in Laravel. You cannot realistically replace the battery without breaking everything else. |
Could you paste a code from your app where you do that? Still seems like overcomplicating simple things easily solvable by good service design.
This is so common claim among Symfony developers I've noticed in last few years. Many people that use Symfony for years are very biased in this way seeing themselves as unopinionated. I was the same when using one framework for years. It's common for confirmation bias. Saying the opposite is like a man saying "this is objective". From the out-group view, Symfony forces me to use lot of over-complicated code just to register simple service and load it into another.
As many Symfony contribution are made ~5 very similar people, it's very opinionated framework. It's not about number of people, but diversity amongts them. 10000 pencils will give you probably the same feature. 2 people from different states caring about each other's experiences will give you another result. I don't say Laravel is perfect in this way, since it drags it's over-complexity pattern from Symfony, but one thing a like about Laravel is that it gives you much greater freedom and doesn't vendor lock as much as Symfony. That's where Symfony could learn a lot. Like in this feature. Symfony is getting very complicated for simple service usage with not much advantage of end users. If there would be one optional place to turn this on, it would make sense, but this just makes my pollute my configs. |
|
I care about this because these features I present during my lectures and mentoring. And people give me WTF and I have no useful reasons for it. I don't want to teach people features just because they exists. I want to teach them the best known approach on the market. I feel like Symfony is to be designed more and more for people that already use it for years, and closing doors more and more for new ones. That's probably why Laravel is doing so great. |
|
@TomasVotruba not sure from where you get that feeling. As @stof said it's simply not possible to enable it by default as it would be a BC. And I think on the contrary Symfony is changing a lot to be easier to use and more friendly to people coming in. It strives at finding a good balance between providing the strictness for the anti-magic people and the magic all the way people. And from what I've seen in the 3.x, it's on good track for it. If like me you're not happy with some defaults, you can always create a compiler pass to add those defaults (e.g. |
|
Note that this feature will be enabled by default by Flex (see https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/3.3/etc/packages/app.yaml#L4), this makes your config file much simpler than before while still being explicit, I think it's the best compromise to satisfy everyone. |
|
@GuilhemN Still the same issue, per file pollution of single feature. Read bellow please.
Mostly from recently added "config programming" features in 3.3, getter autowiring, per config autotowiring etc.
Why do you think I want a BC break? I want an option to enable it for all services. It can be off by default, of course - it should. Maybe use case would be betterIn previous project we had over 30 modules. When I use these features, I will end up with: # src/*Module/config/services.yml
services:
_defaults:
autowire: true
autoconfigure: true # this adds tags actually
# services
// ...30 times repeated everywhere. DRY? SRP? SOLID? I don't think so. I prefer writing only what is necessary. # src/*Module/config/services.yml
services:
# services
// ...Somebody will definitely forget (we all know these bugs) to add this to another Why creating a new way to confuse people? I really don't see a point to make Symfony this playground. Again, I don't talk about you, me, us, using Symfony for years and knowing all it's magic now, so we don't feel like it's magic, but those people, who are using Symfony under a year. Those are the most ignored group nowadays. This feature is already covered in code of FrameworkBundle anyway: # app/config/config.yml
framework:
autowire: true
autoconfigure: true # add tags to servicesOf course I can write a new bundle for every single line option, but it feels like delegating any responsibility of bad design to user. Not help I'd expect from tool. Saved possibly millions of lines and thousands of bug reports in all applications in the worlds. I'm sure, because I've been consulting and mentoring various Symfony/Nette/proprietary projects and this kind of stuffs takes the most time. Not to setup, but to debug eventually, when team grows and complexity rises. I think this is difficult to spot, when working on 1 framework and 1 company, yet when all tests are passing, right. I would noticed it before either. But from my experience and feedback from programmers from my consultancy work, these are double-edged feature more giving now, but more taking later. |
|
A global option is a pain in the ass, because it also impacts all code which is not yours (symfony itself, external bundles, etc...). Having a different behavior for your service configuration depending on some project level configuration would make it a real pain for all bundle maintainers (as they would have to overwrite all config to repeat the Symfony default to ensure it is indeed the default being applied, which then becomes a pain to support several Symfony versions as the list of config settings you need to define is different for each version). |
Can be limited to app of course, that's just implementation detail. I just autowiring for over 6 years in Nette and Symfony apps with no problems. Still, it's about having the better option in case you can afford to. Not to deny it, because it might cause problems. I feel too much fear in here to make a change for better. |
|
@TomasVotruba I don't think hijacking this PR is the right place to debate about that :) That said, the problem I see in your comments come from:
For the first point it's your choice, but then I don't understand the later: you can simply register all the source code of your application as services which would simulate what you have with a DIC which has autowiring by default: you have to register a very limited amount of services and you don't have to re-configure everything. But as I said, it's not the best place to discuss about that. Maybe this would be more appropriate on Slack (https://symfony.com/slack-invite) or a dedicated issue :) |
|
@theofidry What do you mean by "hijack PR"? I want to point out bad design of these features and underline them with my experience and feedback from Symfony users around me. My goal is to improve usability of Symfony, so people are not forced to use more developer-friendly frameworks, support usability by writing package for every new feature or write coupled and legacy code enforced by this.
That's a design pattern, not a problem.
What do you mean? Do prefer having monolithic design with huge coupled files? Feel free to open issue, I'm not much into private Slack talks about public PRs. |
Hence a dedicated issue as it's much broader than the specific feature of this PR which has already been merged.
Bad phrasing, soz.
Ha, that's where I'm asking you what you mean. It's quite common, auto-wiring or not, to have 1 But maybe having more concrete examples here would help :)
Hey, you're the one who wants to voice your concerns, I can't voice them in your stead if I don't share them ;) |
I've learned about this from a blog post, thus writing now.
It seems that the false axiom, that you should reconsider to validate. I've seen single To give you a scale: I work with applications old 3-10 years having 500-1500 services (on average). Never the less, single
What other examples do you need? The one above describes the issue I talk about. I can complete services if you need :D
I'm doing so here and now. |
Yeah, and I've already seen that in not more than 3 service files. I don't recommend to do it though...
I don't understand how your service declaration promotes coupling. If you want to switch of framework you'll have to rewrite your service wiring (auto-wiring involved or not) and that regardless of the number of services/files.
services:
App\:
resource: '%kernel.root_dir%/../src'
autowire: trueOr something along those lines, then you only need to declare the a few services which require dedicated config. With that, I don't see how you run into the issue of having to repeat those file defaults config 30 times over. |
Not really. Well written apps are easy to switch, since you only bind to routing, Kernel and DI logic.
Let's say you have one e-commerce application that contains ProductBundle (or module, extension whatever). It uses Then you want to use CategoryBundle in other project, dealing with online content. If you write your code with modularity in mind, it only takes adding few interfaces. But what if you have one huge Everything over adding a Bundle to AppKernel is redundantI don't want to and take care of config. I want to plugin and code. When you have decouple application and 2-3 projects are going to share few logic, or microservices splitting, one single I've also seen few cases, when somebody added bundle, but didn't register MANUALLY Why not use config for everything?Setting up a module is usually more complicated than adding services, like you mention. Usually, you need to join them and add some parameters. Again, it's about reusability of growing app that need long-term support. If there would be no bundles, extensions and no applications that might share that code, using one |
|
I'm curious and could not find it, where was services:
App\:
resource: '%kernel.root_dir%/../src'EDIT - found it: #21289 |
This is a proposal to allow the user to opt into some automatic
_instanceofconfig. Suppose I want to auto-tag all of my voters and event subscribersIf I'm registering a service with a class that implements
VoterInterface, when would I ever not want that to be tagged withsecurity.voter? Here's the proposed code:The user must opt into this and it only applies locally to this configuration file. It works because each enabled bundle would have the opportunity to add one or more "automatic instanceof" definitions - e.g. SecurityBundle would add the
security.voterinstanceof config, FrameworkBundle would add thekernel.event_subscriberinstanceof config, etc.For another example, you can check out the proposed changes to
symfony-demo- symfony/demo#483 - the_instanceofsection is pretty heavy: https://github.com/hhamon/symfony-demo/blob/81694ac21e63bebe7ecfa22fa689766f2f38ccd5/app/config/services.yml#L20Thanks!