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

hhamon · 2017-02-20T11:05:01Z

This PR will work as expected when the following PRs will be merged in the main Symfony repository:

[Form] allow form types + form type extensions + form type guessers to be private services symfony#21690
[DependencyInjection] make the service container builder register its own self referencing definition symfony#21627

stof · 2017-02-20T11:46:29Z

IMO, we should not merge it until we have a 3.3 release in Symfony, to be able to keep releasing the demo project

hhamon · 2017-02-20T11:47:38Z

Yes that's the plan!

stof · 2017-02-20T11:52:38Z

src/AppBundle/Controller/Admin/BlogController.php

+     * Must be protected to allow Symfony DIC to create a proxy.
+     *
+     */
+    protected function getSlugger(): Slugger


this breaks compat with PHP 5.x, while the demo still supports it for now

True! I will update the PR.

javiereguiluz

@hhamon thanks for this proposal. I've left some comments. They are not positive ... but my intention is not negative either.

I'm just trying to put myself in the Symfony newcomers place and think what things could/would confuse me. Most of them are probably because of how new this way to work is and they'll fade in time. Thanks!

javiereguiluz · 2017-02-20T12:16:30Z

src/AppBundle/Controller/SecurityController.php

@@ -13,6 +13,7 @@



These SecurityController.php changes are the perfect example of why I don't like this new way of developing in Symfony. You remove 1 easy-to-understand line ($helper = $this->get('security.authentication_utils');) and replace it with 6 lines of code ... including some "class guessing" to import the AuthenticationUtils class 😕

Yes this is to show that we can easily inject dependencies into controllers. In this example, the AuthenticationUtils service is always used.

Deciding only on the number of line of code is dangerous. As discussed in symfony/symfony#21623, the bigger issue with $container->get('foo') is that you get no guarantee whatsoever about what you'll get back - no contrat. It's working "by luck" in terms of design. It also hardcodes this service - but it should not be the responsibility of this controller class to decide which object to use her, per IoC/DI principles.

I also prefer the new way, it looks more like typical PHP. No framework magic.

I agree with... everyone! :D

The lines of code are important - there is a "perceived complexity" when something has extra lines of code. It's superficial, but, in a sense, people "judge a book bike its cover" by subconsciously seeing a lot of lines of code and thinking that something is complex.

On the other hand, controllers have always behaved differently than other services (where you do need to inject objects and use their methods directly). That's always been unfortunate, and has increased the later-parts of Symfony's learning curve (the base controller shortcuts make it easy to get started... but then you pay a bit more of a price later when you need to learn about services, DI, etc).

In this case, for me, I'm ok with it. But, it does strike me as odd that we still have a mixture of approaches: sometimes (e.g. $this->render()), we're relying on the base controller and using its shortcut methods. But in other cases, we say that you should inject the service you need? If we're moving towards controllers as services, shouldn't we not extend the base controller anymore? If we didn't, that would be a big problem, as it would increase the boilerplate code for many things. Unless, we added traits with getter injects (which could be used for both controllers and in services). I'm not sure what the best approach is - this just strikes me as walking half-way between 2 worlds.

In fact, does this even work? Since the controller is a service (and I think? that the routing automatically knows to use the service, since it exists), the container is never set, correct? So $this->render() is broken.

javiereguiluz · 2017-02-20T12:19:05Z

src/AppBundle/Controller/Admin/BlogController.php

+     * Must be protected to allow Symfony DIC to create a proxy.
+     *
+     */
+    protected function getSlugger(): Slugger


My guess is that this feature will confuse a lot of users. You add to your class an empty getter ... which during runtime is not empty because somehow Symfony injects the needed code to return what you want to get 😕

javiereguiluz · 2017-02-20T12:20:56Z

src/AppBundle/EventListener/RedirectToPreferredLocaleSubscriber.php

@@ -23,7 +25,7 @@
 *
 * @author Oleg Voronkovich <[email protected]>
 */
