In StripePaymentElement->onReturn do not allow PaymentGatewayException to bubble up if we know the payment was successful on Stripe's end

@Nicolas Bouteille

Is there a way to reproduce this issue? We've run into this a few times as well but never had steps on how to reproduce it and add a fix.

Hi Vitaliy,
I think this comment of yours is the situation you were thinking about:
https://www.drupal.org/project/commerce_stripe/issues/3392413#comment-15... ✨ Add the option to not setup payment methods for future usage Fixed

However, I believe that the problem I am raising here should not just be fixed because there is a bug right now somewhere. I don't think you need to waste time verifying if you find a way to produce a PaymentGatewayException there or not. The fact that it actually can or could happen is enough IMHO. Right now it can happen because in doCreatePaymentMethod we call $stripe_payment_method->attach(['customer' => $customer_id]); and because doCreatePaymentMethod transfers ApiErrorExceptions to ErrorHelper::handleException($e);
But even if this code was not here today, it could be tomorrow...
The protection layer I am suggesting we add should be added not matter what IMHO in case a PaymentGatewayException is someday thrown after we know the payment was successful on Stripe's end.
So if tomorrow someone adds some code that generates a PaymentGatewayException after the payment was successful, we'll still be able to let the customer know that something went wrong but they should not try again right now because the payment might actually have passed successfully...

Hi,
We now have Payment Element live in production! Yeah!
Today we got an error when StripePaymentElement->doCreatePaymentMethod() tries to send the billing details to the payment method
PaymentMethod::update($stripe_payment_method_id, ['billing_details' => $payment_method_data]);
the request generated this error:

card_declined
The card returned a decline code of invalid_account.

The card, or account the card is connected to, is invalid.

The customer may need to contact their card issuer to check that the card is working correctly.

To learn more about why some payments fail and what you can do to decrease your decline rate visit our guide on understanding declines and failed payments: https://stripe.com/docs/declines

Because of this error that was converted to a DeclineException by ErrorHelper->handleException, the order could not be placed.
Just because we could not attach the billing details to the payment method although the payment was successful...

Thanks to my custom try catch, the customer did not receive the error message from PaymentCheckoutController->returnPage
"Payment failed at the payment server. Please review your information and try again."
Instead, she saw a message letting her know that an error had occurred after the payment might have been successful so she should wait for us to contact her. Which we did.

Now I do believe failing to attach the billing details to the payment method after a successful payment should not prevent the correct finalization of the order afterwards. So I am keeping my try catch, but now I don't even re-throw a generic exception anymore, I only log the error with full stack trace so that I am alerted and I can resolve the situation, but I don't stop the order to be place no more and don't show an error message to the customer just because the billing address could not be attached to the PM.

I think any error during the whole "saving the payment method locally" step should not block the finalization of the order.
With Card Element, we did not have such a situation because the PM was always created before the payment was confirmed... I think we should think about this carefully for Payment Element that creates PM after successful payment only...

What do you think?

I think on our website I am actually going to completely remove the code that tries to attach the billing address to the payment method after a successful payment. Now that I know that this can actually cause an error inside doCreatePaymentMethod() which will prevent the payment method creation inside createPaymentMethod(), I think I'd better not even try to attach billing details to the PM at all.
After all, the client could have used a credit card which is not under her name... so trying to attach her address to a credit card under someone else's name could cause an error. Also at this step, the payment is already successful and the payment method already configured for off_session future payments. So what good could bring us to attach the address to the PM? I don't see any positive side, only a potential risk of error.

What do you think?

I'm coming from 💬 Support more payment method types Closed: duplicate . Our client reported a problem in his webshop. A customer paid successfully with PayPal, but got an error message on Drupal side, because PayPal is currently not supported by this module and a PaymentGatewayException is raised with this message "The selected stripe payment method type(stripe_paypal) is not currently supported"

We were not aware that not every activated payment method in Stripe is allowed on Drupal side - and now I'm in a hurry to get a quick workaround. We don't care about re-usable payments here, so having a payment method entity is not important for us. So I'm just thinking about patching the onReturn() function to early exit instead of throwing that exception. --> would that probably lead to other problems inside commerce_stripe module? Because if not, I'll do that as a quick fix. Any feedback is very welcome :) (also on the commerce slack channel). I'm of course open to help implementing outstanding payment providers

Yes indeed, the job of onReturn right now is only to store the Commerce Payment Method.
But even if you skip that part, you still need to let PaymentCheckoutController->ReturnPage redirect to PaymentProcess step so that the Commerce Payment is created and the order completed.

$payment_gateway_plugin->onReturn($order, $request);
$redirect_step_id = $checkout_flow_plugin->getNextStepId($step_id);

As a quick workaround, I would personnally disable any payment type that you have not tested yet. You can do that by replacing in StripePaymentElement.php

'automatic_payment_methods' => [
  'enabled' => TRUE,
],

with something like:
'payment_method_types' => ['card', 'paypal'],

Then you can take the time to test thoroughly any payment type you want to add. Be carefull, some payment types such as bank wire for example could be different than the others. I think they might have a "processing" status at first and will only have a "succeeded" status later on when the bank transfert has been received… so you should definitely take the time to test any payment type before you enable them.

PaymentElement is a great tool to ease the possibility to use multiple payment types, but I don't think it's plug and play. We need to be careful to a lot a details.

thanks for your input!

I think, the best would be to implement the missing plugins. That shouldn't be that tough imho. I'm of course willing to help.

I just want to coordinate to avoid working in the wrong direction. So basically, here are the plugins we'll want to use:

• PayPal
• EPS (Austrian bank redirect)
• Sofort (bank redirect in AT, DE, CH)
• Google Pay
• Link

The last two are optional for us. The most important will be PayPal.

We should coordinate, which extra fields we want/need, any special treatments (like status processing) that are needed, and so on. I think that there could be either a generic plugin for bank redirects or at least an abstract base class to hold common logic together (rather this I think). Any important thoughts, links to important parts of the Stripe API docs, etc would be very helpful.

As next step, I'd open issues for any plugin that I can help to create. But let's discuss the general approach either here or in Slack first

Well, I know this work has already started. Some might already be available on the dev branch you should go check. I think there are other open issues dealing with this more generic need. This one's goal is only to not throw a payment error once we know the payment was successful.

I'm already on latest dev/rc. You're right that a necessary refactoring work has already been committed, which paves the way to implement other payment method types, but nevertheless they need to be implemented like 📌 Add ACH support to Payment Element Gateway Fixed recently.

Ok, you're right. I'll open up issues per payment method type

FYI: Google Pay and Apple Pay should work (we have already integrated these payment methods), we tested it on several LIVE projects and everything is fine.

I haven't tried them yet, but I'm wondering.. which version do you use? 1.1? because in the meantime (dev, 1.2-rc1) this commit may have broken these payment methods: https://git.drupalcode.org/project/commerce_stripe/-/commit/bf118321d3fb...

@agoradesign
This version contains the latest fixes/features https://www.drupal.org/project/commerce_stripe/releases/8.x-1.2-rc1 →

I know. and as I have written, the mentioned commit seems to have the drawback that currently only credit cards work, others need their own plugins now