Making all classes in src/AppBundle available as services #1070

weaverryan · 2017-05-09T20:00:05Z

Hi guys!

tl;dr; This makes all services from src/AppBundle/ available as services by default.

This PR depends on symfony/symfony#22680 and symfony/symfony#22665

If this seems crazy to you, it's actually not: it's almost exactly how the system already works :).
By registering all of src/AppBundle, we will almost undoubtedly register some classes that should not be services. But, because the services are private, unless you actually reference them somewhere, they're simply removed. In other words, we're not really importing all services from src/AppBundle, we're making all classes in src/AppBundle available to be used as services.

Today, thanks to autowiring, if you type-hint a class that is not registered as a service, autowiring registers it for you. That means that the total number of services in the compiled container today versus after this change will be the same.

Today if you don't explicitly register a service as public and don't type-hint/reference it anywhere, it's never added.
After if you don't explicitly register a service as public and don't type-hint/reference it somewhere, it's removed from the final container

So, the end result is the same as now. And there are a number of advantages:

PROS

A) Better developer experience: when a dev creates a new directory, they don't need to remember to go into services.yml and add it. The original directories we added to the SE were a random "guess"... which should feel weird. I think it's because that approach is flawed in general.
B) Less WTF and better DX with autoconfigure: now, if I create a new class that extends Twig_Extension but forget to import my Twig directory, then my extension won't work. After this, we'll find it for sure.
C) Clearer for the documentation: before this change, in many places, we need to say "go to services.yml and add this new directory to your import... unless you already have it... and don't make your line look exactly like our's - you're probably also importing other directories"
D) We could deprecating the "autowire auto-registration" code in 3.4 (i.e. when autowiring automatically registers a private services if no instance exists in the container). That means less magic: this is actually more explicit. This could be awesome - we could remove some "hacky" code (e.g. [DI] Restrict autowired registration to "same-vendor" namespaces symfony#22306). And, if you type-hinted an Entity (for example) in autowiring, instead of the container creating a service for you, it can give you a clear error

In theory, the CON is that this will slow down the container rebuilding as more services are added to ContainerBuilder (before being removed later). On one project, I compared the build time without importing everything (138 services added from src/) versus importing everything (200 services dded from src/). The build time difference was negligible - maybe 10ms difference (both were around 950ms).

Btw, the exclude key added in symfony/symfony#22680 is almost not even needed, since unused services are simply removed. But, currently, the Entity directory is an exception due to the new "argument resolver" trying to see if it can wire those as services. That could be fixed in the future. But exclude is kind of nice, if you want to explicitly safe-guard someone from accidentally type-hinting something from that directory. Again, that's better than now... where we will always auto-register an Entity if you type-hint it.

Cheers!

GuilhemN · 2017-05-10T10:21:14Z

app/config/services.yml

    # this creates a service per class whose id is the fully-qualified class name
    AppBundle\:
-        resource: '../../src/AppBundle/{Command,Form,EventSubscriber,Twig,Security}'
+        resource: '../../src/AppBundle/*'


This syntax doesn't register sub folders, isn't it ?

It definitely does - the FileLoader always passes a $recursive flag set to true to the glob functionality. But, I totally understand what you're referring to with the normal ** syntax :)

Ok, thanks for your explanation ☺

GuilhemN · 2017-05-10T10:22:32Z

app/config/services.yml


+    # controllers are imported separately to make sure they're public
+    # and have a tag that makes services


This sentence looks incomplete

GuilhemN · 2017-05-10T11:23:49Z

app/config/services.yml

    AppBundle\Controller\:
        resource: '../../src/AppBundle/Controller'
        public: true
        tags: ['controller.service_arguments']
+
+    # add more service, or override services that need manual wiring


Ha, thank you for the review! I managed to make a lot of typos... in a very small area :).

That's why we do reviews :) Thanks for your pr!

GuilhemN · 2017-05-10T11:24:41Z

app/config/services.yml

    # this creates a service per class whose id is the fully-qualified class name
    AppBundle\:
-        resource: '../../src/AppBundle/{Command,Form,EventSubscriber,Twig,Security}'
+        resource: '../../src/AppBundle/*'
+        # you can include directories or files


Should be exclude, right ?

GuilhemN · 2017-05-10T11:25:11Z

app/config/services.yml


+    # controllers are imported separately to make sure they're public
+    # and have a tag that makes allows actions to type-hint services


makes should be removed

fabpot · 2017-05-11T17:44:35Z

app/config/services.yml

@@ -14,12 +14,22 @@ services:
        # if you need to do this, you can override this setting on individual services
        public: false

-    # loads services from whatever directories you want (you can add directories!)
+    # makes all classes in src/AppBundle available as services


I would mention that not all classes end up as services

