Allow off_session payment intents for recurring payments

Hi I'm testing this patch.

One thing to note is that it looks like if the future usage is set to none on the checkout stage this produces an error on checkout.

Invalid setup_future_usage: must be one of on_session or off_session

I guess we should not set up future usage if this is set to none.

I've tested the new patch and it works for me.

The code looks straightforward too.

I have issues with anonymous users but I think that's outside the scope of this patch.

There is already an issue for anonymous recuring here https://www.drupal.org/project/commerce_recurring/issues/2940084

+1

Re-rolled #5 on top of #3171411-6: Optionally autosubmit the review pane to streamline UX

And with #3108793: Stripe\PaymentIntent instance has invalid ID, an intent is always created without taking into account if it should happen offsite. So the payment creation with the offsite parameter is never happening. I think we should merge both issues on this one, as we need to ensure that we are aware when the payment is aimed to be created from recurring. The only way I found of doing that is based on the order type, 'recurring'. The attached patch is still on top of #3171411-6: Optionally autosubmit the review pane to streamline UX.

Per conversation with @jonathanshaw, #3108793: Stripe\PaymentIntent instance has invalid ID shouldn't be needed, an intent is always created in the stripe review pane, so we can assume that if there is none, we are in an off_session scenario.
The attached patch is still on top of #3171411-6: Optionally autosubmit the review pane to streamline UX, but might apply with HEAD.

For something like #3172234: Allow to retry payments after failure, including (re)authentication for offsession payments (SCA) to be worked, and already suggested in #12, we need the payment created by commerce_recurring when we are creating a payment intent if that exists. So adding that as a third argument here.

Added tests for that. As these tests are calling the API for real, we do not really know what to expect, so we need a hack to assert if it was called with the right params. I'm (ab)using metadata for that in the test.

The attached patch is still on top of #3171411-6: Optionally autosubmit the review pane to streamline UX.

Here's the patch in #17 re-rolled on HEAD, not on top of #3171411-6.

For those interested in anonymous recurring: this is necessary, but not yet sufficient. It does not set up a Stripe customer.

On a wider note, I think that the future payment intent should be coming from two sources - the order items, and the setting for saving payments, which comes from the admin, or the customer. This patch does not do this, instead it uses a checkout setting.

Anyway, let's see if it applies.

I've been thinking more about this...

I think although the Stripe gateway, and other gateways, are the right place to implement this behaviour, but that the decision about whether a store needs customer-not-present transactions is a sort of policy decision, and the setting belongs at Commerce or Recurring level, or any other product-type module that needs customer-not-present payment. (More on that below.)

And that hints that I think that off-session is a card-related technical term for 'ability to process transactions with the customer not present'. An invoice module has this ability. It's the ability to bill for later payment. And I think that this is best encoded as supporting an interface.

I think that there should probably be a setting at Commerce or Recurring level for 'Require ability to process future transactions with the customer not present'. If set to yes/true, then card payment modules should be setting up future intent to be off-session. And if this setting is switched on, then only payment methods that support the interface should be listed.

There's that patch coming down the Commerce runway, https://www.drupal.org/project/commerce/issues/2871483, which allows admins to set whether to store payment methods. It has the settings for always, never or let the customer choose. These settings are for customer-present/on-session transactions. Meanwhile this patch is the counterpart for off-session; imo it should be dependent on the 483 patch - you can only ask for off-session if you also have the ability to make future charges.

So I think the ideal thing would be to split this patch into two - one with the policy decisions - settings - at Commerce level, probably. And the rest in Stripe (and other gateways.)

I think where this setting sits is worth discussion: it could be attached to products - e.g. Commerce Recurring allows for postpaid subscriptions and trials, so it could be specifically on those products, without being on other products. Or you can imagine it as a setting for all Commerce Recurring products; customers won't be sold a subscription unless there is a way to bill in the future*. (Different modules would implement different mechanisms for that ability - e.g. Invoice might just require an email address.) Or you can imagine it at Store level. Or you can imagine it as an a module which is shared between Commerce Recurring and any other Commerce modules which support future customer-not-present.