-class RedirectToPreferredLocaleListener
+class RedirectToPreferredLocaleSubscriber implements EventSubscriberInterface


Just asking: is this change needed because of Symfony ... or is it because you configured this: resource: '../../src/AppBundle/{EventListener/*Subscriber.php,...}'

In other words ... if the previous config was: resource: '../../src/AppBundle/{EventListener,...}' would this change be required?

We may encourage to produce EventSubscribers in favor of regular EventListeners. We can indeed simplify the resource to regiter all classes in the EventListener/ directory. Because later we also override the listener class to complete its definition.

Now that event subscribers can be made private, the new best practice should be to always use them in place of listeners. Thus, I think also that not advertising listeners in symfony-demo is a good thing - even if it works the same as before.

javiereguiluz · 2017-02-20T12:26:30Z

app/config/services.yml

+    # matter as they're referenced here by their real names in the PHP code.
+    # Symfony is then smart enough to make the corresponding matching when compiling,
+    # optimizing and dumping the container as a raw PHP class in the cache directory.
+    AppBundle\Twig\AppExtension:


This may introduce some confusion. The reason is that you already configured this service previously as part of:

AppBundle\: resource: '../../src/AppBundle/{EventListener/*Subscriber.php,Form/Type,Security,Twig,Utils}'

And also:

_instanceof: Twig_ExtensionInterface: tags: ['twig.extension']

And now you are partially configuring it ... or completing its configuration:

AppBundle\Twig\AppExtension: $locales: '%app_locales%'

So ... the AppBundle\Twig\AppExtension is defined in three different parts of the file 😕

The first section lets Symfony register the classes from these directories as services. It'll make its best to autowire everything. But some service requires some extra manual configuration when Symfony cannot guess the remaining parts (dependency collision or scalar arguments). This is what the third section does.

The second section called instanceof allows to append configuration to every definitions that match the rule type. It has nothing to do with autowiring! This is probably the confusing part that must be well taught to newcomers. It's just a way to apply a group of configuration settings to each service definition matching the rule. It's a way to make configuration not repeat itself. This means you can still manually define your services and use the instanceof section to append the DI tags or extra method calls to your sets of configured services.

Wow, @hhamon I didn't know about _instanceof config section. Are there any docs about it? Could be appended to comment to let people know what's the magic behind it.

Documentation doesn't exist yet. This PR is a PoC to experiment the upcoming new DI features of Symfony 3.3. We'll document everything very soon ;)

Ah, I've searched for _instance here and it was only in config so I thought it is already supported elsewhere.

This part - where we fill in missing arguments - should stay as it is - this is perfect

javiereguiluz · 2017-02-20T12:28:25Z

app/config/services.yml

-
-    app.comment_notification:
-        class: AppBundle\EventListener\CommentNotificationListener
+    AppBundle\EventListener\CommentNotificationListener:
        arguments: ['@mailer', '@router', '@translator', '%app.notifications.email_sender%']


Can't these be autowired?

It can be autowired if we simplify the AppBundle/ section by allowing registering all classes under the EventListener/ directory. However we would still have to manually wire the last argument that is scalar and cannot be guessed by Symfony.

stof · 2017-02-20T14:14:02Z

app/config/services.yml

+    # "{EventListener/*Subscriber.php,Form/Type/*Type.php}".
+    AppBundle\:
+        # Register all classes in the src/AppBundle directory as services
+        resource: '../../src/AppBundle/{EventListener,Form/Type,Security,Twig,Utils}'


most form types don't need to be registered as services at all (they need to become services only when they have dependencies).
And as this is broken currently because you define private services, you could avoid doing it for all of them

Yes the current implementation is broken. I'm working on a PR to make form type services privatable.

If symfony/symfony-standard#1070 is merged, this will simplify

GuilhemN · 2017-02-21T10:27:15Z

src/AppBundle/Controller/SecurityController.php

+     * Constructor.
+     *
+     * @param AuthenticationUtils $helper
+     */


this doc block does not bring useful informations, what about removing it?

