Improve integration DX in regards to model interfaces

bojanz commented 9 years ago

Right now all of the commerceguys libraries provide interfaces with getters and setters, since they are aimed to be implemented by entities. (Address is a future value object, but let's disregard that one).

Such an interface looks like this, and might type hint other interfaces:

    /**
     * Adds a tax rate.
     *
     * @param TaxRateInterface $rate The tax rate.
     */
    public function addRate(TaxRateInterface $rate);

Implementation has shown that applications often need to extend the provided interfaces. For example, a \Drupal\commerce_tax\TaxRateInterface extends the library TaxRateInterface and Drupal's generic ConfigEntityInterface. There might also be additional getters/setters added (for a createdAt field, for example). However, the original type hints can't be changed in any way. So we end up with some entities type hinting the Drupal interface, and some type hinting the library interface, which brings us confusing DX.

There are two solutions for this: 1) Remove all type hints from the interfaces, and resolve them at runtime. We already did this for the collection setters: https://github.com/commerceguys/tax/blob/master/src/Model/TaxType.php#L255

2) Split the setters to their own, optional interface, since they aren't actually required by other (service) classes. (selected solution)

So we'd have a TaxRateInterface with only getters, and a TaxRateEntityInterface that extends the first one, and adds setters. The library entity (used by Doctrine) implements TaxRateEntityInterface, service classes continue to typehint TaxRateInterface. This would allow implementing applications to copy paste TaxRateEntityInterface methods to their own interface, replacing the type hints, adding other methods.

rszrama commented 9 years ago

I've noticed the same issue working with Drupal entities in phpStorm, but I'm not convinced we should be retooling our interfaces simply to address type hinting challenges. Or if we are, I wonder if it wouldn't be better to leave our interfaces as they are and make use of inline @var comments.

donquixote commented 9 years ago

General feedback, without much background in Commerce API - at the risk that you know this stuff already:

I rarely make setters part of the interface. This allows for implementations without setters, for mocking or other purposes. And overall, I would recommend to aim for immutable objects. Either have everything in the constructor, or at least use the setters only in the beginning when the object is being created.

For parameter type hints, if the interface is not strict enough, you should use instanceof to make sure you have exactly the type required for this implementation. Don't rely on the caller. The instanceof also tells the IDE which type it is, but it is not "cheating" like a @var docblock.

EclipseGc commented 9 years ago

For what it's worth, I totally support removing the setters as well. Setter based injection is awful, and if it can be avoided, it should be. I realize that an entity/orm style system is going to want/need setters, but that's because the data is being saved somewhere and manipulated over time. In the case of dumb data objects with little or no real logic, setters just complicate things. It'd be better to do constructor based injection, and just create a new data object when it needs to changed. This is pretty typical in OO.

For the notion of the setters themselves, providing a trait with our setters and opting into using it seems the better solution. You could even provide a "MutableWhateverInterface" per class and have the traits fulfill that interface. This gives Drupal and Doctrine and others a good starting position from which to extend in ways specific to their needs without compromising the underlying architecture of the components themselves.

Just my 2 cents.

Eclipse

donquixote commented 9 years ago

And btw even if typical ORMs might have the same entity object for read/output as for modifying and interaction with storage, imo it is better to separate those. On Sep 2, 2015 5:05 PM, "Kris Vanderwater" notifications@github.com wrote:

For what it's worth, I totally support removing the setters as well. Setter based injection is awful, and if it can be avoided, it should be. I realize that an entity/orm style system is going to want/need setters, but that's because the data is being saved somewhere and manipulated over time. In the case of dumb data objects with little or no real logic, setters just complicate things. It'd be better to do constructor based injection, and just create a new data object when it needs to changed. This is pretty typical in OO.

For the notion of the setters themselves, providing a trait with our setters and opting into using it seems the better solution. You could even provide a "MutableWhateverInterface" per class and have the traits fulfill that interface. This gives Drupal and Doctrine and others a good starting position from which to extend in ways specific to their needs without compromising the underlying architecture of the components themselves.

Just my 2 cents.

Eclipse

— Reply to this email directly or view it on GitHub https://github.com/commerceguys/tax/issues/26#issuecomment-137116110.

bojanz commented 9 years ago

Implemented and committed #2.

commerceguys / tax

Improve integration DX in regards to model interfaces #26