The CartEvents::CART_ENTITY_ADD event is only dispatched if the added order item references a purchaseable entity.
Why is this a problem?
In the case of a donation (for example), there is no purchaseable entity, only an order item with a price that is set on the fly, so no cart add event is dispatched. I can of course run whatever cart logic I need when I add the donation order item to the cart, but this does not allow me to react and run the same logic when some other module adds a similar order item.
Proposed solution
Add a new event, CartEvents::CART_ORDER_ITEM_ADD. Dispatch this whenever any order item is added to the cart.
Note we already have CartEvents::CART_ORDER_ITEM_UPDATE and CartEvents::CART_ORDER_ITEM_REMOVE events. It seems an odd decision to have limited the cart add event to purchaseable entities only.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | commerce-order-item-add-event-3189448-2.patch | 3.59 KB | johnpitcairn |
Comments
Comment #2
johnpitcairn commentedHere's a stab at it. This patch adds a new event,
CartEvents::CART_ORDER_ITEM_ADDas described above. Works for me locally when adding an order item with no purchaseable entity, let's see what testing says.Comment #3
johnpitcairn commentedNote: I thought it better to add a new event rather than messing with
CartEvents::CART_ENTITY_ADD, because calling the$event->getEntity()method from that guarantees a purchasable entity is returned, right? Altering that will break existing implementations.Comment #4
jsacksick commentedI'm not against the idea, the only problem is that there is suddenly 2 events that are fired now, wondering if we can deprecate the older one (or fire that only only for non purchasable entities though that'd mean having to write 2 subscribers instead of one when needing to react to any order item being added.
Comment #5
johnpitcairn commentedFiring just one or the other would be easier I guess, and an event subscriber can listen to both events, but any implementation that wants to respond to all cart add events will need to understand and handle the distinction itself. The DX of that is not ideal (but it sure isn't ideal now either).
Deprecating the older one would mean we still need to fire both anyway until the old one is removed, the new one would need to provide a
getPurchasedEntity()method that returns either a purchaseable entity orNULL, and subscribers that have been using thegetEntity()method in the old event would now need to usegetPurchased Entity()and handleNULL. I think continuing to call the methodgetEntity()would be ambiguous DX, because an order item is also an entity.What is the overhead of dispatching an additional event?
Comment #6
johnpitcairn commentedComment #8
jsacksick commentedCommitted!
Comment #9
johnpitcairn commentedExcellent, thanks!