[DI] Service subscribers #9223

codedmonkey · 2018-02-08T02:20:05Z

Couple of notes:

The page name was changed from Service Locators to Service Subscribers. Maybe Service Subscribers & Locators is more appropiate??
Later on in the page it starts talking about service types as inspired by the API documentation of the ServiceSubscriberInterface, but this term isn't used anywhere else in the documentation.

This is my first contribution to the docs so any input is appreciated.

weaverryan · 2018-02-09T04:31:50Z

Thanks for taking this on! This is one of the best, un-documented features. I have a few big-picture things to suggest first:

I think we need to merge this article with the one created in [DI] Add section about service locators #7458. As was mentioned in [DI] New container.service_subscriber & ServiceSubscriberInterface #7740, these are really the same idea, just configured differently. I may be over-simplifying, but I think we should just have one doc page and really one example... and that example should show ServiceSubscriberInterface (not the container.service_locator tag). Then at the bottom of the page, we'd add one more section that shows how you can use that tag. If those really can't be "squeezed" into the same example, that's ok :).
I think your example is more complex it needs to be. There's actually a really simple use-case for a service locator: laziness. By using the locator, any services aren't instantiated unless you ask for them. So, you could have an example where a service may need to use serviceA under some circumstances or serviceB in other circumstances. So, you use a service locator to avoid needing to instantiate both.

Wdyt?

weaverryan · 2018-02-09T04:34:08Z

service_container/service_subscribers.rst

+    }
+
+Optionally, you'll need to add the ``container.service_subscriber`` tag to
+the services definitions of the updaters to enable the service subscribers.


Thanks to autoconfigure, most users won't need this. We usually handle this with just a small note. For example, from https://github.com/symfony/symfony-docs/blob/master/event_dispatcher.rst

If your methods are not called when an exception is thrown, double-check that you're :ref:loading services <service-container-services-load-example> from the EventSubscriber directory and have :ref:autoconfigure <services-autoconfigure> enabled. You can also manually add the kernel.event_subscriber tag.

weaverryan · 2018-02-09T04:34:37Z

service_container/service_subscribers.rst

+                tags: ['container.service_subscriber']
+                arguments:
+                    - '@Psr\Container\ContainerInterface'
+                    - '@AppBundle\BarManager'


No need to show any of this - the user will not need to wire this stuff

The reason I chose to show this is because it was unclear to me at first how to manually pass the service locator to the the service, but I guess this can be part of the note you suggested in the previous comment so +1.

weaverryan · 2018-02-09T04:35:12Z

service_container/service_subscribers.rst

+    public static function getSubscribedServices()
+    {
+        return [
+            'AppBundle\FooInterface',


Should be FooInterface::class with a use statement on top

Was intended to show there are two ways to type out the FQCN but I can see this is unneccesary.

weaverryan · 2018-02-09T04:38:10Z

service_container/service_subscribers.rst

+
+Service subscribers rely on the ``getSubscribedServices()`` method to return a
+list of services types required to build a service locator for instances of
+that service subscriber::


The getSubscribedServices() has always confused me a bit. As I understand it, if you have:

public static function getSubscribedServices() { return [ 'foo' => 'AppBundle\FooInterface', 'logger' => LoggerInterface::class ]; }

Then foo and logger are the service ids you use internally when fetching. But, does the FooInterface and LoggerInterface values mean that autowiring is used to figure exactly which services to include in the service locator? What if I want to pass in a non-traditional logger - e.g. monolog.logger.event - is that even possible?

Let's describe this directly - it's the most important part of the article.

linaori · 2018-02-13T07:24:47Z

service_container/service_subscribers.rst

+
+        /**
+         * Sends an email with Swiftmailer
+         */


This example contains some docblocks, I'm not sure how useful they are for the actual example as the code underneath is explains pretty much the same thing

True, they're from a time where the methods weren't just simple one-liners. Thanks.

codedmonkey · 2018-02-14T12:41:56Z

I think your example is more complex it needs to be. There's actually a really simple use-case for a service locator: laziness.

@weaverryan That's exactly the right way to describe why you'd want to combine the two pages. I figured we want a more expanded example because it is a different page from #7458 wgucg already explains how the Service Locators work. I'll try to combine the two.

I did like this example though, as it shows how AbstractController uses the feature and how a developer can create a similar structure. But its ok with me if we don't include it.

weaverryan · 2018-02-14T14:31:46Z

Thanks @codedmonkey! Do what feels best when you update this, then we can review it again :)

codedmonkey · 2018-02-16T02:17:16Z

The result reads a lot better, so good suggestion @weaverryan! I mainly added a new segment in the middle of the old Service Locators page and renamed the page to reflect the rewrite.

Other thing, somewhere in the middle I start mentioning service types as mentioned in the API. Is it clear to the developer what this means?

…ribers

javiereguiluz

This contribution is really great! I left some minor comments but the improvements are overall very nice!! Thanks @codedmonkey

javiereguiluz · 2018-04-18T06:20:50Z

service_container/service_subscribers.rst


-Service Locators
-================
+Service Subscribers


Everybody on Slack calls this feature "Service Locators" instead of "subscribers". We also called it "locator" in the official blog post, etc. I'm not asking you to remove "subscribers" but we must add "locators" somewhere in the title. Thanks!

Would Service Subscribers & Locators suffice?

javiereguiluz · 2018-04-18T06:22:21Z

service_container/service_subscribers.rst

+-----------------------------
+
+First, turn ``CommandBus`` into an implementation of :class:`Symfony\\Component\\DependencyInjection\\ServiceSubscriberInterface`.
+Use its ``getSubscribedServices`` method to include as many services as needed


We usually put some parenthesis after the method name to make it look more like a method: getSubscribedServices()

javiereguiluz · 2018-04-18T06:23:23Z

service_container/service_subscribers.rst

+
+    return $handler->handle($command);
+
+Including services


services -> Services because we use a special case for titles. Same in the rest of titles of the article.

javiereguiluz · 2018-04-18T06:24:05Z

service_container/service_subscribers.rst

+        ];
+    }
+
+Service types can also be keyed by a service name for use internally::


