Describe your bug or feature request.

Orders are locked when a user moves to an offsite gateway. Locked orders are not automatically refreshed on load (#3047357). However, locked orders are by default refreshed on save, so long as they are in draft status. If the PaymentOrderUpdater runs before the order is placed (e.g. in a webhook or queue) the order will still be in draft status and therefore refreshed. This can cause multiple issues.

Due to the process running in a webhook/queue, the user session will not be available.

However, there are two sub-cases to consider, in evaluating the impact:

1. The payment gateway manually runs the PaymentOrderUpdater.
Some payment gateways manually run the PaymentOrderUpdater (e.g. commerce_datatrans) to avoid race conditions caused by running the PaymentOrderUpdater in the kernel destructor.

In this case, the PaymentOrderUpdater saves the order, the order refreshes, the commerce condition for any user based* promotion is no longer valid (as the user session is not available). This results in said promotion being removed. The order will then not be marked as paid and will not be placed by the OrderPaidSubscriber (it may still be placed if the user returns from the offsite payment gateway after the webhook runs, by the CheckoutFlowBase).

2. The PaymentOrderUpdater runs in the kernel destructor
In this case, the PaymentOrderUpdate runs in the kernel destructor and saves the order. The order is still draft. This means that a user based condition (or any order processor for that matter that needs a session), will attempt to start a session. Sessions cannot be started in kernel destruct as headers have already been sent. This results in the storage rolling back due to the fatal exception. Here any changes made to the order on the pre-save are lost. Even if the order is not refreshed on save, the save can cause a fatal exception if any event subscriber / hook does anything which attempts to start a session.

*User (rather than order customer) based conditions are not offered out of the box, but are used to allow back-end processing of orders and exist on some promotions.

Actions required

Proposed action 1:
Update documentation to advise payment gateway providers to manually run the PaymentOrderUpdater to avoid it running in the kernel destruct. This avoids race conditions and fatal exceptions that result in the order being saved.

Proposed action 2:
Option A - When saving the order in the PaymentOrderUpdate, mark it to skip the refresh.
If the order is not refreshed in such cases, then it is possible that pricing (or some other input) changes while the user is away at the offsite gateway. However, this eventuality can potentially be neglected.

In the PaymentOrderUpdater::UpdateOrders():

 if (!$previous_total->equals($new_total)) {
      $order->setTotalPaid($new_total);
      if ($save_order) {
        // The order is set not to refresh as a refresh in the webhook/queue 
        // can have deleterious effects due to the absence of the user session .
        $order->setRefreshState(OrderInterface::REFRESH_SKIP);
        $order->save();
      }
    }

Option B - Don't refresh any locked orders. However, this will stop the order from refreshing on the last save before the user is redirected the payment gateway. This may be BC breaking depending on the checkout flow. Therefore, I would probably go with option A, if i were to choose.

In Order::preSave():

if ($this->getState()->getId() == 'draft' && !$this->isLocked()) {
      // Refresh draft unlocked orders on every save.
      if (empty($this->getRefreshState())) {
        $this->setRefreshState(self::REFRESH_ON_SAVE);
      }
      // Initialize the flag for OrderStorage::doOrderPreSave().
      if ($this->getData('paid_event_dispatched') === NULL) {
        $this->setData('paid_event_dispatched', FALSE);
      }
    }

If a bug, provide steps to reproduce it from a clean install.

Install commerce, commerce_promotion
Create a commerce condition that uses the current user.
Setup a promotion that uses said condition.
Use a payment gateway that saves a payment in a webhook before the order is placed, so that the PaymentOrderUpdater runs (either manually in the gateway or in the kernel destruct).
Observer impact (nature of which is dependent on whether the save occurs in the kernel destructor)

Issue fork commerce-3323736

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tcrawford created an issue. See original summary.

tcrawford’s picture

Issue summary: View changes
tcrawford’s picture

Issue summary: View changes
tcrawford’s picture

Issue summary: View changes
tcrawford’s picture

Issue summary: View changes
tcrawford’s picture

Issue summary: View changes

  • jsacksick committed 675d970 on 8.x-2.x authored by tcrawford
    Issue #3323736: PaymentOrderUpdator should run before kernel destructor...
jsacksick’s picture

Status: Active » Fixed

I agree with the fact that the order shouldn't be refreshed at this point.

  • jsacksick committed 9da4465 on 3.0.x authored by tcrawford
    Issue #3323736: PaymentOrderUpdator should run before kernel destructor...

Status: Fixed » Closed (fixed)

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