Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[DI] Add section about service locators#7458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
8d947fe to858d602Compare
ogizanagi left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great! 😃
I just think it misses to mention (and explain) the PSR-11 relation (as you could, and probably should typehint on).
| public function handle(Command $command) | ||
| { | ||
| foreach (handlerMap as $commandClass => $handler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) ? 😄
| class CommandBus | ||
| { | ||
| /** | ||
| * @var ServiceLocator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should avoid typehinting on ServiceLocator (should be internal anyway, right??) but rather use the PSR-11ContainerInterface instead, right?
chalasr commentedFeb 9, 2017
@ogizanagi Thanks, updated |
| $commandBus->handle(new FooCommand()); | ||
| Because only one command is handled at a time, other CommandHandler services are not | ||
| used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[...], but still are instantiated uselessly. (might not be the best phrasing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| Back to our CommandBus which would now look like:: | ||
| // ... | ||
| use Psr\Container\ContainerInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do you type hint against this interface? Simple closure is enough, isn't it?
$this->psr11->get(get_class($command)) makes me think thatFoo\RegisterUser is a service name, but semantically it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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'sMap: you need some generic explicit interface, withNotFound* exception andget/has methods, but PHP doesn't have it (ArrayAccess is kinda similar, though), so here we go.
Yes, PSR-11 mapsstring onobject, but semanticallystring here is notserviceId, 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 calledObject[Store|Provider|Locator|Registry] instead ofContainer.
unkind commentedFeb 10, 2017
@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 commentedFeb 10, 2017
@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). |
chalasr commentedFeb 10, 2017
Status: needs work (the core PR has been updated, now using the new custom tags feature). |
…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#7458This 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:```yamlApp\FooBar: [!service_locator { key1: '@Service1', key2: '@service2' }]``````xml<service public="false"> <argument type="service-locator"> <argument type="service" key="key1"/> <argument type="service" key="key2"/> </argument></service>``````phpnew 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
| return $handlerMap($handlerId)->handle($command); | ||
| And can be used in a loop to iterate over all available services:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should not forget to remove this, asServiceLocator doesn't implementIteratorAggregate anymore. :)
chalasr commentedFeb 13, 2017
I will add a second alternative to the first (bad) example: injecting the container. |
| // ... | ||
| $handlerMap = $this->handlerMap; | ||
| return $handlerMap($handlerId)->handle($command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
return call_user_func($this->handlerMap, $handlerId)->handle($command);?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$locateHandler =$this->handlerMap;return$locateHandler($handlerId)->handle($command);
| */ | ||
| private $handlerMap; | ||
| // ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You may write the constructor here to understand the correlation between the service declaration and the class instanciation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, definitely needed for a good understanding.
| public function handle(Command $command) | ||
| { | ||
| $commandClass = get_class($command); | ||
| $handler = $this->handlerMap->get($commandClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 callingCommandBus::handle(), it's the better example I found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :).
| .. tip:: | ||
| Not only services can be passed accessible through a service locator, | ||
| but all types which are supported by the configuration format used to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe a dot would fit after "used" as well? What about an example?
| public function handle(Command $command) | ||
| { | ||
| $commandClass = get_class($command); | ||
| $handler = $this->handlerMap->get($commandClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :).
| public function handle(Command $command) | ||
| { | ||
| if (isset($handlerMap[$commandClass = get_class($command)])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$this->handlerMap ?
(same below)
weaverryan commentedMar 17, 2017
This may have just changed due tosymfony/symfony#22024 |
chalasr commentedApr 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Finally took the time to finish this. Ready to review! |
HeahDude left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
| // ... | ||
| $locateHandler = $this->handlerLocator; | ||
| $handler = $locateHandler($commandClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not only$handler = $this->locateHandler($commandClass);?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess you mean$this->handlerLocator($commandClass) :).
Anyway, the intermediate variable is mandatory, calling$this->handlerLocator() would make the engine look for ahandlerLocator() method instead of executing the property as a callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, good to know. Let's stay simple there though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(But this is documentation. I think the current code sample is great for this purpose 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok guys, thanks!
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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!
| $container | ||
| ->register('app.command_handler_locator', ServiceLocator::class) | ||
| ->addTag('container.service_locator') | ||
| ->setArguments(array( |
meyerbaptisteMay 2, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I recently implemented a service locatorfor API Platform and the syntax described here is currently not the correct one.
According tosrc/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php#L40-L42, this should be:
->setArguments([ ['AppBundle\FooCommand' =>newReference('app.command_handler.foo'),'AppBundle\BarCommand' =>newReference('app.command_handler.bar'), ],])
Same for the YAML/XML config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, fixed. Thanks!
javiereguiluz commentedMay 4, 2017
@chalasr thanks for contributing these docs! |
…guiluz)This PR was merged into the master branch.Discussion----------[DI] Add section about service locatorsAdds documentation forsymfony/symfony#21553 andsymfony/symfony#22024.Any suggestion will be much appreciated, as usual.Commits-------fa19770 Fix service locator declarationf5e4942 Rewords5efacd0 [DI] Add section about Service Locators
Uh oh!
There was an error while loading.Please reload this page.
Adds documentation forsymfony/symfony#21553 andsymfony/symfony#22024.
Any suggestion will be much appreciated, as usual.