onReturn method of StripePaymentElement should not apply "place" transition

This non-standard approach also has unexpected side effects like #3515600: Order balance is not updated when order placed.

I've also found out that because onReturn throws a redirect exception directly, instead of use CheckoutFlowInterface::redirectToStep() it bypasses CheckoutFlowBase::onStepChange() and any logic there.

Again, this non-standard approach has all kinds of side-effects. At the very least it would be good to document why it's done the way it is, so that anyone tinkering with it understands the consequences of changing it.

@aaronbauman and @jonathanshaw,

Thanks for the report and comments on this. (And the other 2 items mentioned.)

Just got back from DrupalCon Atlanta, but will look to get some of this resolved soon.

Note, though, that there are reasons for the implementation, and I'll articulate them for your understanding and consideration. (Makes sense to get it into the documentation and/or a change record.)

Would also appreciate understanding your scenarios, as well, so that the best end result can be achieved.

@aaronbauman, you mention that the order is already marked complete at this point in your site. Given that this is done immediately after the payment is created, would it be reasonable to presume you are doing so when the payment is created? Can you share thought on this?

I've merged a small commit that ensures the transition is allowed before invoking it.

While certainly not complete, see [#3515814--16059987] for some of the details behind handling the Stripe Payment Element in this fashion.

Thanks for #3515814--16059987 #3515814: PaymentElement assumes next step should be 'complete', it's very helpful. From that:

It is possible that the customer completed the payment with Stripe, but the onReturn is NOT invoked. (This can happen when the customer's network connection drops, battery dies, etc...) In this case, the webhook necessarily handles creating the payment and placing the order.

This seems to be the won't fix answer to this present issue:
(1) the onReturn() methods needs to use the same decison making logic about the order status in response to a successful stripe payment that the webhook uses when it handles a successful payment where the customer has dissappeared from checkout after Stripe takes the payment. It'd be confusing if an order was sometimes placed and sometimes not after payment.
(2) Most sites want the order to be placed after payment is received. It's the 80% use case. Sites that want different can customise the payment gateway's placeOrder() method which is used by both the webhook and onReturn and exists as an seperate method for the very purpose of facilitating customisation of this order handling.

What I'm not clear about is the matter raised in the IS:

According to the interface \Drupal\commerce_payment\Plugin\Commerce\PaymentGateway\OffsitePaymentGatewayInterface:

*OnReturn: Processes the "return" request.
*
* This method should only be concerned with creating/completing payments,
* the parent order does not need to be touched. The order state is updated
* automatically when the order is paid in full, or manually by the
* merchant (via the admin UI).

*

"The order state is updated automatically when the order is paid in full". This appears to refer to the work of
\Drupal\commerce_payment\EventSubscriber\OrderPaidSubscriber::onPaid() which does indeed do exactly that, for any order.

So I think it might be sufficient for the placeOrder() method to simply save the order, triggering OrderPaidSubscriber::onPaid().

Dispatching CheckoutEvents::COMPLETION) from placeOrder() is a bit of a weird one. Is the checkout really complete if the customer abandons it after payment? There's an argument it should only be called from the webhook, not from onReturn (because in onReturn the customer hasn't actually completed checkout necessarily); but there's also an argument for consistency between the two situations. I can see it both ways.

@jonathanshaw in makes sense, if we are supporting the next step not being "complete", that we defer the checkout completion event and the place status transition to the implementer's implementation.

We do need this to be consistent, though, between the webhook and the checkout flow. It is necessary to invoke the checkout completion event, as that typically is what sends the order email. If the customer completed payment, but was not able to return to the site, or made it through the onReturn, but lost the network connection before getting to the complete page, we need to fire those events so that they can still get the order email, among other things that occur from the place transition and/or checkout completion event.

Hopefully, this handles the most common cases, and, as you inferred, the placeOrder method can be overridden, if needed, for custom implementations. (Though I'm now considering we should rename that method, given that it isn't actually placing the order now if the workflow has steps between "payment" and "complete".)