[META] Allow offsite payment gateways to create and use payment methods

I had a quick look at the interfaces last Friday, and have come up with this as a possible way forward?

Essentially, next to existing SupportsStoredPaymentMethodsInterface it adds new SupportsCreatingStoredPaymentMethodsInterface, moving the createPaymentMethod() method declaration there.

The idea is:
1) payment gateways that support creating new credit card tokens only during the checkout (together with order payment) would implement SupportsStoredPaymentMethodsInterface only,
2) additionally, gateways that support "stand-alone" cc token creation as well (outside of checkout) would also implement SupportsCreatingStoredPaymentMethodsInterface,
3) gateways not supporting tokenization at all wouldn't implement any of these interfaces.

This applies to both onsite and offsite gateways, as I imagine we could have an onsite gateway not supporting cc tokenization.

Mind that the llinked commit is just a very rough and incomplete draft.

Thoughts?

I think we should just keep SupportsStoredPaymentMethodsInterface and the payment gateway properly sets the payment method as not being reusable. Otherwise I would say a SupportsReusablePaymentMethodsInterface to identify the reusable flag can be TRUE.

With the linked commit I'd rather keep the OffSiteInterface lean, not extending anything with stored payments. The problem is it feels cumbersome to add a new interface just to make sure a flag is TRUE or FALSE. I'd rather leave this to developer documentation.

Bojan said he wanted to rewrite these stored interfaces - see our Slack from 09/01 - the linked commit is just my take on it.

Setting the payment method as being reusable or not is a different thing. Here in both cases - both for SupportsStoredPaymentMethodsInterface as well as for SupportsCreatingStoredPaymentMethodsInterface the payment method is always reusable. (Well, actually, there is also a 3rd case, when a payment method is not reusable, and then the gateway should not implement any of the "stored" interfaces.)

The issue here is separating the 2 use cases from Bojan's issue description - 1) payment gateways that support creating payment methods only during the checkout process and 2) gateways that support "stand-alone" creation of payment methods (from user's "Payment methods" page, without checkout). The 2nd case is what SupportsCreatingStoredPaymentMethodsInterface is supposed to be used for - though perhaps a different name might fit it better.

It's just that when a payment gateway doesn't support "stand-alone" payment method creation, the createPaymentMethod() method doesn't make any sense, and actually should not be called at all - that's why I have moved it to a separate interface, and did other subsequent changes.

Re keeping the OffSiteInterface lean - no problem with that, I will update the commit accordingly.

Work on this was done here: https://github.com/drupalcommerce/commerce/pull/613

Wouldn't it be ok for the "first" version to just store the payment address and name for a given method.

Let's say I pay with "bankcontact", a single use off-site payment method.
I enter my address and continue to the payment page.

The next time I get to the payment selection page, I select "bankcontact" again, and my address is pre-filled.

The rest of the process goes as usual.

I've been looking into proposed changes and issue summary (both made a long time ago and partially outdated) and I have some doubts in relation to PaymentMethodTypes.

1. It seems like PaymentMethodTypes (Credit Card or PayPal) are used only when payment gateway SupportsStoredPaymentMethod, which effectively limits it to the OnsitePaymentGateways at the moment.
2. In case PaymentGateway doesn't support stored payment method, billing address is used to build PaymentInformation pane.
3. Forms for CreditCard and Paypal are hardcoded in PaymentMethodAddForm rather than being the part of the PaymentMethodType plugin itself - that limits developeres who wants to provide custom PaymentMethodType. I'm sure we have an issue for that but can't find one.
4. If OffsitePaymentGateway were supposed to use the same interface as OnsitePaymentGateway - SupportsStoredPaymentMethod - the form displayed in PaymentInformation checkout pane will depend on the PaymentMethodType plugin assigned to the PaymentGateway - CreditCard by default.

All that makes me wonder:

1. Do we use PaymentMethodType plugins for anything regarding OffsitePaymentGateways?
2. Should we move forms specific for PaymentMethodType from PaymentMethodAddForm to PaymentMethodType plugins?
3. Should we even display this forms for OffsitePaymentGateways? We definitely don't want to show CC form for OffsitePaymentGateways that SupportsStoredPaymentMethod.
4. Can we even reuse SupportsStoredPaymentMethod for OffsitePaymentGateway given ::createPaymentMethod is triggered on PaymentMethodAddForm? That will create stored PaymentMethod for the user who hasn't completed the checkout/haven't been redirected to the offsite payment gateway.
5. ... but we still must provide the billing profile form as a part of the checkout process. Should we use temp storage of some sort for the billing profile and add it to stored PaymentMethod as soon as user returns from OffsitePaymentGateway?

I know this issue has a high priority on the #2913801: Commerce 2.x roadmap, hence I hope we can make some progress in the following weeks. I'll be working on this issue for the next 2 weeks with the hope that @bojanz or @mglaman will find time to brainstorm with me on the direction this issue should follow.

☝ Kinda answering myself 👇

I believe it would be best to reuse SupportsStoredPaymentMethod interface even though it is not the perfect fit here. The one and the only issue I see is that we must create PaymentMethod entity before we even take the user offsite, so that we can collect and sustain payment method data between checkout steps - billing profile, and for some providers, payment-method-type-specific fields, ex. telephone number (Vipps).

A solution could be to create the payment method stub - payment method entity flagged as not yet ready to be paid with and needing offsite finalizing before becoming fully-featured payment method. As far as I'm concerned even though such stubs can easily bloat the database I see it as the easiest and the least time-demanding solution, featuring all perks we get with the Commerce payment architecture.

Nevertheless, for every OffsitePaymentGateway implementing SupportsStoredPaymentMethod we would need an extra method - similar to OnsitePaymentGatewayInterface::createPayment() - that will handle payments for already stored payment methods.

Any objections or could I just try to code it?

@bojanz answered on Slack that he doesn't like the idea of stub.

In that case, we're most likely won't be able to have a payment method form of any kind for payment methods using offsite gateways - at least I can't think of any other way. The only thing we can do is to re-use the approach we have for one-time off-site gateways, so show the billing profile form element in payment details, which will be later referenced in $this->order->setBillingProfile(...) (@see Drupal\commerce_payment\Plugin\Commerce\CheckoutPane\PaymentInformation::submitPaneForm()).

But that means we won't be able to create off-site payment methods outside the checkout flow (-> user page)? We need a billing profile to be stored on the payment method and if we cannot use the order as temp storage then there is no (easy way) of solving it. It sounds like a big tradeoff to me, because the possibility of managing payment methods using user page was something our customers were looking for.

So another idea, which once again isn't perfect by any means but imho still something to be considered is to use TempStore for payment method stub (yes, the stub again) in a similar way we do the node previews. With this approach we don't actually allow $payment_method->save() - that should throw an exception - before the order comes back from the offsite gateway. But TempStore, or at least PrivateTempStore, won't be available for the anonymous user who deals with SupportsNotificationsInterface::onNotify which puts us back to square one...

One more idea before I 🤐. I think this is doable even though we won't support `add-payment-method` form this way. But maybe that's ok? I haven't heard many complaints from offsite-payment-gateway-developers that billing address is not enough for them. And the whole point of offsite gateways is that we won't process data in Drupal.

To the point:

1. Create a separate (from SupportsStoredPaymentMethod) interface for offsite payment gateway plugins. SupportsStoredPaymentMethod makes no sense here simply because createPaymentMethod needs a different signature to leverage Symfony\Component\HttpFoundation\Request which is the key source in offsite gateways.
2. Offsite gateways option labels won't be styled the same way we style Onsite, ie. "@type (@gateway)", simply because it's too early to determine payment method type. Offsite PSP allows you not only to take card payment but also support local payments with direct bank transfers, mobile apps, etc. Hence payment_method of given payment_method_type should be created after customer returns from offsite gateway (or using notify/push callback).
3. If payment provider gives two options - one-time payment OR tokenized payment - developer must create two payment gateway plugins. If the payment gateway plugin implements interface from point 1 we will always create a payment method. At the same time if the payment gateway plugin doesn't implement an interface from point 1 we will never create a payment method.
4. When you choose payment gateway the flow will be as it is now - user goes to review, then to payment checkout step which redirects using offsite-redirect form to the offsite gateway which then redirects back to the return controller. And now to the differences - we will call the ::createPaymentMethod on payment gateway plugin prior to ::onReturn/::onNotify. Former will create the payment method, latter the payment.
5. All subsequent payments using offsite payment method will look just like onsite payments - when PaymentProcess pane is reached we call ::createPayment, the method with the same signature as in OnsitePaymentGatewayInterface, which creates a payment based on the payment method and redirects you to the next checkout step - complete. Since signature and method name, as well as usage, is the same between onsite and offsite we can extract createPayment from OnsitePaymentGatewayInterface into a separate interface and make OnsitePaymentGatewayInterface extend it (BC layer). The interface from point 1 should also extend that one.

As much as I believe that creating payment methods outside the order context (user pages) is just must-have, I would rather do it in the follow-up, limiting the scope for this issue to the workable minimum. Related challenges - createPaymentMethod has now different signature (missing order context), billing profile form needs to be temporarily stored somewhere because the payment method cannot be created before user returns from the offsite gateway.

I should have patch ready tomorrow.

So here's a PoC to #10. There was some refactoring needed but overall I am happy with the results. Needs tests of course - manual and automated but I'd like to get some feedback first. Setting "Needs review" to see which tests fail - I have run only Unit and Kernel locally.

... and the patch :facepalm:

Interesting enough the only failing test is not related to this patch - #3009728: Product translation tests are failing for 8.6.x-dev - which theoretically means the patch in #12 doesn't cause any regression.

As much as I believe that creating payment methods outside the order context (user pages) is just must-have, I would rather do it in the follow-up, limiting the scope for this issue to the workable minimum.

^ #3009860: Support creating reusable payment method for off-site payment gateways from user page

The reroll of #12, but it requires some changes now that we validate step id and throw NeedsRedirectException early in the returnPage route callback . Perhaps we should give up on createPaymentMethod whatsoever and give the module developer freedom to create payment method via onReturn/onNotify if desired? But that would require them to modify Order object - set the payment_method and save - and the order, according to #2930789-6: Expand code documentation for off-site payment gateways, should not be touched.

In spite of all the above, I'm changing status to "Needs Review" to get some feedback about the patch from both the DrupalCI and developers.

+1 vote for this issue.

Required for PayFast, the South African payment processor.

re-rollin

I noticed that setting state to 'completed' on return it will trigger commerce_order.place.post_transition twice in checkout resulting in things like duplicate order emails.

Yes, issue stopped happening after update
there is something else im wondering about, should we save the payment method for anonymous users? I handled that by returning NULL from the createPaymentMethod in the PaymentGateway plugin implementation in case of anonymous orders but I’m not sure this should be the solution.

After applying the patch on the latest version of commerce, I got this error "Error: Call to a member function getPaymentMethodId() on null in Drupal\commerce_payment\Plugin\Commerce\CheckoutPane\PaymentInformation->buildPaneForm() (line 184 of modules/contrib/commerce/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php)."

I've removed the following code from the patch, I don't know if $default_option was defined before.

elseif ($payment_gateway->getPlugin() instanceof SupportsCreatingPaymentInterface && $default_option->getPaymentMethodId()) {
      // If customer wants to reuse off-site payment method.
      $pane_form = $this->buildPaymentMethodForm($pane_form, $form_state, $default_option);
    }

Changed wrong class of file StoredOffsiteRedirectWithPurchase.

If I understand the patch correctly
1. SupportsCreatingOffsitePaymentMethodsWithPurchaseInterface::createPaymentMethod() would be called on checkout after redirecting back from the gateway site but just before onReturn().
2. SupportsStoredPaymentMethodsInterface::createPaymentMethod() is called on the payment information checkout pane. It has another use on the user payment method management pages for onsite gateways.

1. and 2. are mutually exclusive. (We create payment methods differently for onsite and offsite on checkout.) However, we want to make user payment method management pages available for offsite gateways too.

A use case would be Elavon's XML API that supports payment method creation on both checkout and user pages.

My suggestion is introducing a third interface SupportsCreatingOffsitePaymentMethodsOnUserPageInterface with method createPaymentMethodOnUserPage(PaymentMethodInterface $payment_method, array $payment_details). We should also rename SupportsCreatingOffsitePaymentMethodsWithPurchaseInterface to SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface for consistency and clarity. So that's what we would end up with:

Thanks for the great work here. I just start using the patch and noticed that the payment method always attaches to the order as NULL.

It's because of that SupportsCreatingOffsitePaymentMethodsWithPurchaseInterface::createPaymentMethod not require to return any value, but in PaymentCheckoutController.php we have next code:

        // In case payment gateway supports payment method.
        $payment_method = $payment_gateway_plugin->createPaymentMethod($payment_method, $order, $request);

        // Add payment method to the order.
        $order->set('payment_method', $payment_method);
        $order->save();

So in most cases $payment_method will be equal to the NULL.

Changed status.

This patch:

1. Should fix tests.
2. Renames a few things.
3. Injects a few dependencies instead of using \Drupal::.
4. Adds a controller and gateway plugin return/cancel methods for offsite payment method creation.
5. Adds a StoredOffsiteRedirect gateway to commerce_payment_example.
6. Introduces a dummy payment_method creation for the StoredOffsiteRedirect example gateway

Tests are still to be added. Setting to "Needs review" just to make the bot run tests.

Retry.

Retry #2. (Tests are still needed.)

And the tests.

Setting the payment's payment method.

Further test fixes.

Yet another testfix.

Giving this a look over 👀

I have got to run for a call. Here are some initial items. I definitely think we're going to need to split out some of this work into other issues to help focus on some of the API improvements of the payment module. Unless we can reconcile them here. This is a great start, @czigor! With passing tests it's time to tighten it up.

Unassigning for now.

1. +++ b/modules/payment/js/offsite-redirect.js
   @@ -16,7 +16,7 @@
   -      $('.payment-redirect-form', context).submit();
   +      $('.payment-redirect-me', context).submit();
   
   +++ b/modules/payment/src/PluginForm/PaymentMethodOffsiteAddForm.php
   @@ -0,0 +1,113 @@
   +      $form['#attributes']['class'][] = 'payment-redirect-me';
   ...
   +    $complete_form['#attributes']['class'][] = 'payment-redirect-me';
   
   +++ b/modules/payment/src/PluginForm/PaymentOffsiteForm.php
   @@ -60,6 +60,7 @@ abstract class PaymentOffsiteForm extends PaymentGatewayFormBase {
   +      $form['#attributes']['class'][] = 'payment-redirect-me';
   
   @@ -103,7 +104,7 @@ abstract class PaymentOffsiteForm extends PaymentGatewayFormBase {
   -    $complete_form['#attributes']['class'][] = 'payment-redirect-form';
   +    $complete_form['#attributes']['class'][] = 'payment-redirect-me';
   

   Is there a reason we renamed this class? It was changed in #28 but I don't know why. It doesn't seem required and reduces some diff noise.

   I'm not against it with good reasoning, but I'm also for not changing it to just change it (keeps diff easier to read, changes more succint

2. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +  public function __construct(LoggerInterface $logger, EntityTypeManagerInterface $entity_type_manager, TranslationInterface $translation, AccountProxyInterface $current_user) {
   

   Missing phpdoc for $current_user

3. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +    $this->stringTranslation = $translation;
   

   Declared dynamically.

4. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The route match.
   +   */
   +  public function returnPage(Request $request, RouteMatchInterface $route_match) {
   

   Missing return tag

5. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +    /** @var \Drupal\Core\Session\AccountInterface $account */
   +    $account = $route_match->getParameter('user');
   +    /** @var \Drupal\commerce_payment\Entity\PaymentGatewayInterface $payment_gateway */
   +    $payment_gateway = $route_match->getParameter('payment_gateway');
   

   Since these are defined in the route with proper options.parameters values, we can just place them in the method parameters for injection. We don't need to fetch them from the route match.

6. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +    catch (PaymentGatewayException $e) {
   +      // If creating payment method failed we allow onReturn to handle
   +      // creating Payment, which is required to fulfill the payment. This
   +      // exception is muted and logged.
   +      $this->logger->error($e->getMessage());
   +    }
   +  }
   

   There is no return statement here and Drupal will crash since the controller did not return a render array or response.

   If there is an exception, how should this be handled?

7. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The route match.
   +   */
   +  public function cancelPage(Request $request, RouteMatchInterface $route_match) {
   

   Missing @return in phpdoc

8. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +    /** @var \Drupal\commerce_payment\Entity\PaymentGatewayInterface $payment_gateway */
   +    $payment_gateway = $route_match->getParameter('payment_gateway');
   ...
   +    /** @var \Drupal\Core\Session\AccountInterface $account */
   +    $account = $route_match->getParameter('user');
   

   Again, let's add this to method params for injection versus manually pulling from the route match

9. +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   @@ -0,0 +1,149 @@
   +  /**
   +   * Access callback for the offsite payment method endpoints.
   +   */
   +  public function checkAccess(RouteMatchInterface $route_match) {
   +    $account = $route_match->getParameter('user');
   +    return AccessResult::allowedIf($this->currentUser->hasPermission('manage own commerce_payment_method') && $account->id() == $this->currentUser->id());
   

   This would not allow an admin to create a payment method for a user.

   I'm fine keeping this restriction to start. We'll just want to open a follow up to document it's not supported for admins to create offsite payment methods for other users.

10. +++ b/modules/payment/src/Controller/PaymentCheckoutController.php
    @@ -51,11 +60,14 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
    +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface
    +   *   The entity type manager.
        */
    

    missing param name

11. +++ b/modules/payment/src/Controller/PaymentCheckoutController.php
    @@ -94,6 +107,38 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
    +    if ($payment_gateway_plugin instanceof SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface) {
    
    +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface.php
    @@ -0,0 +1,29 @@
    +interface SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface {
    
    +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/SupportsCreatingOffsitePaymentMethodsOnUserPageInterface.php
    @@ -0,0 +1,37 @@
    +interface SupportsCreatingOffsitePaymentMethodsOnUserPageInterface {
    

    Why do we need a specific interface for creating offsite payment methods on checkout?

    If it's supported, it should support both instances since it is the same form.

    At a quick glance, the main difference is the User interface expects a less completed payment method object. I think we should reconcile this so that we don't need more interfaces.

12. +++ b/modules/payment/src/PaymentMethodStorage.php
    @@ -86,7 +86,7 @@ class PaymentMethodStorage extends CommerceContentEntityStorage implements Payme
    -    if (!($payment_gateway->getPlugin() instanceof SupportsStoredPaymentMethodsInterface)) {
    +    if (!($payment_gateway->getPlugin() instanceof SupportsCreatingPaymentInterface)) {
    

    This is where the interface name feels weird. All payment gateways can create payments. But the Payment API assumes the createPayment method is provided a payment with a populate payment method.

    This is code smell which, to me, means we need to reconcile SupportsStoredPaymentMethodsInterface, SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface, and SupportsCreatingOffsitePaymentMethodsOnUserPageInterface

13. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/SupportsCreatingPaymentInterface.php
    @@ -0,0 +1,33 @@
    +/**
    + * Reusable payment methods can be later used to create payment with no user
    + * action required. This applies to all on-site payment gateways and some
    + * off-site payment gateways which have a support for payment methods.
    + *
    + * Add this interface to all payment gateway plugins that support reusing
    + * payment methods.
    + */
    +interface SupportsCreatingPaymentInterface {
    

    Why did we make an interface for creating payments? Oh, because it only existed on the OnSitePaymentGatewayInterface.

    This makes sense. But needs to be worked on some more, further comments to follow.

14. +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/SupportsDeletingPaymentMethodsInterface.php
    @@ -0,0 +1,25 @@
    +interface SupportsDeletingPaymentMethodsInterface {
    
    +++ b/modules/payment/src/Plugin/Commerce/PaymentGateway/SupportsStoredPaymentMethodsInterface.php
    @@ -5,9 +5,9 @@ namespace Drupal\commerce_payment\Plugin\Commerce\PaymentGateway;
    -interface SupportsStoredPaymentMethodsInterface {
    +interface SupportsStoredPaymentMethodsInterface extends SupportsDeletingPaymentMethodsInterface {
    

    Any reason we split out deleting? The only reason we added a "SupportsUpdating" is because we missed that functionality in the first API pass and had to add it as a new interface to support backwards compatability.

EDIT:

As soon as I commented, I realized the createPayment method problem is because it was on OnSitePaymentGatewayInterface and has a dependency on payment methods. The method name really means createPaymentWithPaymentMethod, but that's ugly. But that makes me feel the method really belongs in SupportsStoredPaymentMethodsInterface. But that can also feel weird. But it highlights needing to reconcile SupportsStoredPaymentMethodsInterface, SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface, and SupportsCreatingOffsitePaymentMethodsOnUserPageInterface from the patch.

1. It had to do something with the js getting triggered on forms it was not supposed to. I can't remember exactly. Let's undo the rename and see if the tests pass for now.
2. Fixed.
3. We don't need it, removed. Added the string translation trait which declares it.
4. Fixed.
5. Fixed.
6. Right, so this is the case when the payment method creation fails. I think we should display a message and still redirect to the user's payment methods collection page. Implemented this.
7-10. Fixed.
11. It's because the process is different for creating a payment method on checkout and one on the user pages. For example, they need to have different return controllers. Or the gateway might not even support creating payment methods on the user pages. See Bojan's issue description and \Drupal\commerce_payment_example\Plugin\Commerce\PaymentGateway\StoredOffsiteRedirect.
12-14 I agree that there would be room for improvements on naming, but changing the name of SupportsStoredPaymentMethodsInterface or createPayment() would break BC. For SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface and SupportsCreatingOffsitePaymentMethodsOnUserPageInterface I tried to choose descriptive names even if they might not be in line with existing patterns. We could try to make up for the name calamity in the interfaces' comments.

We can't put createPayment() into SupportsStoredPaymentMethodsInterface since SupportsStoredPaymentMethodsInterface is only for onsite and createPayment() is needed on both onsite and offsite. (Note that SupportsStoredPaymentMethodsInterface and SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface are mutually exclusive as they both have createPaymentMethod() but with different arguments.)

The route entity.commerce_payment_method.collection is personalized but access check \Drupal\commerce_payment\Access\PaymentMethodAccessCheck::checkAccess() does not use administer commerce_payment_method permission, so why that access check extended for new routes?

+++ b/modules/payment/commerce_payment.routing.yml
@@ -101,3 +101,29 @@ commerce_payment.notify:
+  path: '/user/{user}/payment-methods/{payment_gateway}/return'
...
+        type: entity:user
...
+  path: '/user/{user}/payment-methods/{payment_gateway}/cancel'

+++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
@@ -0,0 +1,169 @@
+  public function returnPage(PaymentGatewayInterface $payment_gateway, AccountInterface $user, Request $request) {
...
+      $payment_method = $payment_method_storage->create([
...
+        'uid' => $user->id(),
...
+      return new RedirectResponse(Url::fromRoute('entity.commerce_payment_method.collection', ['user' => $user->id()])->toString());
...
+      return new RedirectResponse(Url::fromRoute('entity.commerce_payment_method.collection', ['user' => $user->id()])->toString());
...
+  public function cancelPage(PaymentGatewayInterface $payment_gateway, AccountInterface $user, Request $request) {
...
+    return new RedirectResponse(Url::fromRoute('entity.commerce_payment_method.collection', ['user' => $user->id()])->toString());
...
+  public function checkAccess(AccountInterface $user) {
...
+      ($this->currentUser->hasPermission('manage own commerce_payment_method')
+      && $user->id() == $this->currentUser->id())
+      || $this->currentUser->hasPermission('administer commerce_payment_method')

Return(cancel) page using to pass User via route, but it used only in checkAccess() and "hacky way" for administrator to masquerade as a user

Is this behavior intended? Then it needs extra test coverage

Thanks for noticing, I was not aware of \Drupal\commerce_payment\Access\PaymentMethodAccessCheck::checkAccess() . Removing the custom access check. (I think they checked the same thing though.)

🙏 thanks, @czigor for the updates and @andypost for catching that access check.

Assigning to myself for another round of review. The patch is just under 70kb, partially because of all of the new files. I'd like to see if we can find ways to spin out any succinct changes into other issues. This touched on a lot of required improvements for the Payment API.

It's because the process is different for creating a payment method on checkout and one on the user pages. For example, they need to have different return controllers. Or the gateway might not even support creating payment methods on the user pages.

This is in regards to my comment about why do we need SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface and SupportsCreatingOffsitePaymentMethodsOnUserPageInterface.

I can see why we need SupportsCreatingOffsitePaymentMethodsOnUserPageInterface. But maybe we can reconcile SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface and SupportsStoredPaymentMethodsInterface

The offsite interface provides

public function createPaymentMethod(PaymentMethodInterface $paymentMethod, OrderInterface $order, Request $request);

The onsite provides

public function createPaymentMethod(PaymentMethodInterface $payment_method, array $payment_details);

The array $payment_details parameter has no limitations to what it may contain. For onsite payment gateways, it pulls from (typically) hidden HTML inputs.

The \Drupal\commerce_payment\Controller\PaymentCheckoutController::returnPage method could very well provide the request in that payment details array, or just the entire request's request parameters (since that would be the true replacement of hidden input values. It might be a GET and might be POST, so use $request->request->all()).

The example in \Drupal\commerce_payment_example\Plugin\Commerce\PaymentGateway\StoredOffsiteRedirect::createPaymentMethod works all of the time, and creating a payment method was intended to work without the context of an order. When working on Zuora and other offiste iframes that support creating the payment method, we never needed items in the order.

As per Matt's comment this patch update is removing SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface.

Error is due to the test gateway still implementing SupportsDeletingPaymentMethodsInterface when SupportsCreatingPaymentInterface implements it as well

So, I was thinking:

We split out SupportsDeletingPaymentMethodsInterface before because we had custom SupportsPaymentMethods. This makes sense, because we have "Supports" which allows creation, then SupportsEdit and we can do SupportsDelete. I consider payment methods as either reusable or non-reusable payment instruments.

So what I'm proposing is we split SupportsDeletingPaymentMethodsInterface out into a new issue, since I don't think it's required here anymore.

Then, with the current patch we don't have a Checkout specific interface anymore. So maybe we should split SupportsCreatingOffsitePaymentMethodsOnUserPageInterface into its own issue. With the latest patch we can support payment methods with offsite gateways – then a follow up could allow creation from user pages.

That could help split things out a lot.

To make it more feasible, we could turn this issue into a META/PLAN and then the following child issues:

• Modify PaymentCheckoutController to support SupportsStoredPaymentMethodsInterface
• Create a payment method delete interface out of SupportsStoredPaymentMethods
• Allow creating offsite gateway payment methods from the user page

I think this route could make it more feasible to commit and grok.

Thoughts?

Discussed with @czigor my thoughts in #54. I'm going to take the work done here and chunk it out into more manageable pieces.

I found a bug in #51 after my idea of removing SupportsCreatingOffsitePaymentMethodsOnCheckoutInterface. The AddPaymentMethodForm is rendered in checkout, when it shouldn't be. For off-site gateways the payment method is not created before the payment transaction, it is a result of that process.

This should fix the test failures, however

• When reusing the off-site payment method, the billing profile form still renders. It shouldn't as the payment method already has a billing profile attached
• The onReturn handler attempts to recreate the payment method, even if an existing one was used

Here's a second revision. This removes the changes to SupportsStoredPaymentMethodsInterface and creation of SupportsDeletingPaymentMethodsInterface. I don't think we need to make this explicit change, especially since we never type hint for it.

Okay, new iteration. SupportsCreatingOffsitePaymentMethodsOnUserPageInterface extends SupportsStoredPaymentMethodsInterface. This removes onReturnOffsitePaymentMethodOnUserPage in favor of createPaymentMethod.

We consume the query or request parameters and pass those is in as the payment details for offsite gateways.

🤦‍♂️ wrong interdiff before.

+++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
@@ -0,0 +1,159 @@
+      $payment_method_storage = $this->entityTypeManager->getStorage('commerce_payment_method');
+      /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
+      $payment_method = $payment_method_storage->create([
+        // Payment method type may depend on the request payload. We create
+        // payment method stub based on default payment method type available
+        // for payment gateway plugin but module developers can swap it
+        // with custom payment method. Keep in mind that $payment_method is
+        // passed by reference and code below relies on that reference to add
+        // payment method to the order.
+        'type' => $payment_gateway_plugin->getDefaultPaymentMethodType()->getPluginId(),
+        'payment_gateway' => $payment_gateway,
+        'uid' => $user->id(),
+        'payment_gateway_mode' => $payment_gateway_plugin->getMode(),
+      ]);

+++ b/modules/payment/src/Controller/PaymentCheckoutController.php
@@ -94,6 +107,39 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
+        $payment_method_storage = $this->entityTypeManager->getStorage('commerce_payment_method');
+        /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
+        $payment_method = $payment_method_storage->create([
+          // Payment method type may depend on the request payload. We create
+          // payment method stub based on default payment method type available
+          // for payment gateway plugin but module developers can swap it
+          // with custom payment method. Keep in mind that $payment_method is
+          // passed by reference and code below relies on that reference to add
+          // payment method to the order.
+          'type' => $payment_gateway_plugin->getDefaultPaymentMethodType()->getPluginId(),
+          'payment_gateway' => $payment_gateway,
+          'uid' => $order->getCustomerId(),
+          'billing_profile' => $order->getBillingProfile(),
+          'payment_gateway_mode' => $payment_gateway_plugin->getMode(),
+        ]);

In two places we create a payment method stub with this patch. \Drupal\commerce_payment\Plugin\Commerce\CheckoutPane\PaymentInformation::buildPaymentMethodForm does something similar.

I want to see if there is a way to reuse some of the code here.

In our offsite code we use the payment gateway and plugin directly whereas the PaymentInformation pane uses a PaymentOption value object.

This patch removes the introduced SupportsCreatingPaymentInterface interface. The createPayment method was moved to SupportsStoredPaymentMethods, as that was the intent of the method when attached to OnsitePaymentGatewayInterface.

This helps reduce the number of API changes and keeps some assumptions, but makes it easy for an off-site gateway to implement stored payment methods. This should also fix the situation where off-site still went off-site when reusing a payment method.

Next steps:

• Address #61 by adding a createFromCustomer method on the payment method storage (naming thanks to lisastreeter)
• Have off-site payment gateways render their add_payment_method form in checkout without crashing, so we properly collect billing information and don't display the collection when reusing a stored payment method We don't want any payment method entities created, just the capture of billing information if configured

One more before heading out. This is a little clean up.

We can't let off-site payment gateways render add-payment-method forms, as it'd create a bogus payment method entity. This patch ensures the billing_information element renders if the gateway collects billing information, but not if we're using an existing stored off-site payment method.

For the checkout flow and reusing payment methods, things seem all right. I want to revisit the sample payment gateway based on my experience with Zuora, which used an iframe as the off-site method and returned a payment token from that process. I also want to review PayPal Checkout's process for stored payment methods.

-----

For Zuora's payment pages, the iframe posts a redirect/callback that contains either an error code or refId. The refId represents the payment method, which allows you to perform charges.

With our currently flow, the offsite gateway would create the payment method and then be able to utilize is in the onReturn, instead of processing it directly in the onReturn callback.

That's the one awkward part. The \Drupal\commerce_payment\Controller\PaymentCheckoutController::returnPage controller is invoking createPaymentMethod and then onReturn. It's a helper that we're calling that for off-site gateways. But maybe they should be responsible for creating the payment method within onReturn? That might be difficult from a developer experience perspective.

---

TBD: PayPal

Going back to #2, and as I have worked on this: I wonder if we should make SupportsCreatingStoredPaymentMethodsInterface. The naming feels weird though because you support stored payment methods and maybe don't support creating? But the idea of creating goes for creation not as a side effect of payment.

For instance, Authorize.net has off-site hosted payment forms. You can then take the transaction ID and convert it into a customer profile and payment profile.

+++ b/modules/payment/src/Controller/PaymentCheckoutController.php
@@ -94,6 +107,38 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
+    if ($payment_gateway_plugin instanceof SupportsStoredPaymentMethodsInterface) {
+      try {
+        $payment_method_storage = $this->entityTypeManager->getStorage('commerce_payment_method');
+        /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
+        $payment_method = $payment_method_storage->create([
+          // Payment method type may depend on the request payload. We create
+          // payment method stub based on default payment method type available
+          // for payment gateway plugin but module developers can swap it
+          // with custom payment method. Keep in mind that $payment_method is
+          // passed by reference and code below relies on that reference to add
+          // payment method to the order.
+          'type' => $payment_gateway_plugin->getDefaultPaymentMethodType()->getPluginId(),
+          'payment_gateway' => $payment_gateway,
+          'uid' => $order->getCustomerId(),
+          'billing_profile' => $order->getBillingProfile(),
+        ]);
+        // In case payment gateway supports payment method.
+        $payment_details = $request->isMethod('GET') ? $request->query->all() : $request->request->all();
+        $payment_gateway_plugin->createPaymentMethod($payment_method, $payment_details);
+
+        // Add payment method to the order.
+        $order->set('payment_method', $payment_method);
+        $order->save();
+      }
+      catch (PaymentGatewayException $e) {
+        // If creating payment method failed we allow onReturn to handle
+        // creating Payment, which is required to fulfill the payment. This
+        // exception is muted and logged.
+        $this->logger->error($e->getMessage());
+      }
+    }

Part of the awakwardness is that we have all of this logic before the onReturn method.

If an on-site supports stored payment methods, they should be responsible for generating them in onReturn.

And if they implement SupportsCreatingStoredPaymentMethodsInterface it means they can be added from the user page.

Maybe.

EDIT: if we made the onReturn responsible, it'd help make the case for a payment method storage method for creating the initialized payment method entity.

Just a quick note to the onReturn method responsibility for creating payment method, previously mentioned in #15

* Note that onReturn() will be skipped if onNotify() was called before the
* customer returned to the site, completing the payment process and
* placing the order.

and

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

see https://www.drupal.org/project/commerce/issues/2930789#comment-12835134

I will post more detailed comment later but please note that #3009860: Support creating reusable payment method for off-site payment gateways from user page has already been created as per #14 and I stick to what I said - this is a good material for a follow up.

I haven't had a chance yet to update this issue, but here's a summary I wrote up in Notion: https://www.notion.so/Payment-API-improvements-b0c721f615d04a18b5cfc48f0...

Per #67: the idea is to get it "working" here, and then split into relevant issues

💪 Cleared off my plate to tackle this today.

Updated patch leveraging SupportsCreatingPaymentMethodsInterface. Letting the tests run, doing a review after lunch.

Feedback for myself. I'm tempted to remove everything related to creating off-site payment methods and handling it in #3009860: Support creating reusable payment method for off-site payment gateways from user page after we commit, but I'm afraid it might cause even more disruption, like if we don't get something right here.

1. +++ b/modules/payment/commerce_payment.routing.yml
   @@ -101,3 +101,29 @@ commerce_payment.notify:
   +    _controller: '\Drupal\commerce_payment\Controller\OffsitePaymentMethodUserPageController::returnPage'
   ...
   +    _controller: '\Drupal\commerce_payment\Controller\OffsitePaymentMethodUserPageController::cancelPage'
   
   --- /dev/null
   +++ b/modules/payment/src/Controller/OffsitePaymentMethodUserPageController.php
   

   I think we should rename this to PaymentUserController like we have PaymentCheckoutController.

   The checkout controller handles checkout procedures. So we should have a user controller.

   This would pertain to adding payment methods, but other actions as well that could come in the future.

   I need to brush off SCA, but we may need callbacks that help with SCA workflows, such as approving a recurring payment or async payment.

   It doesn't cost anything to use the name and allows it to have wider scope.

2. +++ b/modules/payment/src/Controller/PaymentCheckoutController.php
   @@ -94,6 +107,38 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
   +    if ($payment_gateway_plugin instanceof SupportsStoredPaymentMethodsInterface) {
   +      try {
   +        $payment_method_storage = $this->entityTypeManager->getStorage('commerce_payment_method');
   +        /** @var \Drupal\commerce_payment\Entity\PaymentMethodInterface $payment_method */
   +        $payment_method = $payment_method_storage->create([
   +          // Payment method type may depend on the request payload. We create
   +          // payment method stub based on default payment method type available
   +          // for payment gateway plugin but module developers can swap it
   +          // with custom payment method. Keep in mind that $payment_method is
   +          // passed by reference and code below relies on that reference to add
   +          // payment method to the order.
   +          'type' => $payment_gateway_plugin->getDefaultPaymentMethodType()->getPluginId(),
   +          'payment_gateway' => $payment_gateway,
   +          'uid' => $order->getCustomerId(),
   +          'billing_profile' => $order->getBillingProfile(),
   +        ]);
   +        // In case payment gateway supports payment method.
   +        $payment_details = $request->isMethod('GET') ? $request->query->all() : $request->request->all();
   +        $payment_gateway_plugin->createPaymentMethod($payment_method, $payment_details);
   +
   +        // Add payment method to the order.
   +        $order->set('payment_method', $payment_method);
   +        $order->save();
   +      }
   +      catch (PaymentGatewayException $e) {
   +        // If creating payment method failed we allow onReturn to handle
   +        // creating Payment, which is required to fulfill the payment. This
   +        // exception is muted and logged.
   +        $this->logger->error($e->getMessage());
   +      }
   

   The createPaymentMethod method was removed from this interface. It is on the payment gateway to create a payment method within it's onReturn method.

   If we don't, we should have commerce_payment.order_updater service sync the payment's payment method to the order. That would help having to manually set the payment method.

3. +++ b/modules/payment/src/Form/PaymentMethodAddForm.php
   @@ -71,7 +73,7 @@ class PaymentMethodAddForm extends FormBase implements ContainerInjectionInterfa
   -      if (!$payment_gateway || !($payment_gateway->getPlugin() instanceof SupportsStoredPaymentMethodsInterface)) {
   +      if (!$payment_gateway || (!($payment_gateway->getPlugin() instanceof SupportsStoredPaymentMethodsInterface) && !($payment_gateway->getPlugin() instanceof SupportsCreatingOffsitePaymentMethodsOnUserPageInterface))) {
   

   Clean this up to just SupportsCreating interface

4. +++ b/modules/payment/src/PaymentGatewayStorage.php
   @@ -70,7 +71,7 @@ class PaymentGatewayStorage extends ConfigEntityStorage implements PaymentGatewa
   -      return $payment_gateway->getPlugin() instanceof SupportsStoredPaymentMethodsInterface;
   +      return $payment_gateway->getPlugin() instanceof SupportsStoredPaymentMethodsInterface || $payment_gateway->getPlugin() instanceof SupportsCreatingOffsitePaymentMethodsOnUserPageInterface;
   

   This should just be SupportsStored

5. +++ b/modules/payment/src/PluginForm/PaymentOffsiteForm.php
   @@ -60,6 +60,7 @@ abstract class PaymentOffsiteForm extends PaymentGatewayFormBase {
   +      $form['#attributes']['class'][] = 'payment-redirect-form';
   

   This is already added in the processRedirectForm process callback.

Okay, I think this is pretty close.

I created \Drupal\commerce_payment\PaymentMethodStorageInterface::createForCustomer to help with initializing a payment method to be created. #3137107: Provide a storage method to initialize a new payment method for a customer

It's also safe to spin \Drupal\commerce_payment\Plugin\Commerce\PaymentGateway\SupportsCreatingPaymentMethodsInterface into its own issue and move createPaymentMethod from SupportsStored to this new method.

It should also be safe to move createPayment from the OnSite interface to SupportsStoredPaymentMethodsInterface, as every gateway I saw implements the OnSite interface. #3137116: Move createPayment to SupportsStoredPaymentMethodsInterface

That spins a lot of the ground work out of this issue.

Reroll of #72 after the following were committed

• #3137107: Provide a storage method to initialize a new payment method for a customer
• #3137116: Move createPayment to SupportsStoredPaymentMethodsInterface

Next, I'll create an issue for the SupportsCreatingPaymentMethodsInterface change.

One thing got missed:

  protected function getDefaultForms() {
    $default_forms = [];
    if ($this instanceof SupportsStoredPaymentMethodsInterface) {
      $default_forms['add-payment-method'] = 'Drupal\commerce_payment\PluginForm\PaymentMethodAddForm';
    }

This needs to change to: SupportsCreatingPaymentMethodsInterface

🥳2.19 released with support for off-site payment gateways to charge stored payment methods.

The remaining task is #3009860: Support creating reusable payment method for off-site payment gateways from user page. Let's close this issue and carry on there with this last patch.

@mglaman thanks for creating a patch, I tried your last posted i.e. #70 but sorry to say my requirement is not being achieved.
Actually I'm finding a solution to process the recurring payments/subscriptions with offsite payment-gateways those not giving any payment methods just redirecting to own payment page(gateway).

Could anyone give me a solution or idea please, hoping will have positive reply as quick as possible.

I tried PATCH-70 & PATCH-74 but both failed, giving this error
With #Patch 70 :

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-05-15/meta_allow_offsite-283838...

With Patch-74:

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-05-15/meta_allow_offsite-283838...

rerolled 74 against dev from git