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.)

Comments

Neograph734 created an issue. See original summary.

neograph734’s picture

Title: Copy order item added/updated/deleted cart events to order? » Move some cart specific functions to order level instead?

EDIT: Merged into summary.

neograph734’s picture

Title: Move some cart specific functions to order level instead? » Bring OrderItemMatcher and order content alteration events to cart to order?
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new33.81 KB

Re-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 :)

neograph734’s picture

Title: Bring OrderItemMatcher and order content alteration events to cart to order? » Bring OrderItemMatcher and order content alteration events from cart to order?

Status: Needs review » Needs work
neograph734’s picture

Status: Needs work » Needs review

Setting to Needs Review to get some feedback on the idea.

neograph734’s picture

StatusFileSize
new62.63 KB
new725 bytes

65 failing tests was a bit too much...

neograph734’s picture

StatusFileSize
new61.78 KB

Apparently I was testing commerce without patching it... :s
This should look better

neograph734’s picture

StatusFileSize
new60.56 KB
neograph734’s picture

StatusFileSize
new64.99 KB

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

neograph734’s picture

StatusFileSize
new64.87 KB

Instead of throwing an Exception, let's save the Order instead.

neograph734’s picture

These 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.

neograph734’s picture

The 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.

bojanz’s picture

Suggestion 1: Move the orderItemMatcher from Cart to Order to allow combining items on all order types. (Combine set to FALSE to preserving the default behavior of adding a new entry in Order::addItem().

Suggestion 2: 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. Additionally copy the Cart::updateItem() to Order to have a consistent behavior.

You 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.

neograph734’s picture

Title: Bring OrderItemMatcher and order content alteration events from cart to order? » Introduce order content alteration events in commerce_order?
Issue summary: View changes

Thanks 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.

neograph734’s picture

StatusFileSize
new8.89 KB
new14.07 KB

Here 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.

neograph734’s picture

Title: Introduce order content alteration events in commerce_order? » Allow handling order changes on order level instead of in the cart
Category: Feature request » Bug report
Issue summary: View changes
Status: Needs review » Active

The more I think of it, the more I believe this is actually a bug instead a feature request.