Better support for remote payment methods / redirects

You're absolutely right that the current implementation of redirecting users needs to be improved. The reason no RedirectResponse is returned, is because I figured payment execution may not always happen in a context that allows responses to bubble up to the controllers.

The payment type's context (a.k.a. "payment executor) can be embedded and not have control over the controller and the response. This is what happens with Payment for Drupal Commerce 7.x-1.x, for instance. Basically whenever the context is not the primary application within Drupal.

I guess the payment type's context can then still translate a response to whatever its primary application supports.

I wonder if we can refactor \Drupal\payment\Plugin\Payment\Method\PaymentMethodInterface::isPaymentExecutionInterruptive() to define whether the payment method execute() method will return a response or can be called 'inline' without interrupting the request, and if the payment type's context can then process *any* type of response that is given back.

We must take care when allowing payment methods to return responses, because if they can return a response, there must be a request too. What if there isn't one?

Why must there be?

Because if there is no request (and payment method plugins can have that injected themselves, we don't need to pass it on), what will plugins return a response to?

What about this? The only drawback I see is that with the old approach, payment methods defaulted to being interruptive (which usually means they redirect the user), to which payment type contexts could respond by executing payments in separate windows. Now, payment methods can say they return responses upon payment execution, but don't, or not return responses while they say they do.

That will be easier in most cases, but a redirect is a payment method implementation and I would not want to move that entire (and implementation-specific) responsibility up the chain.

Returning a response takes slightly more code, but does keep the approach fairly generic.

If those methods return responses, then they could theoretically also return responses that display an HTML form with some JavaScript to submit the form using POST, right?

I switched to Url because of form redirects, which is used in one of the two cases. where we actually do something with the response of the whole stack.

Could you elaborate on this?

Returning a Url object for REST works only if the redirect uses GET rather than POST.

I am extremely careful with this, because other platforms have implemented these features in too specific ways in the past and in doing so imposed limitations on plugins and I don't intend on making the same mistake.

Alternatively, is there a way to get the controller for the current request and feed it a Response object directly, or is that the hack that Payment currently uses?

That only supports a GET redirect. It's still perfectly possible to do anything else, but I don't want to communicate to developers that we support one type of payment execution more than we support others.

Alternatively, is there a way to get the controller for the current request and feed it a Response object directly, or is that the hack that Payment currently uses?

Would you know more about this?

Completely unfinished, but this builds upon my first patch to return responses.

Re-roll.

I think I am convinced that returning a \Drupal\Core\Url is useful. However, I would like to explore the options to also allow payment methods to return a response if not a web service, but a user requests payment execution. Is there a way to distinguish between such requests? We could also introduce a PaymentExecutionInterface and let implementations expose whether they can return a URL and/or a response, or nothing at all, but that could be considered overkill.

Such responses could contain a form that does a POST redirect, for instance. It's slightly nicer than doing a GET redirect to such a form first.

This also means that any payment method that does not need to redirect payers can return the URL returned by PaymentType::resumeContext(), because we'd have to change that method here as well.

What other than a GET redirect would you want to support? POST redirects don't really exist and you can return an actual page either.

can or can't?

Well, if a controller is being used for the current request, then we can feed it a Response, right? So by default executing a payment returns nothing, but we will allow a Url to be returned for redirects. However, if there is a sane way to check if a controller is being used, we can let payment methods also return a Response.

Such Response objects can then contain a complete page, possibly with a form that automatically (using JS) POSTs to a third party URL. We can also easily do this by first returning a Url, redirect to another page on the site, and then POST a form to the remote server, but that would require an extra step. It's not very clean.

I discussed this with @donquixote at DrupalCamp Ghent and we worked out the following idea:

• PaymentMethodInterface::executePayment() returns NULL or a PaymentExecutionInterface object. The object can contain a URL and a Response object that both allow calling code to continue the payment execution if necessary. Calling code can then decide to feed back the response to the controller, or to perform a GET redirect to the given URL.
• #2372041: Provide a helper for form POST redirects aims at providing helpers for payment methods that need to perform form POST redirects.

What do you think of this?

I think we still have slightly different ideas, but Xano knows much more about the module and the use cases. Introducing the return object and the helper seems like a good direction. Once implemented on a few payment methods, we'll see if this can or should be improved.

EDIT: Main points I was that
- Payment plugins should focus on what distinguishes one payment method from another, and avoid redundant boilerplate. This is what the helper is for.
- The less details of the helper are exposed to payment plugins, the easier it will be to refactor the helper later.

These were my thoughts, all in the "DRY" direction. But I am not pushing for a specific solution.

(And now back to silently observing)

This is a partial patch for the approach discussed in the past few comments.

The patch from #42 introduces the concept of a payment execution status, which can have three 'values':

1. Completed.
2. In progress. This is when the payment method should provide additional information for resuming/continuing/completing the execution workflow.
3. Failed/aborted.

The patch uses booleans for 1 and 3, and an object for 2, which admittedly is somewhat inconsistent and if we continue using this approach, we'll want to fix this. The object that is available in case the execution is still in progress contains a URL at which execution can be resumes. An optional response may be available too, and if the payment type's context is able to pass it on to a controller, it can do so. However, only support for the URL and GET redirects is required.
In addition to all this, the patch also adds a resume entity operation, so payers can make use of this feature from within the UI. It's a little out of scope, but it illustrates why I added a new method for getting the payment execution status and it provides a default use case.

Maybe we will want to change resume to complete, so we can use it for payments of which the owner never even started execution yet, as is the case with \Drupal\payment_reference\Element\PaymentReferenceBase.

PaymentInterface::execute() will need to be refactored too.

The last two patches should give you a good idea of why I wanted to try and support responses as well. Now we have a dedicated Complete operation for payments with a corresponding controller, this route can just return the payment's execution completion response. Because we can always build a RedirectResponse on-the-fly as long as we have a URL (and we will always have one), any calling code can also assume that as long as there is a completion response, it can freely choose between using the URL directly or the complete response.

Adding the Complete operation was not necessary for this issue, but it provides a good use case, as described above. It also shows that the fundamental changes we are making here contribute to supporting customers resuming payments they started earlier, but for which they aborted the execution workflow.

Just a few unit test failures left.

Fixed all unit test failures and bugs they exposed.

Teestboooot?

...

Attempted test run #396...

Re-roll.

I cleaned up a bit based on my findings for #2372041: Provide a helper for form POST redirects.

Re-roll.

Re-roll.

This is the correct patch file.

Re-roll.

Having an object that can have either a response or a redirect URL seems to be very complex to support. How do you know which one to use when when you get such a response object? I've seen you for example also hardcode getRedirectUrl() in the form submit callback, what happens if a payment method returns a Response?

It MUST return a \Drupal\payment\Response\ResponseInterface which contains both a URL and a Symfony response. There MUST always (also) be a URL to redirect to. If the payment method provides no complex response of its own, it can return a \Drupal\payment\Response\RedirectResponse, which converts a URL to a Symfony response automatically. Calling code is allowed to either use the URL or the Symfony response from the, because redirecting to the URL or returning the Symfony response MUST have the same effect in terms of payment execution.

Also not sure about the separation of doExecutePayment() and getPaymentExecutionResult(), what do we gain from that? I'm not sure what I'm supposed to do now in which method. Our payment methods just build up a URL to redirect to, for now, I've built it in doExecutePayment(), stored it in a property and then I return it in the other method.. but why do I have to do that?

\Drupal\payment\Plugin\Payment\Method\PaymentMethodInterface::getPaymentExecutionResult() exists so we can determine whether the payment execution process was completed or not. This allows us to resume the process in case the payer closed their browser window, for instance. \Drupal\payment\PaymentMethod\PaymentMethodBase::doExecutePayment() is a convenience method, so plugins won't have to deal with all the logic in \Drupal\payment\PaymentMethod\PaymentMethodBase::executePayment().

Additionally:

Our payment methods just build up a URL to redirect to, for now, I've built it in doExecutePayment(), stored it in a property and then I return it in the other method.. but why do I have to do that?

If the redirect URL is unique to the payment and cannot be computed, you must indeed store it somewhere (likely the plugin configuration) so it can be returned at any time. You will only have to return it in \Drupal\payment\Plugin\Payment\Method\PaymentMethodInterface::getPaymentExecutionResult(), not in any other methods.

Who would call getPaymentExecutionResult() without first calling executePayment()? Is it even possible? Because right now in my implementation, it is not, and it is also not possible to call it later on.

\Drupal\payment\Controller\CompletePayment does this in the patch in order to let payers continue a payment after the execution process was aborted. This can, of course, only be done if the payment method supports the continuation of the payment's execution. If it doesn't, it does not have to return a response/URL and it can simply say execution was completed.

doExecutePayment() might be a convience method, but it is a *required* convenience method, and I must save the payment there or things do not work. So again, why not just return a result from doExecutePayment()?

If you have suggestions for making saving payments easier, I'd be happy to hear them. I feel we have to impose this responsibility either on payment type or payment method plugins, but I may well be wrong about that.
Why would it be easier to let doExecutePayment() return the result? executePayment() is the API method and the default implementation already takes care of returning the result. As illustrated by \Drupal\payment\Controller\CompletePayment we may need the result in other places, which is why we need a dedicated method.

Also, why wrap the Response object in a PaymentExecutionResult? The idea of having such an object seems useful, but why wrap it twice?

I did that, because responses are now also used by payment types. See \Drupal\payment\Plugin\Payment\Type\PaymentTypeInterface::getResumeContextResponse(). Can you suggest a way to make this simpler, without mixing things up too much?

Feedback from my IRC talk with @Berdir:

• Make \Drupal\payment\PaymentMethod\PaymentMethodBase::doExecutePayment() non-abstract, because not all payment methods will need it anymore.
• Save the payment in \Drupal\payment\PaymentMethod\PaymentMethodBase::executePayment().
• Merge \Drupal\payment\Response\Response and \Drupal\payment\Response\RedirectResponse, so calling code can simply pass on a URL that will be converted to a redirect response automatically.

Re-roll.

A method like that would return a URL if payment execution is still in progress, and NULL if it has been completed? That means we'll have two ways on the same class to get this information:

1. getPaymentExecutionResult() which returns an object with a method to check the status and that returns a boolean.
2. This method that returns a boolean in case execution is done, or an object (either Url or Response) in case it's not done.

I can't help but think this sounds rather inconsistent.

@berdir and I talked on IRC and decided will will discuss possible small improvements in follow-ups, so we can already start making use of the new approach.

Thanks @berdir and @donquixote for providing useful feedback!