Support multiple payment gateways on Checkout Payment information pane

Not a code review but a format nitpick: you'll need to reroll this to fix indentation. It appears to use a combination of tabs / 4 spaces or else at least inconsistent indentation. Thanks!

indeed there are some issues about the previous patch.

i also i'm not sure about using the payment method types as select options.

Here is a new patch which uses the payment gateways as options for Payments select.
I'm not sure if a payment gateway has a multiple payment method types.
maybe i didn't get (yet) the payment system.

it seems the patch needs re-roll

there is a new patch for the latest dev.

it seems the patch does not work with Off-site payments
"add-payment-method" is missing for this kind of payments.

i'm not sure if there's something else needed for "Payment Information" before this selection for multiple payment gateways.

any suggestions are welcome

So, if I understand this correctly, this patch will make it possible to have multiple payment gateways enabled on a site? I did find a couple of things that I feel can be improved.

I feel very strongly about the substr solution. That's feels like it can go wrong in a ton of unpleasant ways. I think adding a new variable (something like: $first_time_order, or $no_default_payment_selected, or $create_payment_method) would make this more readable and less fragile.

1. +++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php
   @@ -163,8 +166,8 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
   +   * @param array $payment_gateways
   

   Even though this is now an array, it's an array of PaymentGatewayInterfaces, so we can still still document that here.

   @param \Drupal\commerce_payment\Entity\PaymentGatewayInterface[] $payment_gateway
   

2. +++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php
   @@ -173,31 +176,34 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
   +      /** @var \Drupal\commerce_payment\Entity\PaymentGatewayInterface $payment_gateway */
   

   Fixing of the @param should remove the need to add this @var comment. (PhpStorm understands this)