Fixed thanks.

…egistration and autowiring features.

yceruto · 2017-02-21T14:41:33Z

app/AppKernel.php

@@ -32,6 +32,7 @@ public function registerBundles()
        if (in_array($this->getEnvironment(), ['dev', 'test'])) {
            $bundles[] = new Symfony\Bundle\DebugBundle\DebugBundle();
            $bundles[] = new Symfony\Bundle\WebProfilerBundle\WebProfilerBundle();
+            $bundles[] = new Symfony\Bundle\WebServerBundle\WebServerBundle();
            $bundles[] = new Sensio\Bundle\DistributionBundle\SensioDistributionBundle();
            $bundles[] = new Sensio\Bundle\GeneratorBundle\SensioGeneratorBundle();


Could you add WebServerBundle and SensioGeneratorBundle to dev environment only?

Refs: https://github.com/symfony/symfony-standard/blob/master/app/AppKernel.php#L26-L29

…ronment.

dunglas · 2017-02-22T10:36:28Z

app/config/services.yml

+    # be lazy instanciated when they're really needed. This is why the inherited
+    # default "public" attribute is overriden to force the registered controller
+    # services to be marked public.
+    AppBundle\Controller\:


WYDT about removing the AppBundle like in symfony/symfony-standard#1034?

Interesting discussion. I really like the idea of getting rid of the AppBundle.

I'm sorry but we are not going to get rid of AppBundle in the Symfony Demo. For this kind of changes, the Symfony Demo must go behind the Symfony Framework itself. If Symfony decides to go bundle-less for Symfony 4.0 (I wish it will) then we'll update the Symfony Demo accordingly. But we can't do that before. I'm sorry.

We can still create PR to test this setup without merging it.

dunglas · 2017-02-22T10:40:43Z

src/AppBundle/Controller/Admin/BlogController.php

+    /**
+     * Must be protected to allow Symfony DIC to create a proxy.
+     */
+    protected function getSlugger()


I would not advertise this feature in the demo (it's for advanced users). Using constructor injection is OK here and works out of the box.
It's also easier to understand for newcomers.

Yes I have the same feeling as well.

dunglas · 2017-02-22T10:42:05Z

src/AppBundle/Controller/SecurityController.php

@@ -13,6 +13,7 @@



I also prefer the new way, it looks more like typical PHP. No framework magic.

weaverryan · 2017-02-25T18:01:17Z

app/config/services.yml

+    # Thus, its class is not namespaced and cannot be defined in a global rule.
+    # Indeed, registering new classes whose filename matches a glob pattern as
+    # services like in the two previous sections only works for namespaced classes.
+    app.twig.intl_extension:


Would we want this to also match the class name?

Yea, I think it should be:

# register vendor class a service Twig_Extensions_Extension_Intl: ~

If I do that, I get this error:

RuntimeException in CheckDefinitionValidityPass.php line 52: The definition for "Twig_Extensions_Extension_Intl" has no class attribute, and appears to reference a class or interface in the global namespace. Leaving out the "class" attribute is only allowed for namespaced classes. Please specify the class attribute explicitly to get rid of this error.

…configuration (weaverryan) This PR was squashed before being merged into the 3.3-dev branch (closes #22234). Discussion ---------- [DI] Introducing autoconfigure: automatic _instanceof configuration | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes (mostly, a continuation of a new feature) | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#7538 This is a proposal to allow the user to opt into some automatic `_instanceof` config. Suppose I want to auto-tag all of my voters and event subscribers ```yml # current services: _defaults: autowire: true _instanceof: Symfony\Component\Security\Core\Authorization\Voter\VoterInterface: tags: [security.voter] Symfony\Component\EventDispatcher\EventSubscriberInterface: tags: [kernel.event_subscriber] # services using the above tags AppBundle\Security\PostVoter: ~ AppBundle\EventListener\CheckRequirementsSubscriber: ~ ``` If I'm registering a service with a class that implements `VoterInterface`, when would I ever *not* want that to be tagged with `security.voter`? Here's the proposed code: ```yml # proposed services: _defaults: autowire: true autoconfigure: true # services using the auto_configure_instanceof functionality AppBundle\Security\PostVoter: ~ AppBundle\EventListener\CheckRequirementsSubscriber: ~ ``` 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.voter` instanceof config, FrameworkBundle would add the `kernel.event_subscriber` instanceof config, etc. For another example, you can check out the proposed changes to `symfony-demo` - symfony/demo#483 - the `_instanceof` section is pretty heavy: https://github.com/hhamon/symfony-demo/blob/81694ac21e63bebe7ecfa22fa689766f2f38ccd5/app/config/services.yml#L20 Thanks! Commits ------- 18627bf [DI] Introducing autoconfigure: automatic _instanceof configuration

…configuration (weaverryan) This PR was squashed before being merged into the 3.3-dev branch (closes #22234). Discussion ---------- [DI] Introducing autoconfigure: automatic _instanceof configuration | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes (mostly, a continuation of a new feature) | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#7538 This is a proposal to allow the user to opt into some automatic `_instanceof` config. Suppose I want to auto-tag all of my voters and event subscribers ```yml # current services: _defaults: autowire: true _instanceof: Symfony\Component\Security\Core\Authorization\Voter\VoterInterface: tags: [security.voter] Symfony\Component\EventDispatcher\EventSubscriberInterface: tags: [kernel.event_subscriber] # services using the above tags AppBundle\Security\PostVoter: ~ AppBundle\EventListener\CheckRequirementsSubscriber: ~ ``` If I'm registering a service with a class that implements `VoterInterface`, when would I ever *not* want that to be tagged with `security.voter`? Here's the proposed code: ```yml # proposed services: _defaults: autowire: true autoconfigure: true # services using the auto_configure_instanceof functionality AppBundle\Security\PostVoter: ~ AppBundle\EventListener\CheckRequirementsSubscriber: ~ ``` 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.voter` instanceof config, FrameworkBundle would add the `kernel.event_subscriber` instanceof config, etc. For another example, you can check out the proposed changes to `symfony-demo` - symfony/demo#483 - the `_instanceof` section is pretty heavy: https://github.com/hhamon/symfony-demo/blob/81694ac21e63bebe7ecfa22fa689766f2f38ccd5/app/config/services.yml#L20 Thanks! Commits ------- 18627bf9f6 [DI] Introducing autoconfigure: automatic _instanceof configuration

weaverryan

I think we're just about stable enough (we should be, it's beta!) to finish this. @hhamon would you have time in the next few days or week so we can get it ready and test before release? It's also a good test case for the features :)

weaverryan · 2017-05-12T17:58:23Z

app/config/services.yml

+    # and dumped to raw PHP code.
+    _defaults:
+        autowire: true
+        public: false


Add the new autoconfigure: true

weaverryan · 2017-05-12T17:58:55Z

app/config/services.yml

+    # "{EventListener/*Subscriber.php,Form/Type/*Type.php}".
+    AppBundle\:
+        # Register all classes in the src/AppBundle directory as services
+        resource: '../../src/AppBundle/{EventListener,Form/Type,Security,Twig,Utils}'


If symfony/symfony-standard#1070 is merged, this will simplify

weaverryan · 2017-05-12T17:59:55Z

app/config/services.yml

+    # Thus, its class is not namespaced and cannot be defined in a global rule.
+    # Indeed, registering new classes whose filename matches a glob pattern as
+    # services like in the two previous sections only works for namespaced classes.
+    app.twig.intl_extension:


Yea, I think it should be:

# register vendor class a service Twig_Extensions_Extension_Intl: ~

weaverryan · 2017-05-12T18:00:53Z

app/config/services.yml

+    # matter as they're referenced here by their real names in the PHP code.
+    # Symfony is then smart enough to make the corresponding matching when compiling,
+    # optimizing and dumping the container as a raw PHP class in the cache directory.
+    AppBundle\Twig\AppExtension:


This part - where we fill in missing arguments - should stay as it is - this is perfect

weaverryan · 2017-05-12T18:01:37Z

src/AppBundle/Controller/Admin/BlogController.php

@@ -86,7 +86,7 @@ public function newAction(Request $request)
        // However, we explicitly add it to improve code readability.
        // See http://symfony.com/doc/current/best_practices/forms.html#handling-form-submits
        if ($form->isSubmitted() && $form->isValid()) {
-            $post->setSlug($this->get('slugger')->slugify($post->getTitle()));
+            $post->setSlug($this->getSlugger()->slugify($post->getTitle()));


Now, we should pass this in as an argument to the controller:

public function newAction(Request $request Slugger $slugger)

weaverryan · 2017-05-12T18:08:47Z

src/AppBundle/Controller/SecurityController.php

+    {
+        $this->helper = $helper;
+    }
+


Remove, and move below:

public function loginAction(AuthenticationUtils $authUtils)

This needs to wait for symfony/symfony#22701

weaverryan · 2017-05-12T18:10:08Z

app/config/services.yml

-        tags:
-            - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest }
+        Twig_ExtensionInterface:
+            tags: ['twig.extension']


All of these _instanceof can be removed thanks to autoconfigure - I don't think we'll need any _instanceof in this file.

javiereguiluz · 2017-05-22T07:56:27Z

I'm finishing this PR in #562. Hugo, I'm sorry but I couldn't reuse your original commits because there were too many conflicts (and because Symfony changed the way some of these new features work too). Thanks!

This PR was squashed before being merged into the master branch (closes #562). Discussion ---------- Updated the application to Symfony 3.3.0 This finishes #483 and uses the all the new 3.3 features for DI services. Commits ------- 6979117 Updated the application to Symfony 3.3.0

stof reviewed Feb 20, 2017

View reviewed changes

javiereguiluz reviewed Feb 20, 2017

View reviewed changes

hhamon force-pushed the feat-symfony-33 branch from de0a81c to 3372071 Compare February 20, 2017 13:04

stof reviewed Feb 20, 2017

View reviewed changes

GuilhemN reviewed Feb 21, 2017

View reviewed changes

Hugo Hamon added 5 commits February 21, 2017 11:59

[app] make the app/config/services.yml file use the new Symfony autor…

5b04da5

…egistration and autowiring features.

[app] simplify services configuration.

2f5ab24

[AppBundle] encourage the use of EventSubscriber everywhere.

83ff99a

[AppBundle] inject dependency explicitly in DateTimePickerType class.

356219a

[AppBundle] remove unneeded comment.

6c2b65d

hhamon force-pushed the feat-symfony-33 branch from 8cd79f3 to 6c2b65d Compare February 21, 2017 10:59

[app] fixes typo.

23c8719

yceruto reviewed Feb 21, 2017

View reviewed changes

[app] load WebServerBundle and SensioGeneratorBundle only in dev envi…

81694ac

…ronment.

dunglas reviewed Feb 22, 2017

View reviewed changes

weaverryan reviewed Feb 25, 2017

View reviewed changes

weaverryan mentioned this pull request Mar 31, 2017

[DI] Introducing autoconfigure: automatic _instanceof configuration symfony/symfony#22234

Closed

weaverryan reviewed May 13, 2017

View reviewed changes

javiereguiluz mentioned this pull request May 22, 2017

Updated the application to Symfony 3.3.0 #562

Closed

javiereguiluz closed this May 22, 2017

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

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

Conversation

hhamon commented Feb 20, 2017 • edited Loading

stof commented Feb 20, 2017

hhamon commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 21, 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

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

Choose a reason for hiding this comment

yceruto Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a 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

javiereguiluz commented May 22, 2017

hhamon commented Feb 20, 2017 •

edited

Loading

nicolas-grekas Feb 21, 2017 •

edited

Loading

yceruto Feb 21, 2017 •

edited

Loading