One other issue - whether to support anonymous customer not present. E.g. a subscription which is invoiced monthly, but the customer has no account on Drupal. Subscription stops if the customer does not pay the invoice.

* I think a recurring product always needs at least a way to contact a customer about payment. I don't think it actually needs a payment method, though almost all stores will want one. So we could just say Commerce Recurring does not support transactions where there is no payment method provided while the customer is present. I think that would preclude postpaid transactions without taking payment details while the customer is present.

Sorry about the essay. It's been going round and round my head.

Just to add I forgot about the setting for "Collect payment methods on free orders" which I think is part of https://www.drupal.org/project/commerce/issues/2871483. Potential interaction.

@jonathanshaw, thank you so much for your patch at #5!

After several years of trying to get Stripe and Recurring to play together, no more invalid id and payments go through for cards with No authentication (U.S. card): 4242 4242 4242 4242.

===

But run into a new issue, test cards with Authentication required: 4000 0027 6000 3184 get declined.

1. Stripe notice: The card was declined as the transaction requires authentication
2. Website the order stays active with: Payment required

Jonathan, is there perhaps a solution to this please? So close to making this work.

Most appreciated 🌿

@JonathanShaw, thank you for your response.

So I have now tried a couple of hours, still getting ‘card was declined. This transaction requires authentication’ on a card that needs authentication.

Steps:

1. Disabled ‘review pane’, kept ‘stripe review pane’
2. Setup future storage: off session

Orders show ‘needs payment’ and subscription is still active

In Stripe (test mode), ‘payment declined by bank’.

===

See in attached files the Stripe module that includes your commits from #5 😁

===

Using a card with no authentication required works perfectly.

Oh, so close! Would immensely appreciate your guidance 🌷

Oh wow, thank you Jonathan 🌷

Johnathan may be right, but I think you've hit a bug; I've spent the last week and a half looking at the Stripe module. I'll post some reports and patches today or tomorrow, but I think you have almost certainly hit a bug.

Jonathan - because I've been working on a whole lot of the Stripe problems together, plus I have a stack of the patches installed, I don't have a nice clean series of patches. What's the best way to tackle this so that other people benefit from this work too?

Oh yes please @Jonathan and @Jeff, a copy of the patched module would be appreciated, it’s a minefield out there 😊 Most most appreciate both of you for getting Stripe to work 🌷

Using patch from #18 successfully allows me to create payment intents for recurring automatic billing (and avoiding the issue "Stripe\PaymentIntent instance has invalid ID")

In my use case, I'll always ask for off session payment intents as the customer might reuse one of his card for buying subscriptions or recurrent products. Thanks @Jeff Veit for the patch and @jonathanshaw for all your amazing documentation work.

What's left to be done here ?

This needs a reroll following the changes in recent commits.

here is the patch #18 for last commits

We would like to use this functionality and it's great to hear this patch has been working on prod per #35.

While I understand it would be good to have this functionality well integrated into Commerce Stripe it seems this needs deep changes in the code, as it can be seen following the issues in #36. This means that the functionality may need months or even years to complete.

I like this issue's approach: "requires no serious alteration to the existing architecture". I would suggest maintaining this patch alive and working until a proper solution is developed. We plan to use it and we can update it if any issues are detected.

Ok, thanks for the information in #38. Because there were different issues related I thought the second approach (the good one) had progressed not so much.

We'll try to test that second approach so we can help to push it forward. Thanks!

The most recent patch here still applies but now introduces some regressions into current dev Stripe::createPaymentIntent(). I'll try to take a look at re-rolling at splitting it up as per #38 in the next day or two.

This patch applies on top of #3259211: Provide more parameters to createPaymentIntent() and #3259349: Allow for payments without stripe review pane.

And this one includes those patches.

Test failures are the same as for #3259211: Provide more parameters to createPaymentIntent(), good. Setting back to postponed as per #36.

reroll #34 patch against latest dev

#46 file contained wrong patch

@skyredwang if you want this issue to move forward, please also provide a patch that applies on top of #3259211: Provide more parameters to createPaymentIntent().

That will be the way forward, as per comment #38.

Updated #3171408-41: Allow off_session payment intents for recurring payments so it applies again.

@ac Thank you.

Um hang on. The last commit was 27 Apr, and #41 applied on top of that plus the other two patches mentioned, so I'm not sure why it needed a reroll? @ac Did you not apply the two required patches first? I think you may have just created another combo patch.

Note #41 requires the patches from #3259211: Provide more parameters to createPaymentIntent() and #3259349: Allow for payments without stripe review pane, all applied against latest dev commit.

A number of issues now depend on #3259211: Provide more parameters to createPaymentIntent() getting committed, including this issue.

#41 no longer applied due to commits on #3259211: Provide more parameters to createPaymentIntent() (latest 2nd May 2022)

Ah, gotcha, I was thinking you meant latest dev.

I've just marked #3259211: Provide more parameters to createPaymentIntent() RTBC and reached out to a maintainer to see if we can get it committed. If not, I guess we need to reroll on top of both patches, since #3259349: Allow for payments without stripe review pane was split out of this issue as well. But let's see if we can get a commit.

I did leave an extra line in my reroll though. Attached fixes it.

#42 is still the patch to use if you want a combo patch. The failing tests are related to failing tests in an earlier version of the included #3259211 and do not prevent it applying, since dev hasn't changed.

I may be able to reroll this in the next day or so.

#3259211: Provide more parameters to createPaymentIntent() is committed so this can be unpostponed.

Rerolled the patch against last 8.x.1.x-@dev
i did it kind of blinded, didn't try to understand what it is doing, just removed from #42 what was already there

Re-rolled the patch against dev branch

Will attempt a reroll.

@riabovol: Regarding the MRs from #61-63, I don't see @Bojan's comments anywhere in this thread.

Let's see how this goes.

Hmm. Think I rerolled based on the wrong patch, it should be #54. MR18 is unrelated, and not sure about MR17. This is a mess to follow. Revisiting.

This patch should pass, it does locally.

One addition to PaymentIntentTest::testCreatePaymentIntent() - if we are testing the dataset with a passed-in payment generated, the prophesized payment needs to return a valid Price for PaymentInterface::getAmount(), to avoid a MinorUnitsConverter::toMinorUnits() error.

I've been trying to get commerce_stripe going with recurring payments for a few weeks now while in Stripe test mode.
commerce_recurring 8.x-1.0-rc1
commerce_stripe 8.x-1.0-rc8
commerce_license 8.x-2.0-beta2

I've applied the patch at #72 and set my future usage to off session in the Stripe Review pane. I still get a fail on the first recurring attempt.

RuntimeException: Failed to start the session because headers have already been sent by "/vendor/symfony/http-foundation/Response.php" at line 1239. in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() (line 152 of /vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php).

This is different from the one I got prior to the patch, which was:

/html/core/modules/big_pipe/src/Render/BigPipe.php" at line 264. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 815 of /html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php)

Will keep testing and see what I come up with.

@kent@passingphase.nz - If you remove commerce_license do recurring payments work? Are any other patches applied against commerce, commerce_recurring, advancedqueue or commerce_stripe?

Commerce license is an essential part of the recurring payment existence, in that these are subscriptions, for which is a license is assigned. If I have no luck I will try removing commerce license as a test.

The only other patch applied is to commerce, as follows: https://www.drupal.org/files/issues/2021-05-12/commerce-send_mail_after_... , which involves sending an email when purchasing and registering at the same time.

I do notice, however, that the error occurs when I am browsing the site at the same time that the recurring order attempts to process the payment. For instance, attached is a screenshot of the dblog error. At the time that this attempt was made via cron, I was looking at the admin/reports/fields page. It appears that I am interfering with the payment process while navigating the site. Weird.

Hm. How is cron being triggered? The built-in automatic cron, or an external cron run? So we don't junk up this RTBC issue any further, maybe this warrants a separate issue, probably in either commerce_recurring or commerce_license.

Thanks, @John Pitcairn. I'll try running cron from crontab, and post this issue on a suitable module elsewhere.

Trusting you guys on this, committed, thanks!