Not sure about this but: for use internally -> for internal use ?

This PR was merged into the 3.4 branch. Discussion ---------- [DI] Service subscribers Fixes #7740. Couple of notes: * The page name was changed from `Service Locators` to `Service Subscribers`. Maybe `Service Subscribers & Locators` is more appropiate?? * Later on in the page it starts talking about service types as inspired by the API documentation of the `ServiceSubscriberInterface`, but this term isn't used anywhere else in the documentation. This is my first contribution to the docs so any input is appreciated. Commits ------- b1f0343 Incorporate feedback 1754ff8 Minor tweaks to service subscribers article 5a80eea Refactor service locators article to primarily describe service subscribers

* Made some minor PHP CS improvements; * Added a versionadded directive indicating this feature is new; * Added a redirect rule for the old file name

wouterj · 2018-04-18T20:57:17Z

This is huge! Thanks a lot for creating this PR @codedmonkey! I hope we didn't loose you due to the slow response time, as this is a very nice first contribution (I didn't even know the feature existed 🙄 ).

I've made some very minor fixes in 931e6f6, they were too minor to delay this PR even more. It's now merged in all 3.4+ versions and will be online in just a few minutes/hours. Thanks again!

carsonbot added Status: Needs Review DependencyInjection labels Feb 8, 2018

codedmonkey mentioned this pull request Feb 8, 2018

[DI] New container.service_subscriber & ServiceSubscriberInterface #7740

Closed

weaverryan requested changes Feb 9, 2018

View reviewed changes

linaori reviewed Feb 13, 2018

View reviewed changes

Refactor service locators article to primarily describe service subsc…

5a80eea

…ribers

codedmonkey force-pushed the fix_7740 branch from 32fdf16 to 5a80eea Compare February 16, 2018 18:48

codedmonkey changed the title ~~[DI] Service subscribers~~ [WIP] [DI] Service subscribers Feb 21, 2018

Minor tweaks to service subscribers article

1754ff8

codedmonkey changed the title ~~[WIP] [DI] Service subscribers~~ [DI] Service subscribers Feb 28, 2018

codedmonkey mentioned this pull request Mar 6, 2018

Autoconfigure service locator tag #9374

Closed

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Apr 18, 2018

javiereguiluz approved these changes Apr 18, 2018

View reviewed changes

Incorporate feedback

b1f0343

wouterj closed this Apr 18, 2018

wouterj reopened this Apr 18, 2018

wouterj merged commit b1f0343 into symfony:3.4 Apr 18, 2018

wouterj added a commit that referenced this pull request Apr 18, 2018

[#9223] Added some very very very minor fixes

931e6f6

* Made some minor PHP CS improvements; * Added a versionadded directive indicating this feature is new; * Added a redirect rule for the old file name

xabbuh added this to the 3.4 milestone Apr 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Service subscribers #9223

[DI] Service subscribers #9223

codedmonkey commented Feb 8, 2018 •

edited

Loading

weaverryan commented Feb 9, 2018

weaverryan Feb 9, 2018

weaverryan Feb 9, 2018

codedmonkey Feb 14, 2018

weaverryan Feb 9, 2018

codedmonkey Feb 14, 2018

weaverryan Feb 9, 2018

linaori Feb 13, 2018

codedmonkey Feb 14, 2018

codedmonkey commented Feb 14, 2018

weaverryan commented Feb 14, 2018

codedmonkey commented Feb 16, 2018 •

edited

Loading

javiereguiluz left a comment

javiereguiluz Apr 18, 2018

codedmonkey Apr 18, 2018

javiereguiluz Apr 18, 2018

javiereguiluz Apr 18, 2018

javiereguiluz Apr 18, 2018

wouterj commented Apr 18, 2018

[DI] Service subscribers #9223

[DI] Service subscribers #9223

Conversation

codedmonkey commented Feb 8, 2018 • edited Loading

weaverryan commented Feb 9, 2018

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

codedmonkey commented Feb 14, 2018

weaverryan commented Feb 14, 2018

codedmonkey commented Feb 16, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Apr 18, 2018

codedmonkey commented Feb 8, 2018 •

edited

Loading

codedmonkey commented Feb 16, 2018 •

edited

Loading