Skip to content
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

Closed
rosstuck opened this issue Mar 25, 2015 · 7 comments
Closed

Deprecate Command interface #43

rosstuck opened this issue Mar 25, 2015 · 7 comments

Comments

@rosstuck
Copy link
Member

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.

@acairns
Copy link
Contributor

acairns commented Mar 25, 2015

Was never a fan of the interface.

+1

@jenkoian
Copy link

+1

there are some coupling arguments and library integrations that've come up recently where the Command interface has been seen as a disadvantage

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?

@boekkooi
Copy link
Contributor

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.
And in case someone really really needs to pass in a plain object then just create a wrapper command and a custom locator for that.

I'm 👎 on this proposal.

@rosstuck
Copy link
Member Author

@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.

@acairns
Copy link
Contributor

acairns commented Mar 25, 2015

@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! :)

@sagikazarmark
Copy link
Member

@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.

@rosstuck
Copy link
Member Author

All packages should now be updated, thanks to everyone for your feedback! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants