-
Notifications
You must be signed in to change notification settings - Fork 88
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate Command interface #43
Comments
Was never a fan of the interface. +1 |
+1
For my own clarity, are some of these coupling arguments similar to this issue I had with Simple Bus SimpleBus/CommandBus#2 or something else entirely? |
Well I personally like this interface for commands since it allows programmers to be more specific in there code. I also think that for code/system analyzing tools it's a minor disadvantage not having the interface. For example I expect that someone writes a tool that will check if all there commands have a command handler using the interface this is relatively simple but without it..... Also if the interface is removed why do the is_object check? I mean a (hash/)array could also be a valid command in that case or a closure or a purple elephant. I'm 👎 on this proposal. |
@jenkoian Hey, long time, no see! Hope you're doing well. 😄 I hadn't seen the SimpleBus issue but that sounds a bit similar to one of the concerns I had. @boekkooi I also like the specificity of it but it does impose a small burden. On the static analysis front, I've been thinking about adding a Tactician Metadata package which would let us supplement commands with extra metadata from annotations/xml/yml, conceivably for use in other packages as well (think queueing options) etc. This would be an opt-in thing so you couldn't drop it entirely in on every Tactician project but for folks who want that sort of stricter handling (i.e. us) we could require that (say, add a middleware that errors out if you haven't registered the command) and not require anyone else to come along for the ride. That's way outside the scope of this ticket but it's one alternative. About the is_object check: we'd still typehint it in the docblock as object but this forces it more strictly. It's also political, I really think Commands should be objects/structs as we discussed but not everyone feels the same, so I'd like to be a bit more firm so people don't start abusing Tactician the same way. That said, purple elephants would rock and should be a supported use case. 🐘 😄 @acairns Thanks! Side note: That example still won't work in this case though because strict error handling (rightly) won't allow the child interface to be more specific than the parent. |
@rosstuck yeah, realised just before you replied and deleted the post - it was a bad example for the point I was trying to make. I'll go get that coffee now! :) |
@boekkooi The perfect example is Doctrine's Entities. There isn't any common interface for them. However I see your point as well, having an interface like that makes you feel more "comfortable" and makes your command a command (literally). But your is_object concern is not valid. Expecting an object is as good as expecting a specific Type. (I wish there was an object type hint in PHP) Again: doctrine expects objects as entites with a cause. Although the use case is not the same. the analogy is. |
All packages should now be updated, thanks to everyone for your feedback! 😄 |
We originally added the Command interface when we were using decorators rather than middleware. This was useful because we can't just typehint for an object, but otherwise the interface has no real value except for arguably doing static analysis like building a list of all commands in the system. That would be a cute feature but it's not really that valuable.
I personally liked the original POPO approach to Tactician. Also, there are some coupling arguments and library integrations that've come up recently where the Command interface has been seen as a disadvantage.
Therefore, I'm going to deprecate the Command interface and replace it with an is_object check in the
CommandBus
instead. There's really only one Command Bus instance that'll be feasibly used here so there's little reason to do otherwise, I think.This (along with a small change to CommandHandlerMiddleware) will probably form the basis for 0.4. When I have time to make these changes, I'll probably make PRs to all the related projects as well to start removing the interface.
If there's any major objections, we can leave it in for a release or two to provide more migration time.
The text was updated successfully, but these errors were encountered: