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.

Comments

John Pitcairn created an issue. See original summary.

johnpitcairn’s picture

Status: Active » Needs review
StatusFileSize
new3.59 KB

Here's a stab at it. This patch adds a new event, CartEvents::CART_ORDER_ITEM_ADD as described above. Works for me locally when adding an order item with no purchaseable entity, let's see what testing says.

johnpitcairn’s picture

Note: 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.

jsacksick’s picture

I'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.

johnpitcairn’s picture

Firing 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 or NULL, and subscribers that have been using the getEntity() method in the old event would now need to use getPurchased Entity() and handle NULL. I think continuing to call the method getEntity() would be ambiguous DX, because an order item is also an entity.

What is the overhead of dispatching an additional event?

johnpitcairn’s picture

Assigned: johnpitcairn » Unassigned

jsacksick’s picture

Status: Needs review » Fixed

Committed!

johnpitcairn’s picture

Excellent, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.