weaverryan · 2017-05-12T17:32:34Z

Awesome! I tweaked the description of importing from src/ and also updated to the new glob exclude pattern in symfony/symfony#22680

I also found an even more necessary reason for this change. Suppose you have:

class IceCreamGenerator
{
    public function __construct(LoggerInterface $logger) {}
}

But you do not register this as a service. Instead, you just type-hint this in a controller:

public function indexAction(IceCreamGenerator $generator)

This will work: autowiring will auto-register IceCreamGenerator as a service. But, suppose now that you add a un-autowireable arg to it:

class IceCreamGenerator
{
    public function __construct(LoggerInterface $logger, $foo) {}
}

This is the error the user sees (only when they visit this page) is:

Controller "AppBundle\Controller\BlogController::indexAction()" requires that you provide a value for the "$generator" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or because there is a non optional argument after this one.

The error is very unclear. The reason is due to how the "controller args" feature works: if it has any issue autowiring an arg, it just skips it (it needs to do this, due to things like entities). That's fine, but the error is a problem.

But instead, if you load all services from src/AppBundle as I suggest in this PR, then you get correct error (and at compile time):

Cannot autowire service "AppBundle\Delicious\IceCreamGenerator": argument "$foo" of method "__construct()" must have a type-hint or be given a value explicitly.

GuilhemN · 2017-05-12T21:38:06Z

app/config/services.yml

-        resource: '../../src/AppBundle/{Command,Form,EventSubscriber,Twig,Security}'
+        resource: '../../src/AppBundle/*'
+        # you can exclude directories or files
+        # but if a service is unused, it's removed anyways


isn't anyway more usual?

fabpot · 2017-05-14T16:27:14Z

Thank you @weaverryan.

…ices (weaverryan) This PR was merged into the 3.3-dev branch. Discussion ---------- Making *all* classes in src/AppBundle available as services Hi guys! tl;dr; This makes *all* services from `src/AppBundle/` available as services by default. > This PR depends on symfony/symfony#22680 and symfony/symfony#22665 If this seems crazy to you, it's actually not: it's almost exactly how the system already works :). By registering *all* of `src/AppBundle`, we will almost undoubtedly register some classes that should *not* be services. But, because the services are private, unless you actually reference them somewhere, they're simply removed. In other words, **we're not *really* importing all services from `src/AppBundle`, we're making all classes in `src/AppBundle` *available* to be used as services**. Today, thanks to autowiring, if you type-hint a class that is *not* registered as a service, autowiring registers it for you. That means that **the total number of services in the compiled container today versus after this change will be the same**. * **Today** if you don't explicitly register a service as public and don't type-hint/reference it anywhere, it's never added. * **After**   if you don't explicitly register a service as public and don't type-hint/reference it somewhere, it's removed from the final container So, the end result is the same as now. And there are a number of advantages: **PROS** * A) **Better developer experience: when a dev creates a new directory**, they don't need to remember to go into `services.yml` and add it. The original directories we added to the SE were a random "guess"... which should feel weird. I think it's because that approach is flawed in general. * B) **Less WTF and better DX with `autoconfigure`**: now, if I create a new class that extends `Twig_Extension` but forget to import my `Twig` directory, then my extension won't work. After this, we'll find it for sure. * C) **Clearer for the documentation**: before this change, in many places, we need to say "go to services.yml and add this new directory to your import... unless you already have it... and don't make your line look exactly like our's - you're probably also importing other directories" * D) **We could deprecating the "autowire auto-registration" code in 3.4** (i.e. when autowiring automatically registers a private services if no instance exists in the container). That means less magic: this is actually *more* explicit. This could be awesome - we could remove some "hacky" code (e.g. symfony/symfony#22306). And, if you type-hinted an `Entity` (for example) in autowiring, instead of the container creating a service for you, it can give you a clear error In theory, the CON is that this will slow down the container rebuilding as more services are added to `ContainerBuilder` (before being removed later). On one project, I compared the build time without importing everything (138 services added from `src/`) versus importing everything (200 services dded from `src/`). The build time difference was negligible - maybe 10ms difference (both were around 950ms). Btw, the `exclude` key added in symfony/symfony#22680 is almost not even needed, since unused services are simply removed. But, currently, the `Entity` directory is an exception due to the new "argument resolver" trying to see if it can wire those as services. That could be fixed in the future. But `exclude` is kind of nice, if you want to explicitly safe-guard someone from accidentally type-hinting something from that directory. Again, that's *better* than now... where we will *always* auto-register an Entity if you type-hint it. Cheers! Commits ------- 47d3561 Making *all* services in src/AppBundle available as services

fabpot · 2017-05-14T16:32:32Z

see symfony/recipes#65 for the equivalent on Flex.

xabbuh · 2017-05-15T10:19:16Z

