Problem
The PurchasableEntityInterface specifies a getPrice() method that should allow you to add orderItems for any entity that implements it. The DefaultPriceResolver does not support this method, and instead requires a price field. Any Entity implementing PurchasableEntityInterface and computing it's price in getPrice() causes Fatal Errors whenever the OrderRefresh service is used:
OrderItem::setUnitPrice() must be an instance of Drupal\commerce_price\Price, null given
Solution
DefaultPriceResolver should call the getPrice() method on the purchasable entity instead of looking for a field value.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3005098-10.patch | 3.86 KB | rlmumford |
| #7 | 3005098-6-bc-break.patch | 833 bytes | bojanz |
| #4 | 3005098-4.patch | 2.43 KB | rlmumford |
Comments
Comment #2
rlmumfordThis major bug was introduced by #2941401. I can't see any documentation there discussing why this was introduced. The recommended solution at the top of the ticket still makes use of the getPrice method.
Comment #3
rlmumfordComment #4
rlmumfordHere is a patch that hits the stated aim of issue #2941401 without breaking all other entities that implement PurchasableEntityInterface.
Comment #5
rlmumfordComment #6
bojanz commentedThis would be an even bigger BC break.
We have two options here for fixing DefaultPriceResolver:
1) Add a getListPrice() to PurchasableEntityInterface, make the method then call either getPrice() or getListPrice() based on the field name.
2) Make the method use getPrice() if $field_name is "price", then use hasField and get for others.
Solution depends on whether we want to require any purchasable entity to have a list price. Our initial thinking was that we didn't want to impose that requirement since list prices are display only.
Ultimately, the real problem is that we have no test coverage for a custom entity type implementing PurchasableEntityInterface. We need to add it, otherwise this situation will happen again.
Comment #7
bojanz commentedSo, the most minimal fix is this.
We still need to decide whether to introduce PurchasableEntityInterface::getListPrice() though. Thoughts?
Comment #8
rlmumfordAlternatively, we could add a new interface "ContextAwarePurchasableEntityInterface" that defines some method of getting different prices in different contexts. Depending on fields seems to be a bad idea as we can't enforce those with the interface.
This new interface could define getContextualPrice that takes Context as an argument.
DefaultPriceResolver would check whether the Entity implemented the new interface; if it did pass the context through to getContextualPrice, if not just call getPrice.
Comment #9
rlmumfordThis patch adds a new interface "VariablePricePurchasableEntityInterface" that defines the method "getContextualPrice(Context $context)".
DefaultPriceResolver calls the contextual price method if the new interface is implemented.
ProductVariationInterface now extends VariablePricePurchasableEntityInterface instead of PurchasableEntityInterface
The behaviour of PurchasableEntityInterface is entirely unchanged.
I'm not convinced about the names I've used for these interfaces and methods but I think the approach is workable.
Comment #10
rlmumfordI missed some changes in that last patch so I've hidden it to prevent confusion.
Comment #13
bojanz commentedThere is no use case for introducing such an interface, cause we have nowhere in Commerce to consume it from.
The current context field_name is set in a field formatter, if you don't have a field you don't have a field formatter either.
This brings us back to #7, and the question of whether it is desirable to also have getListPrice() on the PurchasedEntityInterface.
Comment #14
rlmumfordI don't understand what the original use case was for splitting them out like that, so I can't comment on the field formatter point. Why does one price field need to be able to display a different field? (I might have completely misunderstood that).
You're right that #7 is the most minimal fix and resolves the BC Break.
I would be against adding getListPrice to PurchasableEntityInterface because that would just be another BC Break. Also, once you have to idea of a ListPrice/Price distinction I think you open to door to all kinds of other prices.
Comment #15
rlmumfordI would be happy to mark #7 as RTBC as long as we're sure that 'field_name' isn't going to be passed as 'list_price' to an entity that doesn't have that field by mistake.
Should we always default back to getPrice if the list_price is NULL?
Comment #17
bojanz commentedI can confirm that.
Added a comment to explain why we're invoking the getter, and committed #7.