Separate command and options

[shadowhand]

Having separate value objects for options makes it easier to understand
what options are available to a command and simplifies the validation
process.

This change was implemented as separate classes so that usage can be
upgraded incrementally and version 2.0 can remove the deprecated things.

[shadowhand]

Should we separate the the immutable traits from this package to avoid version conflicts?

[elazar]

Not sure I follow. Why would the immutable traits not being separate cause version conflicts?

[shadowhand]

equip/data would depend on equip/immutable and this version would depend on equip/immutable only.

[shadowhand]

The goal is to prevent packages that depend on equip/data from having conflicts with this package.

[elazar]

Are we anticipating those sorts of conflicts would happen often enough to warrant the overhead that comes from maintaining an entirely separate repository?

[shadowhand]

equip/immutable#1 here's what it would look like.

[elazar]

What it could look like isn't difficult to visualize, but that doesn't address the points I expressed above. How many use cases are we likely to encounter where we'd want to use the immutable traits apart from the repository and entity implementations, even assuming this repository did serve as a solitary example? I'm not convinced it's worth the trouble of separation and long-term maintenance versus pulling in a bit more code from api-commands than might be needed for this particular use case.

[shadowhand]

Anywhere you want an immutable value object without entity/repo stuff.

[shadowhand]

This could be really useful for notifications too.

[elazar]

Anywhere you want an immutable value object without entity/repo stuff.

Well, yeah. I included that in the question when I asked it. This doesn't answer it. I'm looking for specifics.

This could be really useful for notifications too.

How so? Would we not have entities or repositories for notifications?

[shadowhand]

This is not modified, just moved.

[shadowhand]

@equip/contributors CR please!

[elazar]

Having separate value objects for options makes it easier to understand what options are available to a command and simplifies the validation process.

Not sure I see how these changes accomplish what's described above. Can you give specific use cases that these changes would improve over the existing code?

[shadowhand]

@elazar for example if we have a command that allows for some options that are not required and do not have defaults (say, "employee code") then having a value object makes it much more clear that this option exists.

Currently in many of our commands we are not sure of what all the possible options are, so having a value object makes a source of truth for these scenarios. It also makes the command implementation easier and separates concerns better, so that the command objects can contain less option checking.

[shadowhand]

The code in this scenario would look something like:

final class CreateUserOptions extends Options
{
    protected $first_name;
    protected $last_name;
    protected $email;
    protected $password;
    protected $phone_number;
    protected $employee_code;
    protected $role = UsersRepository::ROLE_EMPLOYEE;
}

Edit: updated to match PR changes.

[shadowhand]

@equip/contributors anyone else have thoughts about this?

[elazar]

I think representing required options within OptionsTrait may be a flawed approach. It's possible two commands could take the same set of options, but require different options in that set (e.g. creating a user, where we'd want to require an e-mail address, versus updating a user, where we don't necessarily need an e-mail address unless it's being changed). By coupling requirements to the options themselves (versus the commands using them), we lose the ability to support this use case without adding further complexity.

I think having value objects for options is a good idea, but I also think it may be possible to implement it satisfactorily without deprecating existing code or breaking backward compatibility. For example, we could provide a generic implementation of OptionsInterface and, if an array is passed in for options, simply wrap it in an instance of that generic class within AbstractCommand. This leaves the possibility open for us to change the signature of withOptions() in the future to type hint against OptionsInterface in a future release and to simply recommend switching to use of OptionsInterface implementations in the interim.

[shadowhand]

@elazar any further thoughts on this? I've updated my example code and switched to using an abstract class for Options (and no interface) because it's a lot more convenient.

[elazar]

@shadowhand I don't follow: how is it more convenient to have no interface? This PR seems to be moving further away from design by contract and favoring composition over inheritance. I can see having a stock implementation of said interface, but I don't see why the command and options classes can't be implementations of an interface to allow for alternative implementations.

[shadowhand]

@elazar YAGNI. The classes are so simple and high level that I don't see why anyone would need the interfaces. Type hinting against a CommandInterface gets you nothing, since every command implementation is totally unique and not swappable. There might be some value in an OptionsInterface but I don't want to add it unnecessarily. It is trivial to add if someone actually requests it.

[ameech]

This looks reasonable @shadowhand. Is this something we still want to move forward with?

[shadowhand]

Corrected this type hint, as per this Scrutinizer issue.

[shadowhand]

@ameech @elazar check this again please? I'd like to move forward with this.

[elazar]

No immutable mutators (i.e. with*() methods)? If not, that's fine, just curious.

[shadowhand]

No, because options cannot be modified after construction.

[elazar]

👍

[shadowhand]

Released in 1.3.0.