Add Locator to retrieve Handlers from a Container #66

mvriel · 2015-06-01T16:52:32Z

In this PR I have added a Locator that is capable of retrieving
a handler from a Container implementing the Container Interop
standard / shared interface.

The Locator's default behaviour is to append the suffix 'Handler' to
the given Command Name (assuming the Command Name is an FQCN)
and uses that to find a handler in the Container that is provided
in the constructor.

A secondary behaviour for the Locator is that you can pass a map with
which you can map command names to specific identifiers with which
a handler can be returned. This is especially convenient when you are
using a container that operates based on identifiers and not class
names.

This specific locator depends on the container-interop/container-interop
package but I have explicitly mentioned this in the suggest section
of the composer because it is not a hard requirement for the usage of
Tactician, only when you want to use this new Locator.

Using unit tests and by manually performing tests using dummy data
I have been able to verify that no errors occur due to omitting this
dependency by default.

The DocBlock mentions in its example that the getHandlerForCommandMethod accepts an object (the Command) but in reality it accepts a string. I have altered the DocBlock to match this behaviour.

In this Commit I have added a Locator that is capable of retrieving a handler from a Container implementing the Container Interop standard / shared interface. The Locator's default behaviour is to append the suffix 'Handler' to the given Command Name (assuming the Command Name is an FQCN) and uses that to find a handler in the Container that is provided in the constructor. A secondary behaviour for the Locator is that you can pass a map with which you can map command names to specific identifiers with which a handler can be returned. This is especially convenient when you are using a container that operates based on identifiers and not class names. This specific locator depends on the `container-interop/container-interop` package but I have explicitly mentioned this in the suggest section of the composer because it is not a hard requirement for the usage of Tactician, only when you want to use this new Locator. Using unit tests and by manually performing tests using dummy data I have been able to verify that no errors occur due to omitting this dependency by default.

rosstuck · 2015-06-01T19:40:52Z

This looks great! Normally, we'd split the container-interop part out to a separate plugin package but since it's just interfaces (and this is an oft-requested feature), I think the soft dep is pretty justifiable.

The one thing I think might be worth refactoring here is the behavior around command name. Technically, that ought to be in a CommandNameExtractor implementation, rather than a CommandLocator. The Handler suffix version is pretty clever and could be really useful to a lot of people, even those who don't want to use a container backend. Likewise, we could do the same for a CommandClassName => mapped string version (I'm genuinely surprised we didn't bother building that already. D'oh!)

So, if we split that behavior out into two different CommandNameExtractors, this would be perfect and useful to a broader audience; they'd just need to pick which one to use. If you're really counting on the fallback from one to another there, check out #55, we could do something similar there.

mvriel · 2015-06-01T20:50:11Z

I am sorry but I am having trouble understanding which behaviour surrounding the command name that you'd like to extract to a CommandNameExtractor. Do you mean that a CommandNameExtractor should also be used for a CommandHandlerName?

To me the strategy determining the Command Handler's name is a responsibility of the Locator; or at least delegated by the Locator. This would mean that the constructor of the ContainerLoader would feature another dependency for the Command Handler Name Extractor.

And do I get it right that you identify the following patterns that you'd like to have extracted into CommandNameExtractors:

Suffix a string, defaults to 'Handler', to the given Command's FQCN
Given a lookup table to the CommandNameExtractor, try to map the FQCN of the Command to a value in that lookup table

The reason for the fallback behaviour is because I can imagine three scenarios:

The container uses identifiers to find the command handler with (the lookup)
The container uses handler classes that are not suffixed with 'Handler' but may even be in a different namespace (the lookup again)
The container uses a simplified lookup mechanism where 'Handler' is added as a suffix to the Command's FQCN (the Handler thing).

Since these two differ only slightly I thought I'd combine them into one as it feels like overkill to separate them. Though if we were to use CommandHandlerNameExtractors as described above then this could be used as a strategy and there is no need for separate versions

mvriel · 2015-06-01T21:15:00Z

@rosstuck I have pushed a proof of concept based on your remarks; is this what you mean?
There is a distinct difference between a CommandNameExtractor and a CommandHandlerNameExtractor though; in the Locators we only know the FQCN of the Command but don't have the actual object, meaning that a Command Handler Name Extractor must accept a string.

A CommandNameExtractor however receives a Command object; meaning that these differ in what they accept.

mvriel · 2015-06-01T21:15:22Z

Also: if this is what you mean I will fix the tests; didn't bother right now (also due to time ;) )

rosstuck · 2015-06-02T07:15:36Z

In Tactician, we don't really have a separation between the CommandName and the HandlerName (there isn't really the concept of a HandlerName at all). The CommandNameExtractor just figures out what the identifier should be and the HandlerLocator returns the one for that identifier.

So, in this case, we wouldn't have a CommandHandlerNameExtractor, we'd just use the CommandNameExtractor. Because there's no other real use case for CommandNameExtractors anyways.

Anyways, that'd be the quick solution for getting this merged. :)

On a future oriented note: That's the way it works now, since it lets folks change their naming strategy separate from their container, which was a real issue when we had the two coupled. I admit it's a bit suboptimal, this was a later refactoring and we probably need to address this better in the next release or 2.0. You can see the issues it's caused with Cascading Locators in #55.

At this point, my thought is making the HandlerLocator itself implement container-interop (been pondering this for a while and would solve a few issues) or passing the Extractor as a parameter somehow so the two can evolve separately.

If you've got any thoughts about that, lemme know. :)

mvriel · 2015-06-02T07:18:56Z

@rosstuck There is just one problem with the CommandNameExtractor: The rest of the application passes a Command object while the Locators pass a Command FQCN (hence: string). This means that the current implementation of the Locators cannot consume normal CommandNameExtractors

rosstuck · 2015-06-02T07:22:29Z

Sorry, to clarify: the locators aren't meant to purely work based on FQCN. They're meant to just receive any string name. The extractors can return anything: Symfony service or tag name or the result of a $command->getCommandName() or whatever you'd like.

In the original version, the CommandLocators just took the entire command object, then implemented their own location logic. But that caused coupling issues when people had different styles of service naming and then had to reimplement the entire Locator just to change that part.

At the time, we also considered passing the Extractor as a dependency but for wiring together in frameworks, that meant either required constructor position (no go) or making it a parameter in the $locator->getHandler call, but then we might as well unwrap it (which is what we did)

mvriel · 2015-06-02T07:34:07Z

@rosstuck the problem is not de output or its meaning of the Locator; the problem is the input of the CommandNameExtractor's extract method.

The CommandNameExtractor interface requires that the extract method receives an object; but the Locator does not have an object.

rosstuck · 2015-06-02T07:37:19Z

Indeed, I think we're in agreement but need to sync up a bit. I've got to start the work day here, maybe catch up tonight on IRC or Slack? 😸

mvriel · 2015-06-02T07:38:53Z

I can be a bit busy this evening but I will definitely try to get a hold of you ;)

malukenho · 2015-06-02T13:13:14Z

src/Handler/Locator/InMemoryLocator.php

@@ -15,7 +15,7 @@
 *      $inMemoryLocator->addHandler($myHandler, 'My\TaskAddedCommand');
 *
 *      // Returns $myHandler
- *      $inMemoryLocator->getHandlerForCommand(new My\TaskAddedCommand());
+ *      $inMemoryLocator->getHandlerForCommand('My\TaskAddedCommand');


can be My\TaskAddedCommand::class ?

That is an allowed notation; I have explicitly used the pre-php 5.5 notation because this library its minimum version is not 5.5

Naw, Tactician is only >= 5.5 😄

Holy smokes! I have totally had that wrong in my head! :D

@mvriel @rosstuck

Given the time that has passed, and given that PHP 5.5 is not supported anymore, maybe we can update this?

Also see #102.

localheinz · 2016-07-23T18:52:24Z

src/Handler/Locator/ContainerLocator.php

+final class ContainerLocator implements HandlerLocator
+{
+    /** @var ContainerInterface */
+    private $container;


@mvriel

For consistency, should we turn this into a multi-line docblock, rather than single line comment

/** * @var ContainerInterface */

?

rosstuck · 2016-07-23T18:54:08Z

Closing this before @localheinz gets carried away :)

The container-interop work here is now fulfilled by the new version of https://github.com/thephpleague/tactician-container. The other part involving a command name extractor probably needs to be picked up separately. If this is something we'd like to carry forward with perhaps it's a good idea to restart this one from scratch sometime :)

localheinz · 2016-07-23T18:54:46Z

@rosstuck

Ha, nice! Thanks for the heads-up!

mvriel added 2 commits June 1, 2015 07:35

Match DocBlock with Implementation

274dfae

The DocBlock mentions in its example that the getHandlerForCommandMethod accepts an object (the Command) but in reality it accepts a string. I have altered the DocBlock to match this behaviour.

Refactor ContainerLocator to use a CommandHandlerNameExtractor

441a498

malukenho reviewed Jun 2, 2015
View reviewed changes

rosstuck closed this Jul 23, 2016

localheinz reviewed Jul 23, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Locator to retrieve Handlers from a Container #66

Add Locator to retrieve Handlers from a Container #66

mvriel commented Jun 1, 2015

rosstuck commented Jun 1, 2015

mvriel commented Jun 1, 2015

mvriel commented Jun 1, 2015

mvriel commented Jun 1, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

malukenho Jun 2, 2015

mvriel Jun 2, 2015

rosstuck Jun 2, 2015

mvriel Jun 2, 2015

localheinz Jul 23, 2016

localheinz Jul 23, 2016

rosstuck commented Jul 23, 2016

localheinz commented Jul 23, 2016

Add Locator to retrieve Handlers from a Container #66

Add Locator to retrieve Handlers from a Container #66

Conversation

mvriel commented Jun 1, 2015

rosstuck commented Jun 1, 2015

mvriel commented Jun 1, 2015

mvriel commented Jun 1, 2015

mvriel commented Jun 1, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

rosstuck commented Jun 2, 2015

mvriel commented Jun 2, 2015

malukenho Jun 2, 2015

Choose a reason for hiding this comment

mvriel Jun 2, 2015

Choose a reason for hiding this comment

rosstuck Jun 2, 2015

Choose a reason for hiding this comment

mvriel Jun 2, 2015

Choose a reason for hiding this comment

localheinz Jul 23, 2016

Choose a reason for hiding this comment

localheinz Jul 23, 2016

Choose a reason for hiding this comment

rosstuck commented Jul 23, 2016

localheinz commented Jul 23, 2016