Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The order entity and interface should not need to have any knowledge of uc_payment. This means removing the getter/setter from the interface, using hook_entity_base_field_info() to add the payment method field to the entity, and using $order->get('payment_method') to access the field value.
This can wait until #2620956: Convert payment methods and gateways into configuration entities is committed.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | decouple-payment.patch | 15.5 KB | longwave |
Comments
Comment #2
longwaveComment #4
longwaveComment #5
longwaveComment #6
TR CreditAttribution: TR commentedI'm not sure I understand the logic for this. I don't see this as decoupling uc_payment from the order entity, I see it as removing the abstraction of payment and replacing it with a concrete implementation, which will complicate things if we intend to support payment methods other than uc_payment.
Specifically, orders always need payments, even free orders. That's the definition of an order in an e-commerce system. Right now, we have a base field which stores a string payment_method to hold the id of the payment method. It used to hold the id defined in hook_uc_payment_method(), but now that payment methods are configuration entities it holds the config entity id of the method. Right now, we have methods defined on the order entity which give other modules access to the payment method used. Providing methods allows us to hide the implementation - right now we use a string base field but in principle this can be changed to e.g. an entity reference to the payment method without having to change any modules that use the Order.
When I see things like:
what I see is a step backwards - the class variable payment_method which used to be a protected implementation detail is now exposed to the world and is something we now have to set and get directly, breaking encapsulation.
Instead, I would go the opposite direction:
where getPaymentMethod() returns an object implementing the PaymentMethodInterface, and getId() is a method defined on that interface (which would function like getPlugin()->id()). This has the additional advantage that this data we're returning is now typed, which means we have API documentation about what it means and how to use it, unlike in the previous case where all we have is a string being passed around with simply an undocumented understanding of what it's supposed to hold and nothing to ensure/check that the contents mean anything.
To go further, we could define a ubercart.payment service with an interface that defines what Ubercart needs in terms of capability from a payment system. Then switching between uc_payment and Payment for example, or even using both at the same time, could be done simply by choosing a different service implementation at runtime.
Comment #7
longwaveThe logic behind this is that payment_ubercart or similar might eventually entirely replace uc_payment. Payment could add whatever fields it needs directly to the order entity, without the need for any glue code at all from uc_payment. Payment also provides its own transaction entities so we would not need duplicate storage in uc_payment_receipts (which we should convert to entities anyway, but that's another issue). It seemed to me that there was so much duplication between uc_payment and Payment that it would be cleaner to remove uc_payment entirely if possible, rather than have to maintain a compatibility layer between the two.
I am not sure I see the need of defining a service here either; again it would just be another compatibility layer to maintain.
Comment #8
longwaveWhile working on #2681831: Convert uc_payment to use entities I had an idea that might let us use interfaces while decoupling payment (and anything else we want) from orders: subclass the entity storage class for orders, and then use the decorator pattern in either ::postLoad() or ::loadMultiple() to wrap Order objects in a set of extra interfaces immediately after they are loaded, e.g. PayableOrderInterface which could provide getPaymentMethod(), getPayments(), getBalance(), etc. on top of the OrderInterface methods, but only if uc_payment is installed. Not sure if this will work but it might be worth a try at least.
Comment #9
longwaveThis would also let us properly specify $order->payment_method as an entity reference rather than a string. At the moment this cannot be done as the target entity type is not guaranteed to exist.
Comment #10
TR CreditAttribution: TR commentedI like the idea in #8. It's certainly worth working on. The same strategy can be use for shipping quotes I think, to provide method for shippable orders.