app/config/services.yml

+        resource: '../../src/AppBundle/*'
+        # you can exclude directories or files
+        # but if a service is unused, it's removed anyway
+        exclude: '../../src/AppBundle/{Entity,Repository}'


What about excluding controllers here explicitly too? They are registered below anyway (and excluding them here may prevent some confusion).

I'm mixed. On one hand, I want exclude to be a detail that people don't think about or even notice. In fact, the only reason it's even needed is for Entity, to avoid a weird situation where the new "controller service args" functionality keeps a reference to them so they're not removed. Adding Controller to it makes this setting seem even more important.

On the other hand, especially in the future if we remove the "auto-register services" functionality from autowiring, exclude will be a nice way to explicitly prevent things from being used as services.

So, I kind of think we should not add it. In theory, we could also add AppBundle.php... but I think we should keep it minimal instead.

This PR was squashed before being merged into the master branch (closes #7906). Discussion ---------- More updates for "Type-based" injection Apologies for another big PR, and also if I'm stepping on anyone else's feed - I'm just trying to charge through and make all the changes to the docs. This contains 2 things: 1) More updates to entries to favor type-based injection (type-hinting and relying on autowiring) over manually configuration. I have more chapters to do still, but I'm mostly done. 2) Important updates (mostly to `service_container.rst`) now that we're importing *all* of `src/AppBundle` in `services.yml` (symfony/symfony-standard#1070). Side note: wooohooo!!! P.S. I deleted one entry that I thought was really poor and unnecessary. Commits ------- f0361fa More updates for "Type-based" injection

This PR was merged into the master branch. Discussion ---------- Tweak configuration for app.yaml symfony/symfony-standard#1070 Commits ------- 7581f44 tweaked configuration for app.yaml

weaverryan mentioned this pull request May 10, 2017

Example of the Doctrine doc with types as the first-class citizen symfony/symfony-docs#7876

Closed

GuilhemN reviewed May 10, 2017

View reviewed changes

weaverryan force-pushed the import-src-dir branch from 282b146 to b444b41 Compare May 10, 2017 10:48

GuilhemN reviewed May 10, 2017

View reviewed changes

nicolas-grekas mentioned this pull request May 10, 2017

[DI] Fixing missing "exclude" functionality from PSR4 loader symfony/symfony#22680

Merged

weaverryan force-pushed the import-src-dir branch from b444b41 to 18fd46a Compare May 10, 2017 12:43

fabpot reviewed May 11, 2017

View reviewed changes

weaverryan force-pushed the import-src-dir branch 2 times, most recently from 663fc68 to 571e54e Compare May 12, 2017 17:19

GuilhemN approved these changes May 12, 2017

View reviewed changes

weaverryan mentioned this pull request May 12, 2017

[WIP] make the app/config/services.yml file use the new Symfony autoregistration and autowiring features. symfony/demo#483

Closed

Making *all* services in src/AppBundle available as services

47d3561

weaverryan force-pushed the import-src-dir branch from 571e54e to 47d3561 Compare May 13, 2017 01:03

fabpot merged commit 47d3561 into symfony:master May 14, 2017

fabpot mentioned this pull request May 14, 2017

Tweak configuration for app.yaml symfony/recipes#65

Merged

weaverryan deleted the import-src-dir branch May 14, 2017 17:58

weaverryan mentioned this pull request May 14, 2017

More updates for "Type-based" injection symfony/symfony-docs#7906

Closed

xabbuh reviewed May 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making all classes in src/AppBundle available as services #1070

Making all classes in src/AppBundle available as services #1070

weaverryan commented May 9, 2017 •

edited

Loading

GuilhemN May 10, 2017

weaverryan May 10, 2017

GuilhemN May 10, 2017

GuilhemN May 10, 2017

GuilhemN May 10, 2017

weaverryan May 10, 2017

GuilhemN May 10, 2017

GuilhemN May 10, 2017

GuilhemN May 10, 2017

fabpot May 11, 2017

weaverryan commented May 12, 2017

GuilhemN May 12, 2017

fabpot commented May 14, 2017

fabpot commented May 14, 2017

xabbuh May 15, 2017

weaverryan May 15, 2017


		# controllers are imported separately to make sure they're public
		# and have a tag that makes services


		# controllers are imported separately to make sure they're public
		# and have a tag that makes allows actions to type-hint services

Making *all* classes in src/AppBundle available as services #1070

Making *all* classes in src/AppBundle available as services #1070

Conversation

weaverryan commented May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented May 12, 2017

Choose a reason for hiding this comment

fabpot commented May 14, 2017

fabpot commented May 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Making all classes in src/AppBundle available as services #1070

Making all classes in src/AppBundle available as services #1070

weaverryan commented May 9, 2017 •

edited

Loading