Displaying PaymentElement at Review step might not be the way to go

Hey Nicolas, Wasn't this answered in ✨ Why does PaymentInformation->submitPaneForm handles things differently between Card Element and Payment Element? Active point 3 and in your comment ✨ Why does PaymentInformation->submitPaneForm handles things differently between Card Element and Payment Element? Active ?

Hi Andy,

Actually in the issue you mention, the question was : "Why is the Payment Element form now displayed at Review step instead of "Order Information" step (I believe we mistakenly called this step "Payment Information" in our discussion but the step before Review is actually Order Information. The "pane" on the other hand is indeed called PaymentInformation and its "default_step" is indeed "order_information").
Why not use SetupIntent to first create the Payment Method during Order Information step, and then create the PaymentIntent at Payment step like what was done with the basic Stripe Card implementation? Why create the Payment Method only after the Stripe payment has been done?

TomTech provided us with the info that "If a setup intent is required in the checkout flow with this change, then it would break support for Klarna, Affirm, WeChat Pay, AliPay, and any other payment method that does not support future usage". So we now knew why displaying Payment Element form at Order Information step was not a good idea and why Review was chosen instead.

Now, the question I am raising here is: why do we display the Payment Element form at Review step instead of Payment step (the step after Review). Because according to this answer of jsacksick, it seems we should have done so:

even for an offsite payment gateway, this code (PaymentProcess::createPayment) should be invoked prior to redirecting to the gateway (Stripe) meaning no payment should have occurred yet. Also, it is the job of the payment gateway (Commerce Stripe) to register the payment with the right amount from onReturn()

The problem I raised is that PaymentProcess::createPayment creates the initial commerce_payment object in state new with its amount being the order balance. This code is expected to be executed prior to redirecting the customer to Stripe.

In the current implementation of Stripe Payment Element, the redirection to Stripe is done at Review step and PaymentProcess::createPayment is only called after the customer has returned, creating the commerce_payment with its amount being the order balance instead of the amount actually paid on Stripe AND we never double check that the amount of the commerce_payment is indeed the amount paid on Stripe. Since the customer can be away for some time because of 3D Secure validation, Paypal validation etc, the order balance might have changed by then. I actually faced this in production because of expired promo.

The fact that the order balance might have changed by then is actually due to the fact that because we are making the payment at Review step, the order is not actually locked yet! Which is really bad actually (see 🐛 Lock the order before stripe.confirmPayment() is called Active )
Indeed, CheckoutFlowBase::onStepChange is locking the order only at 'payment' step. Not review. This should be fixed ASAP even if we have to temporary lock the order at review step.

Actually even though locking the order is critical, it will not prevent the order balance to change (at least for now), because of a bug in Commerce core I have reported here:
🐛 Make sure the price of a locked order is never updated especially when a promotion expires. Active

The more I think about it, the more I think we should indeed put the Payment Element form at the Payment step as stated by jsacksick.

What does this imply?

Well, right now, because the Stripe payment is done before the Payment step, when we arrive inside PaymentProcess::buildPaneForm because the payment_method has already been created and attached to the order, we go inside the first if statement:

if ($payment_gateway_plugin instanceof SupportsStoredPaymentMethodsInterface && !$this->order->get('payment_method')->isEmpty()) {
  $payment->payment_method = $this->order->get('payment_method')->entity;
  $payment_gateway_plugin->createPayment($payment, $this->configuration['capture']);
  $this->checkoutFlow->redirectToStep($next_step_id);
}

which calls StripePaymentElement::createPayment so that is updates the commerce_payment from new to completed.

Calling StripePaymentElement::createPayment before the Stripe payment has been made would be problematic because the code actually expects the order to have a payment_method already and also, because the order would not have a stripe_intent yet, the code would try to confirm the PaymentIntent as an off session payment.

Fortunately, if we put Payment Element at Payment step, the order won't have a payment_method yet which means we will skip the first if and go to the next one, the one we are actually supposed to use I guess:

elseif ($payment_gateway_plugin instanceof OffsitePaymentGatewayInterface) {

It seems this is where we should display the offsite payment form: Payment Element form.

I guess StripePaymentElement::createPayment should not be called by PaymentProcess::buildPaneForm
but by StripePaymentElement::onReturn after creating and storing the payment_method.