Cascading Locator

[rizqidjamaluddin]

Would there be interest in a locator that iteratively attempts to use a stack of other locators, continuing to the next when the first one fails? I've been using this a lot lately, generally to have the option of assigning explicit locators when necessary and a sensible default otherwise. Example snippet:

class CascadingLocator implements HandlerLocator {

    /** @var HandleLocator[] */
    protected $locators;

    public function __construct (array $locators) {
        array_walk($locators, function($i){ if(!$i instanceof HandlerLocator) throw new NotALocatorException; });
        $this->locators = $locators;
    }

    public function getHandlerForCommand (Command $command) {
        foreach ($this->locators as $locator) {
            try {
                $handler = $locator->getHandlerForCommand($command);
            } catch (MissingHandlerException $e) {
                continue;
            }
            return $handler;
        }
        throw MissingHandlerException::forCommand($command);
    }
}

And thus usage would be wrapped:

$explicitBindings = new InMemoryLocator;
$defaultNameTransposingLocator = new SomeCustomTransposingLocator;
new CommandHandlerMiddleware($extractor, new CascadingLocator([$explicitBindings, $defaultNameTransposingLocator]), $inflector);

A proper PR would, naturally, need tests, hence this just being an Issue for now. It's really just a utility class, so I'm not sure if it deserves its own separate repository.

[rosstuck]

Hi @rizqidjamaluddin! The idea of a Composite or Chain locator has come up a few times already, so it's safe to say there's interest.

I think the big question is should we add a hasHandler() method to the HandlerLocator interface, rather than using the exception as flow control? The recent split of the NameExtractor interface has also changed the semantics of this interface a bit since it now takes a string name instead of the actual command. I suspect that makes it a bit simpler to do here, but we'd need to update the Symfony and Silex providers though that shouldn't be too much work.

If you're interested in doing the hasHandler() refactoring, I'd be happy to see the PR and ensure the framework integrations get updated. Likewise, if anyone else has an alternative suggestions or better ideas, please pitch in. :smile:

If we did add this, it doesn't have any deps so I agree with just shipping it in the core package as a regular ol' HandlerLocator, no Plugins namespace necessary.

[rosstuck]

If you're not interested in doing the refactoring, btw, I can pick it up myself then swing back around to this but it'll be a couple of weeks probably.

[rtuin]

I think this needs a valid use case but I can't think of one. Not saying it's a bad idea, but my gut says that we don't need more than one HandlerLocator. Kinda missed why the Command interface was dropped, but the idea of contracting command types (via interfaces) to middlewares (and thus HandlerLocators) really appealed to me.

[rizqidjamaluddin]

@rosstuck - I think the exceptions-as-flow control is indeed the big question. One potential issue with using a try-catch I see is that a custom locator might be throwing a more advanced MissingHandlerException (e.g. with details about why it can't find a handler) and that information would be lost in the process. Adding hasHandler() would introduce a backwards compatibility break, though, is that alright? I imagine lots of people have custom locators out there.

@rtuin - for me, I write large modular apps, often with legacy components or with modules being migrated over from another command bus. Sometimes certain parts of the application will have a different command-handler mapping scheme than others that isn't easily refactored (or refactoring on the spot isn't an option). Usually this means having the ability to explicitly set a handler with InMemoryLocator while other, more sensible parts of code can default to a standard container-locator. Or a packaged-based setup where individual packages could register their own handler bindings while going through a single shared bus.

For that matter, would it make more sense to have the CascadingLocator allow more locators to be added in runtime? One could have a single global instance and then packages would implement their own locators for their classes.

[rosstuck]

@rtuin I've heard of at least two other Tactician users doing something similar so there's apparently a demand for it. Both were in brownfield contexts, if you're in a pure greenfield situation it might be less common indeed.

Regarding the Command interface, check out #43 for the discussion. I would say there's nothing wrong with your app or some plugins defining their own marker interfaces (Bernard integration still has a QueueableCommand interface, for example). I just feel it's better that comes from your own app, rather than being forced on you by Tactician itself. But the ability to shuffle off certain commands by interfaces is something we absolutely must still support.

@rizqidjamaluddin I'm okay with a minor BC break here, we're still pre-1.0 and can update the Symfony, Silex and League containers without much hassle so most users probably won't notice. The interface change is mainly additive.

I do wonder about changing how the CommandNameExtractors work: right now we pass them directly to the CommandHandlerMiddleware but perhaps it's better to make them constructor dependencies for most HandlerLocators because I imagine the string produced by the CommandNameExtractor is likely quite unique to the HandlerLocator backend. That may not be an issue admittedly, it's just a thought. If you can give it a mental test against your scenarios and let me know what you think, I'd much appreciate it.

