Currently commerce assumes that an order can only be changed while in the cart. An example of this is that
the checkout step is only reset from the CartManager::resetCheckoutStep(), but also all events emitted related to changes to carts content. Any change on the order content from the order entity does nothing. (eg. $order->addItem()
This does not really make sense because a cart is not even a special entity. It is only an order with a boolean value. For consistency (and contrib) I strongly believe it would make more sense to handle all these events in the order entity. Besides, you can install commerce_order without installing commerce_cart. In this situation, you have no way to know what happens in an order.
Proposed resolution
- Move (and deprecate) all cart events to commerce_order and emit them from there; optionally with a cart parameter.
- Rework all cart events and change them into order events. If action is needed only for a cart; check $event->isCart() or $event->getOrder->get('cart')->value.
------------------------------
Original issue
There are no events for altering [adding/updating/deleting] an order's contents (order items) for alterations that happen outside of a cart. For some event handlers it makes sense to subscribe to events where an order item is added/updated/deleted on any order instead of only for cart orders. This became evident in #3083384: Checkout flows should be resolved again after the cart/order has changed.
Proposal: Have Order::addItem(), Order::removeItem() and Order::setItems() also emit events to let other modules know that the order's items have changed. These should be similar to the events emitted by CartManager.
Optional: Copy the Cart::updateItem() to Order to have a consistent behavior and a central place for emitting events. (It can be called from the cart manager.)
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3086876-16-order_content_events_with_update_event.patch | 14.07 KB | neograph734 |
| #16 | 3086876-16-order_content_events.patch | 8.89 KB | neograph734 |
Comments
Comment #2
neograph734EDIT: Merged into summary.
Comment #3
neograph734Re-titling again and attaching a first patch. I did not focus on any tests and would like to get some feedback on the proposal before spending time on tests :)
Comment #4
neograph734Comment #6
neograph734Setting to Needs Review to get some feedback on the idea.
Comment #7
neograph73465 failing tests was a bit too much...
Comment #8
neograph734Apparently I was testing commerce without patching it... :s
This should look better
Comment #9
neograph734Comment #10
neograph734I think I have solved all of the above errors; new tests have to be written for all new events and the Order::updateItem() method and I have merged the patch from #3088971: OrderItemMatcher getCustomFields logic seems odd in as well.
Comment #11
neograph734Instead of throwing an Exception, let's save the Order instead.
Comment #12
neograph734These 2 tests seem to have uncovered an issue where the inclusive tax is not shown on the product. #3089043: Inclusive tax display not calculated on product display.
Nope, in hindsight it seems that this patch actually broke the price resolver.
Comment #13
neograph734The price matcher is using disposable orders, so saving them as soon as order items were added was not really useful. Since in Order::postSave() the back-reference is set anyway, it does not really matter if the order_id is set once the order item is added.
Comment #14
bojanz commentedYou are trying to do two very different things in one huge (and probably impossible to commit) issue.
I suggest opening a separate issue for suggestion #1, and keeping suggestion #2 here.
Keep in mind that the issue for suggestion #1 needs a clearly defined use case, cause we don't currently have a place in the admin pages where we'd use an OrderItemMatcher.
Comment #15
neograph734Thanks for the feedback bojanz. The issue indeed became bigger and bigger. I have created #3089740: Move OrderItemMatcher from cart to order module and narrowed this one down.
Comment #16
neograph734Here are 2 patches. The first is for the add/remove/set events. The second patch combines those with an update event as described in the issue summary.
I could not find any test for the existing events, so those are not included.
One thing to keep in mind is that the price resolver uses Order::setItems for resolving the display price as well, so that event is always thrown when viewing an order with a calculated price display.
Comment #17
neograph734The more I think of it, the more I believe this is actually a bug instead a feature request.