Separate stored payment methods from new in the Payment Checkout Pane

Attached is a patch implementing this and two screen recordings showing up the behavior when there is no stored payment methods and when there are some.

Updated patch.

Fixed failing test.

Added new test case covering switching between stored and new payment methods.

I'm not against the idea, the problem here is BC, that can potentially break any custom code/contrib altering the checkout form and the payment information pane in particular...
I recently committed code to Commerce PayPal that would stop functioning once this goes in for example...

Additionally "Stored payment methods" is probably too technical (Perhaps it should be "Existing payment methods"? A non technical customer might think we're saving credit card numbers.

One thing we could do to mitigate the impact of this change would be to introduce a setting at the checkout pane level that toggles whether the form is built the "legacy" way vs this "new" way although that'll make it harder to alter since you'd have to handle 2 very different usecases.

Chiming in: why do we need a new method? Can't the existing payment options be discerned if new or not and split into different radios that way?

@mglaman, can you elaborate a little more?

To add some background: I started this patch off from a patch from @dwkitchen: #3170412: Allow a store to always collect a billing address before payment method selection. Perhaps I got a little biased on how he was approaching splitting the payment radios.

@dwkitchen: I don't think there's a need to revert the PayPal code, because it'll still work for older versions of Commerce and nothing will happen once this gets committed.

I agree with the comment #10 from @mglaman and prefered to keep it simple and skip introducing new public methods / APIS.

I've been working on this today and managed to simplify the code thanks to a "trick".
By specifying the #parents attribute to be the same for both the "stored_payment_method" and "new_payment_method" elements, the radio options share the same name attribute.

Thanks to this trick, we don't have to make changes to the logic that figures out what the default selected option is... And we don't have to update both the clearValues() and submitPaneForm() methods which were way less readable with these changes.

Additionally, the tests seem to work as is.
One caveat though with that approach is that I had to disable Drupal's default validation that ensures the selected option is valid and is part of the #options array (I did replace this by logic in validatePaneForm() that's responsible of that).

That said, even though I'd really love to get this in, I don't think we can safely commit this to the 2.x branch as it'll affect existing sites, I don't know whether we can consider this a BC break, but the form structure has changed, however if CSS was targeting the form element using the name attribute, this should still work thanks to the trick.

Thoughts?

I like #15, it's clean and easy to grok. I agree this probably needs to go into 3.x since it changes the pane form (kills $pane_form['payment_method'].) Otherwise it's a hot mess of backward compatibility support.

+1 from me on the last patch and pushing to 3.x

Committed this to 3.0.x.

Now that the 3.0.0 release is approaching, I'm having second thoughts about this change. The "Add payment method" label doesn't make sense in the case there is no payment gateway implementing the SupportsCreatingPaymentMethodsInterface.

Re #21 I think it's SupportsStoredPaymentMethodsInterface. that is a problem not SupportsCreatingPaymentMethodsInterface

Offsite gateways may implement the former without implementing the latter. Onsite gateways always implement SupportsCreatingPaymentMethodsInterface therefore it also implement SupportsStoredPaymentMethodsInterface

I reverted this today... So marking this as needs work considering #3470316: Payment Information fieldset label is misleading if mix of gateways supporting and not supporting stored payment methods is available and other inconsistencies found.
After discussing this further internally, I now believe we should attempt to provide a better alternate UX via an alternate pane or something... Or a behavior you can toggle within the existing pane via settings. This would be a less disruptive change and would allow people to optin/optout of this change....

Rather than opt-in or -out as a part of pane plugin definition I think it would be beneficial for the codebase upgrading efforts (strong typing, etc) if we could provide an alternative plugin class, compatible in behavior (ie. pane form submission result) with the current one. The new plugin class could be swapped via hook_commerce_checkout_pane_info_alter. In that scenario, the site-wide checkout.settings config could control which class to use for the Payment Information checkout pane plugin. The old class would throw deprecation error as mentioned in https://www.drupal.org/about/core/policies/core-change-policies/how-to-d.... The old class would be the default for existing sites, and the class with a modernized UI used by default on new Commerce installations. During Commerce 3.x lifecycle one can change the plugin class back and forth and old, legacy class as well as the option to change it will be removed in the 4.x