3. +++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php
   @@ -173,31 +176,34 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
   +      $default_option = $values['payment_method'];
   ...
   +      $default_option = 'new_' . $default_payment_gateway->id();
   
   @@ -209,18 +215,24 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
   +    if (substr($default_option, 0, 4) == 'new_') {
   

   So, if I create a gateway that starts with new_, it will always trigger here? That sounds needslessly fragile. It's probably easier to have an additional variable?

4. +++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php
   @@ -209,18 +215,24 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
   +      $payment_gateway = PaymentGateway::load(substr($default_option, 4));
   

   Yeah no, that substring here feels very fragile.

After digging trough the original code, I see that the new_ is nothing new, so you can definitly ignore my remarks about that in #6.

It is definitely a hack though. My idea has been to rewrite this pane to use individual radios that have special keys on them ('#payment_gateway' and such)

The patch in #4 no long applied to HEAD. Reroll here.

I don't understand how #4 actually supports multiple gateways. It doesn't crash but that's not helping either. is this a good direction? Not sure.

This fixes .1 and .2 of my own remarks in #6.

+++ b/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php
@@ -170,31 +173,33 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
+      $id = 'new_' . $payment_gateway->id();
+      $options[$id] = $payment_gateway->label();
...
+      $default_payment_gateway = reset($payment_gateways);
+      $default_option = 'new_' . $default_payment_gateway->id();

@@ -206,18 +211,24 @@ class PaymentInformation extends CheckoutPaneBase implements ContainerFactoryPlu
+    if (substr($default_option, 0, 4) == 'new_') {
+      $payment_gateway = PaymentGateway::load(substr($default_option, 4));

kinda black magic

Here is a re-work of the previous patches.
What was wrong about them: Actually they didn't implement a multiple gateways solution, but they tried to include all the gateways and their payment methods in the same place.

The new patch/PR:
Build 2 elements
1. Payment gateway selector
2. Payment method form which includes the methods selector and the Add payment method form.

On this, we'll have 2 ajaxs for each selector.
Tested also with an Offsite Payment (PayPall Express Checkout).

Known issues:
- Sometimes changing the gateway there is no Payment method selected (in the radios options)
- Payment gateways with js libraries (ex. Paymill/Stripe) on "PaymentMethodAddForm" won't work. It seems the js are not loaded properly.
Not sure if this core issue could be related : #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs.

Questions/Discussions:
- JS issues with ajax : Maybe we need build something to get the js libraries in the payment information pane.
- What about single Payment gateway / single Payment method - How
- Option to save payment method for the customer - as Save (this) Cardonfile for the new payment methods.
- Customization of the pane, including those selectors (API).

Patch available and also interdiff
PR : https://github.com/drupalcommerce/commerce/pull/638

p.s. imho, if this patch/pr looks ok to you, we should added to module, so we could get more feedback for more people on this.

@vasike, the patch applies good and works fine as far as i tried.

One thing observed is that the radio option which is selected jumps to top after ajax automatically.
Is this expected functionalilty??

@rgpublic : i agree that the UX is far from good.
for ex. Even the labels of the selectors doesn't sound good to me.

But maybe this could be a future issue.
Maybe this could be related with the Payment gateways implementations and not only about Payment information pane.

some thought to improve the UX:
- Better labeling
- Use payment icons and extra info for gateways.
- Nested selectors : Payment method selector and or new inside the Gateway selection - similar as Accordions for gateways.
- Better UX for payment method : Use previous payment / Save payment / New payment instead of simple selector.

p.s. i still believe we should focus to have the multiple payment gateways and then more people could contribute to improve the UX.

@vasike i found that the patch you have put up doesn't apply on new head because of new commit. Re-rolled a new patch against latest head.
Attaching it here.

Edit: can you raise a pull request at commerce repo on Github??

Also, i found that the previous PR failed due to small issues. They can be found below:

https://travis-ci.org/drupalcommerce/commerce/builds/202616331

@sumanthkumarc : thanks a lot for the support

@rgpublic : it seems you're right about single selector.

Talked with Bojan and he confirmed that there should be one selector.
He also suggested the commerce_shipping ShippingRateWidget approach.

So i re-work the solution into a new fresh patch
- It builds a single selector which also "transport" the payment gateway / payment method info.
- remove attachPaymentMethodForm as it doesn't make sense anymore
- Tested also with an Offsite Payment (PayPall Express Checkout)

Known issues:
- (Still) Payment gateways with js libraries (ex. Paymill/Stripe) on "PaymentMethodAddForm" won't work. It seems the js are not loaded properly. see comment #15 for more
- For now anonymous could see the existing payment methods. But for this, there should be a solution #2832493-10: Checkout shows payment methods which cannot be reused.

new PR for the patch #21
https://github.com/drupalcommerce/commerce/pull/640

@vasike the patch again needs reroll because of new commits on head

PR rerolled
https://github.com/drupalcommerce/commerce/pull/640

PR updates:

- Some clean-up suggested by travis
- Fixes for Payment checkout tests.

Now we're green

However, working on "Fixes for Payment checkout tests. "
- Who's should be default payment method, the saved one or the new add payment method?
- How to order the payment gateways and their payment methods?
- and maybe other things ...

I think we may consider also the default payment method for the selection.
Following #2790533: Add the ability to set a default payment method

Looks like this needs reroll already, had a quick look at doing that myself but not sure about the changes in PaymentInformation

Not really, as it was no conflict with the latest dev.
However i merged the latest '8.x-2.x' into the PR for future re-rolls

@Berdir : thanks

Yeah sorry, I was confused. I tried to use https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul... but that didn't apply. I just learned that .patch creates a patch-per-commit format that didn't apply while .diff works just fine.

Can confirm that this works pretty well, although I'll likely end up not using it after discussing my requirements with @bojanz, but that's OK.

One thing I noticed is that payment gateways that do not support payment methods (SupportsStoredPaymentMethodsInterface) still get a "Create new credit card: $Gateway". Looks like $this->configuration['payment_method_types'] defaults to credit_card even when that interface is not there and the UI doesn't show payment methods when configuring the gateway. Should probably return an empty list fur those and special case them somehow?

1. about JS
there is already an issue open #2854852: Payment gateway JS is not always loaded, i'll linked as related

2. about naming: "Create new credit card: $Gateway" or other names.
It was just something for work/test/review.
Having multiple gateway/integration and all of them add a "new credit card" option - hard to choose ;)
So waiting for suggestions how it should be.

3. About SupportsStoredPaymentMethodsInterface
The PR will use this only to get the saved payment methods, but allowing to have other gateways that not support + also for OffSite gateways (as Express Checkout from PayPal).

My turn.

Spent significant time improving the patch. Here's the current progress. To be wrapped up in the morning.

The payment gateway storage now respects the payment gateway weight.
The logic for building the options was significantly improved, and now properly supports gateways that don't implement SupportsStoredPaymentMethodsInterface (off-site, manual).

Opened a followup for berdir and others building custom checkouts: #2858599: Move a portion of the PaymentInformation pane logic to a PaymentOptionsBuilder.

The patch looks better and it works (except the js not loaded issue).

However it seems there is a switch order for some options (on change)
in my case
New credit card (Paymill)
New credit card (Example)

The selected one become the first. I think it starts with second change on them.

this was always (in all patches) there.

Payment method options for 2 Payment gateway definition of the same gateway type.

Ex.
New credit card (Example)
New credit card (Example)

For the Example (On-site) gateway type.

Is this ok?
Or we should use the Name we define for the gateways.

I noticed that too. If they're not yet (I think not) then we should probably add a weight to payment gateway configurations and use the draggable list builder to have explicit control over the other?

Can be done in a follow-up I think.

There is a weight already (although controlled globally, not per store or per checkout flow - perhaps neither of which is necessary out of the box).

switch order fix i found.

similar with ConfigEntityListBuilder approach:

  /**
   * {@inheritdoc}
   */
  public function loadMultipleForOrder(OrderInterface $order) {
    // @todo Invoke the attached conditions to determine eligibility.
    // @todo Fire event.
    $payment_gateways = $this->loadByProperties(['status' => TRUE]);
    uasort($payment_gateways, [$this->entityType->getClass(), 'sort']);

    return $payment_gateways;
  }

@berdir
We already have a weight and a draggable list builder. So the question is where it goes wrong :)

EDIT: Looks like the weight is not initialized to a numeric value on initial save. You need to explicitly save the drag & drop listing to get the right values.

Confirmed that #38 is the right approach, applied it to the patch.
Rewrote the tests for full coverage.
Added a tweak that hides the radios if there's only 1.

And committed. Thanks, everyone!