Consider not enforcing command class existence

[Richtermeister]

Hi all! First off, great bundle, really appreciate how everything is laid out and works out of the box.

There is one issue that I ran into, tho, and wanted to inquire:

In my current application I have a lot of similar commands (crud operations, basically), and instead of maintaining separate CreateXYZCommand classes, I only have one command class ResourceCommand($resource, $action, $data) This works well as far as Tactician is concerned, since they allow me to specify a CommandNameExtractor, and they don't enforce that commands actually exist as classes.

In https://github.com/thephpleague/tactician-bundle/blob/master/src/DependencyInjection/HandlerMapping/Routing.php#L68 however, this is being enforced, and since this is a final class that is referenced directly by client code, it cannot be cleanly bypassed (I am currently overriding the entire file via composer autoload hackery).

So, I was wondering if it is important to you to check for command existence, considering Tactician itself does not impose this restriction. If not, I would love to help remove it :)

Thank you for your time and consideration, – Daniel

[tyx]

Hello !

I'm not sure to get your point but what about writing a dedicated middleware that rewrite your command ?

[rosstuck]

Hi, sorry for the late response but from the Tactician side of the things, I don't think this is something that will change. Counting on the class to actually exist is useful in a number of circumstances (for example, if we ever add more static analysis features). I think @tyx has a good suggestion here, a custom middleware would do the trick nicely.

[Richtermeister]

@rosstuck Thank you for your reply, but I'm not sure I have made my case properly. Please allow me to try again.

Based on the existence of the CommandNameExtractor interface and its description:

Extract the name from a command so that the name can be determined by the context better than simply the class name

it seems implied that commands are meant to be logical concepts, and do not necessarily have to each be backed by a specific class.

Tactician itself currently allows me to come up with arbitrary command names, as long as I provide a HandlerLocator to match them back to a handler, and by itself all that works great. Requiring commands must exist based on class name alone would be a BC break for Tactician, if introduced now, as anybody can currently rely on this extract/map feature.

However, it is this bundle which imposes this "commands must exist as classes" restriction. Given that Tactician itself does not impose it, it seems arbitrary and unnecessary, and due to the way it is implemented, it is impossible to circumvent.

In my case, this circumstance would force me to create ~30 "identical" classes (CreateUserCommand, ...) instead of just using one command (ResourceCommand("User", "create", $data)), which the CommandNameExtractor could easily rewrite to CreateUserResourceCommand and then be mapped to a $userResourceHandler->create($command) method. Multiply this over dozens of resources all handled similarly..

If this fails to convince, I would like to understand how a middleware can help me "rewrite" my command, since the resulting command must apparently be a real class. Are you recommending to generate these classes via code-gen?

Thank you for your consideration, – Daniel

[Richtermeister]

Also, to clarify, there is an actual command object in play, but just one: ResourceCommand, carrying different payloads based on intent. Since routing currently relies on the command name, this is not enough to map to a handler. I am not requesting a way to not have any commands, and as far as static analysis is concerned, a ResourceCommand is dispatched into the Bus, and a ResourceCommand is received by the handler. My issue only concerns the routing.

[tyx]

Ok I get your point I think https://github.com/thephpleague/tactician-bundle/blob/master/src/DependencyInjection/HandlerMapping/ClassNameMapping.php#L13

We enforce class existence here indeed.

But it is only a default mapping strategy, you can override this behavior by writing your own mapping strategy. https://github.com/thephpleague/tactician-bundle/blob/bf10a37e27e4306bb9de5d909dba36226aa8e3c4/src/TacticianBundle.php#L20-L25

Then you will just need to provide you own command name extractor that will read your ResourceCommand and transform it to your NotRealCommand to allow your handler locator (that will no longer ensure class exists) to map to your handler that will definitively receive the initial command.