[DI] Add section about service locators #7458

chalasr · 2017-02-09T22:11:50Z

Adds documentation for symfony/symfony#21553 and symfony/symfony#22024.
Any suggestion will be much appreciated, as usual.

ogizanagi

Great! 😃

I just think it misses to mention (and explain) the PSR-11 relation (as you could, and probably should typehint on).

ogizanagi · 2017-02-09T22:28:06Z

service_container/service_locators.rst

+
+        public function handle(Command $command)
+        {
+            foreach (handlerMap as $commandClass => $handler) {


No need for iterating over the handler map, right?

$commandClass === get_class($command); if (isset($handlerMap[$commandClass])) { $handler = $handlerMap[$commandClass]; return $handler->handle($command); }

(or even using null coalesce operator) ? 😄

ogizanagi · 2017-02-09T22:32:44Z

service_container/service_locators.rst

+    class CommandBus
+    {
+        /**
+         * @var ServiceLocator


We should avoid typehinting on ServiceLocator (~~should be internal anyway, right??~~) but rather use the PSR-11 ContainerInterface instead, right?

chalasr · 2017-02-09T22:59:33Z

@ogizanagi Thanks, updated

ogizanagi · 2017-02-09T23:02:46Z

service_container/service_locators.rst

+    $commandBus->handle(new FooCommand());
+
+Because only one command is handled at a time, other CommandHandler services are not
+used.


[...], but still are instantiated uselessly. (might not be the best phrasing)

done, also moved the "Service locators are intended to solve this problem" just after this "wrong" example in order to know what it solves before reading it

unkind · 2017-02-10T15:10:14Z

service_container/service_locators.rst

+Back to our CommandBus which would now look like::
+
+    // ...
+    use Psr\Container\ContainerInterface;


Why do you type hint against this interface? Simple closure is enough, isn't it?

$this->psr11->get(get_class($command)) makes me think that Foo\RegisterUser is a service name, but semantically it isn't.

Goal is to show the provided PSR-11 implem first, so it's clear what you can do when receiving such argument, including throwable exceptions. Just below there's an example showing that it can be used as a callable and how it can be used

I'm not even sure that PSR-11 makes sense for this feature at all. In this context PSR-11 looks like a replacement for the Java's Map: you need some generic explicit interface, with NotFound* exception and get/has methods, but PHP doesn't have it (ArrayAccess is kinda similar, though), so here we go.

Yes, PSR-11 maps string on object, but semantically string here is not serviceId, but something more generic. It looks like far-fetched concept here.

I can't imagine case when I need to type hint against the PSR-11 interface unless I really do something highly cohesive with DI-containers, where I want to say explicitly: "yes, this is about DI-containers", e.g. decorator for DI-containers which logs service ids.

Second example with callable looks for me better even if it has implicit interface. At least, it has nothing to do with DI-containers.

So the example with PSR-11 interface looks a bit unnatural for me. Probably, it's just me. Just my 5 cents.

Yet I understand your concerns, I think PSR-11 is legit in this context since the primary goal of the locator is to fetch services from the DIC (keeping in mind that this doc is about symfony/DI).
I find the callable typehint useful for cases where there's no DIC behind the locator, which should not be the main usage of this feature to me since it is first built to avoid container-aware stuff inside the symfony full stack.

semantically string here is not serviceId, but something more generic. It looks like far-fetched concept here.

In my opinion, services identifiers should remain the same in the locator than in the container from which they come from, so they're safely accessible no matter which container/registry/locator/provider is given to you, that is primordial to me.
Redefining identifiers might just be useful in some cases that I'm still not sure to get, but it seemed logic for others so here we are.

Second example with callable looks for me better even if it has implicit interface. At least, it has nothing to do with DI-containers.

As you pointed out in the code PR, container is just a kind of map after all and if you ask me, PSR-11 could be called Object[Store|Provider|Locator|Registry] instead of Container.

unkind · 2017-02-10T15:13:55Z

@chalasr what do you think about example how to register command handlers for this command bus with tags? I think it will be a very common case for this feature.

chalasr · 2017-02-10T15:29:34Z

@unkind Sounds good, though I think we try to keep sections "standalone" as possible, so maybe not a concrete example but a simple mention saying that tags can be useful in this case and a link to the corresponding doc (working with tagged services).
// cc @javiereguiluz

chalasr · 2017-02-10T15:32:34Z

Status: needs work

(the core PR has been updated, now using the new custom tags feature).

@service2

…ocators (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Replace container injection by explicit service locators | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20658 | License | MIT | Doc PR | symfony/symfony-docs#7458 This adds a new `ServiceLocatorArgument` (`!service_locator`) argument type which takes a list of services, meant to be used as a concrete service locator in order to avoid the remaining needs for injecting the container when it's only a matter of dependency lazy-loading. Config: ```yaml App\FooBar: [!service_locator { key1: '@Service1', key2: '@service2' }] ``` ```xml <service class="App\FooBar" public="false"> <argument type="service-locator"> <argument type="service" key="key1" id="service1"/> <argument type="service" key="key2" id="service2"/> </argument> </service> ``` ```php new ServiceLocatorArgument(array('key1' => new Reference('service1'), 'key2' => new Reference('service2')); ``` Usage: ```php $locator->has('key1') // true $locator->has('service1') // false, the defined key must be used $locator->get('key1'); // service1 instance $locator->get('service1'); // exception $locator->has('not-specified') // false $locator->get('not-specified'); // exception ``` We have some concrete use cases in the core where this would be useful (see e.g. SecurityBundle's FirewallMap), same in userland/3rd party code (see related RFC). Commits ------- e7935c0 [DI] Replace container injection by explicit service locators

ogizanagi · 2017-02-13T19:11:45Z

service_container/service_locators.rst

+
+    return $handlerMap($handlerId)->handle($command);
+
+And can be used in a loop to iterate over all available services::


Should not forget to remove this, as ServiceLocator doesn't implement IteratorAggregate anymore. :)

chalasr · 2017-02-13T21:13:59Z

I will add a second alternative to the first (bad) example: injecting the container.

unkind · 2017-02-13T23:22:21Z

service_container/service_locators.rst

+    // ...
+    $handlerMap = $this->handlerMap;
+
+    return $handlerMap($handlerId)->handle($command);


return call_user_func($this->handlerMap, $handlerId)->handle($command);?

$locateHandler = $this->handlerMap; return $locateHandler($handlerId)->handle($command);

jderusse · 2017-02-14T20:36:23Z

service_container/service_locators.rst

+         */
+        private $handlerMap;
+
+        // ...


You may write the constructor here to understand the correlation between the service declaration and the class instanciation

You're right, definitely needed for a good understanding.

jderusse · 2017-02-14T20:40:48Z

service_container/service_locators.rst

+        public function handle(Command $command)
+        {
+            $commandClass = get_class($command);
+            $handler = $this->handlerMap->get($commandClass);


It's confusing that the service ID of the handler is the fqdn class name of another class
At first I thought it was the same class and I wonder why you ask to the services locator something you already have.. :/

The command bus I use just works like that :) Handler services take a tag including the command class and are resolved exactly like this when calling CommandBus::handle(), it's the better example I found

Sometimes it happens that we inject '@doctrine' in some manager to use it as a locator and lazy load repositories, because its methods may require one or more of them, but some would argue that you should explicit your dependencies in the constructor no matter what. I think that in such case in the future I would opt for this new feature instead, hoping that a plugin will handle the autocompletion before then :).

HeahDude · 2017-02-27T19:33:19Z

service_container/service_locators.rst

+.. tip::
+
+    Not only services can be passed accessible through a service locator,
+    but all types which are supported by the configuration format used to


Maybe a dot would fit after "used" as well? What about an example?

HeahDude · 2017-02-27T19:43:16Z

service_container/service_locators.rst

+        public function handle(Command $command)
+        {
+            $commandClass = get_class($command);
+            $handler = $this->handlerMap->get($commandClass);


Sometimes it happens that we inject '@doctrine' in some manager to use it as a locator and lazy load repositories, because its methods may require one or more of them, but some would argue that you should explicit your dependencies in the constructor no matter what. I think that in such case in the future I would opt for this new feature instead, hoping that a plugin will handle the autocompletion before then :).

apfelbox · 2017-03-06T15:32:52Z

service_container/service_locators.rst

+
+        public function handle(Command $command)
+        {
+            if (isset($handlerMap[$commandClass = get_class($command)])) {


$this->handlerMap ?
(same below)

weaverryan · 2017-03-17T19:12:02Z

This may have just changed due to symfony/symfony#22024

chalasr · 2017-04-12T18:07:44Z

Finally took the time to finish this. Ready to review!

HeahDude

👍

HeahDude · 2017-04-12T19:36:20Z

service_container/service_locators.rst

+
+    // ...
+    $locateHandler = $this->handlerLocator;
+    $handler = $locateHandler($commandClass);


Why not only $handler = $this->locateHandler($commandClass);?

I guess you mean $this->handlerLocator($commandClass) :).
Anyway, the intermediate variable is mandatory, calling $this->handlerLocator() would make the engine look for a handlerLocator() method instead of executing the property as a callable.

The intermediate variable assignment is necessary for php to understand it's a callable property, and not a method.
But as of PHP7, it could be $handler = ($this->handlerLocator)($commandClass)

Indeed, good to know. Let's stay simple there though

(But this is documentation. I think the current code sample is great for this purpose 😄 )

Ok guys, thanks!

javiereguiluz

@chalasr thanks a lot for this! I've reworded it a bit, but maintaining the spirit of your article.

@weaverryan @nicolas-grekas could you please review if the contents are still valid after the latest DI changes? Thanks!

meyerbaptiste · 2017-05-02T15:15:14Z

service_container/service_locators.rst

+        $container
+            ->register('app.command_handler_locator', ServiceLocator::class)
+            ->addTag('container.service_locator')
+            ->setArguments(array(


I recently implemented a service locator for API Platform and the syntax described here is currently not the correct one.

According to src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php#L40-L42, this should be:

->setArguments([ [ 'AppBundle\FooCommand' => new Reference('app.command_handler.foo'), 'AppBundle\BarCommand' => new Reference('app.command_handler.bar'), ], ])

Same for the YAML/XML config.

Indeed, fixed. Thanks!

javiereguiluz · 2017-05-04T07:52:18Z

@chalasr thanks for contributing these docs!

…guiluz) This PR was merged into the master branch. Discussion ---------- [DI] Add section about service locators Adds documentation for symfony/symfony#21553 and symfony/symfony#22024. Any suggestion will be much appreciated, as usual. Commits ------- fa19770 Fix service locator declaration f5e4942 Rewords 5efacd0 [DI] Add section about Service Locators

carsonbot added Status: Needs Review DependencyInjection labels Feb 9, 2017

chalasr mentioned this pull request Feb 9, 2017

[DI] Replace container injection by explicit service locators symfony/symfony#21553

Merged

chalasr force-pushed the service-locator branch 3 times, most recently from 8d947fe to 858d602 Compare February 9, 2017 22:35

ogizanagi reviewed Feb 9, 2017

View reviewed changes

chalasr force-pushed the service-locator branch from 858d602 to 3284fac Compare February 9, 2017 22:59

ogizanagi reviewed Feb 9, 2017

View reviewed changes

chalasr force-pushed the service-locator branch from 3284fac to 36a3f92 Compare February 9, 2017 23:05

unkind reviewed Feb 10, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 10, 2017

ogizanagi reviewed Feb 13, 2017

View reviewed changes

unkind reviewed Feb 13, 2017

View reviewed changes

jderusse reviewed Feb 14, 2017

View reviewed changes

HeahDude reviewed Feb 27, 2017

View reviewed changes

apfelbox reviewed Mar 6, 2017

View reviewed changes

weaverryan added the Missing Documentation label Mar 17, 2017

weaverryan added this to the 3.3 milestone Mar 17, 2017

[DI] Add section about Service Locators

5efacd0

chalasr force-pushed the service-locator branch from 36a3f92 to 5efacd0 Compare April 12, 2017 17:41

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 12, 2017

ogizanagi approved these changes Apr 12, 2017

View reviewed changes

HeahDude approved these changes Apr 12, 2017

View reviewed changes

HeahDude added Status: Reviewed and removed Status: Needs Review labels Apr 12, 2017

Rewords

f5e4942

javiereguiluz approved these changes Apr 28, 2017

View reviewed changes

meyerbaptiste reviewed May 2, 2017

View reviewed changes

Fix service locator declaration

fa19770

javiereguiluz merged commit fa19770 into symfony:master May 4, 2017

chalasr deleted the service-locator branch May 4, 2017 08:07

weaverryan mentioned this pull request May 5, 2017

[DI] New container.service_subscriber & ServiceSubscriberInterface #7740

Closed

weaverryan mentioned this pull request Feb 9, 2018

[DI] Service subscribers #9223

Merged


		return $handlerMap($handlerId)->handle($command);

		And can be used in a loop to iterate over all available services::

[DI] Add section about service locators #7458

[DI] Add section about service locators #7458

Conversation

chalasr commented Feb 9, 2017 • edited Loading

ogizanagi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

chalasr commented Feb 9, 2017

ogizanagi Feb 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

unkind commented Feb 10, 2017

chalasr commented Feb 10, 2017

chalasr commented Feb 10, 2017

Choose a reason for hiding this comment

chalasr commented Feb 13, 2017

unkind Feb 13, 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

jderusse Feb 14, 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

weaverryan commented Mar 17, 2017

chalasr commented Apr 12, 2017 • edited Loading

HeahDude 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

javiereguiluz left a comment

Choose a reason for hiding this comment

meyerbaptiste May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented May 4, 2017

chalasr commented Feb 9, 2017 •

edited

Loading

ogizanagi left a comment •

edited

Loading

ogizanagi Feb 9, 2017 •

edited

Loading

ogizanagi Feb 9, 2017 •

edited

Loading

unkind Feb 13, 2017 •

edited

Loading

jderusse Feb 14, 2017 •

edited

Loading

chalasr commented Apr 12, 2017 •

edited

Loading

meyerbaptiste May 2, 2017 •

edited

Loading