[rizqidjamaluddin]

I actually only just saw the Extractor classes. I don't think they're very relevant for me, but that's probably because I've always used FQCNs for command identities. I could see people running into the same issue as I'm having with Locators, though - different bits of code identifying commands in different ways. I think we need more information to decide on that.

[rosstuck]

Indeed, I can see it as a real issue. Also, for folks who want a really hardcoded solution unique for them, they can skip the CommandNameExtractor part (if they choose) and go straight from Command->Handler in their mapping.

Going to ping @sagikazarmark because he did all of the hard work on CommandNameExtractors, curious what he thinks. :)

[rizqidjamaluddin]

At the end of the day, I think the real goal is to allow a single command bus to serve a variety of commands/handlers and the relationships between them - e.g. a person who used laravel's self-handling commands wants to keep their old setup while moving to separated commands/handlers in the long run. Maybe we're approaching it inefficiently by looking at individual locators and extractors, when what we really want is a composite command handler?

[sagikazarmark]

Let me correct one thing first:

Bernard integration still has a QueueableCommand interface, for example

This is false, it has been removed as it was a direct child of Bernard`s message interface. Actually, this is a great example why Commaind interface gone was a good decision.

Regarding the catch/hasHandler issue:

Both have pros and cons. Using try-catch blocks for something we expect is ugly. Also adds a small amount of overhead.

hasHandler seems to be a better choice, although in some cases where handlers are not resolved from a static, but a dynamic map it might also add some unwanted behavior. Also adding something to the interface for the sake of one handler doesn't seem to be a good idea to me.

As a solution we could add a ChainableHandler interface which contains this method. The core locators could implement this interface, but this way implementing it for custom ones is not necessary.

As for the extraction thing:

I think that is the hardest question of all. I agree that in some cases the extraction logic does not add any value (custom locators, etc). But I have two issues making some locators accepting a name extractor:

• It adds complexity to the object graph
• It adds a new component to the object construction

So I agree name extraction logic could be a dependency of locators, but I don't have an idea how it could be nicely done.

[sagikazarmark]

Here is what I have come up with regarding name extraction:

1. We create a HandlerLocatorByName interface which CAN be implemented by a locator and adds a locateByName method.
2. We create something like a CommandNameExtractorAcceptingLocatorSomethingWithAMuchBetterName which is a decorator around a HandlerLocatorByName interface. It also accepts a command name extractor.

(Following this way command name extraction can even go into a plugin)

[rosstuck]

As an alternative, I've created an ugly POC about how we could pull this off, feedback is welcome: https://github.com/thephpleague/tactician/commit/e8540a3ff725046d9b015e0f149f3848aff2a391

The POC adds cascading locators naturally, as well as a self-executing command setup as part of core. It also lets users just knock out their own hard-coded execution strategies if they want or reuse our bits through composition.

A few points:

• This isn't a complete POC. Tests need to be updated, class locations aren't final.
• I'd probably end up moving CommandHandlerMiddleware to Execution, it just cluttered the diff here.
• If we move to a pure callback type scenario, that might open the door to some changes in the middleware $next chain, not sure about it yet though.
• There's some functional concept I'm surely missing here. :grin:

All in all, I'm not sure about this one, folks: it adds the missing features but we get an even more complicated setup phase. Not sure it's worth the tradeoffs here. :-/

Thoughts?

[sagikazarmark]

Sorry, what? :smiley:

I am feeling this is the scenario of "Bringing a gun to a knife-fight".

[rosstuck]

I think this is the only really "clean" way of implementing it. We can go a step further and remove the CommandHandlerMiddleware, making the execution strategy the final step of the callable chain always. That would make it a little more direct. We could also refactor to bring this stuff closer to the command bus, splitting MiddlewareChain out to its own object so the CommandBus can implement this stuff more freely.

Either way, it seems like we just can't do this easily with the current Tactician setup so we may need to pass on this feature or implement the Exception based version.

[sagikazarmark]

Actually, we are talking about two features, aren't we?

1. CommandNameExtractors to be moved somewhere else
2. Chained locators

IMO rather the exception based thing than following such a "clean" way.

[rosstuck]

If we don't move Extractors, I fear chained locators will be greatly handicapped since odds are two different containers won't use the same naming convention.

@rizqidjamaluddin I'm open to taking the Exception based version, albeit with some design regrets. We'll just have to think harder about this scenario come Tactician 2.0, sorry. :)

[sagikazarmark]

Why the release if we just wanted to move the extractors out of the handler middleware? Also, what do you think about the interface idea i mentioned earlier?

[rosstuck]

Going to close this one now, will review all the 2.0 tagged stuff probably sometime next year.

Thanks again to everyone for contributing to this thread with their ideas. 😻