Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Payment Intents gateway. #128

Merged
merged 5 commits into from Jul 18, 2019

Conversation

@andris-sevcenko
Copy link
Contributor

andris-sevcenko commented Jul 16, 2019

Add support for the SCA-ready Payment Intents API by adding a new gateway.

This PR resolves #122, resolves #118 ,resolves #79.

Add support for the SCA-ready Payment Intents API by adding a new gatewa.
@andris-sevcenko andris-sevcenko marked this pull request as ready for review Jul 16, 2019
@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 16, 2019

I have no idea why Travis is failing on the 3 response mocks in one specific environment.

Hopefully, @barryvdh or someone with more experience in that area can help me out there or I can figure it out eventually. The review process can begin, anyway.

@m2de

This comment has been minimized.

Copy link

m2de commented Jul 18, 2019

Hi @andris-sevcenko . That looks pretty comprehensive. Could you maybe summarise what the difference / changes when using payment intents compared to a previous charge method? E.g. what is the typical / minimal process to actually use this? I have got some time (in the next few days) to have a go at implementing this in one of our projects to try it out and this would certainly help me understand the implementation process better, which I'm sure would also be helpful to others.

My current workflow is to attain a token from Stripe (using Stripe.js) and then passing this token into a OmniPay Stripe request.

// Something like ...

$parameters = [
        'amount' => $this->amount,
        'currency' => $this->currency,
        'token' => $this->token,
]

Omnipay::create('Stripe')->setApiKey($key)->purchase($parameters)->send()

Thanks again for your work on this!

@m2de

This comment has been minimized.

Copy link

m2de commented Jul 18, 2019

Sorry, realise you have already done this on the issue #122 (comment). I will have another look through this and see if I can make things work my end. Cheers.

@barryvdh

This comment has been minimized.

Copy link
Member

barryvdh commented Jul 18, 2019

@m2de can you give this a try?

Is this ready for merging?

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 18, 2019

@m2de To better follow the Omnipay conventions, I added completePurchase and completeAuthorize as well which you can use in step 7 as aliases for the whole process.

So, really, if your site has a homogenized flow for both Credit Card and Offsite gateways, the only caveats are:

  1. If you're creating a payment method (as per https://stripe.com/docs/payments/payment-intents/quickstart#collect-payment-method), then you should use the paymentMethod parameter on the purchase instead of token. However, creating a token (the same way you would for the Charge API) requires you use the token parameter. Payment Method is recommended, but, at least for now, the token works as well.
  2. If you're authorizing, instead of automatically capturing, when capturing you must set the paymentIntentReference parameter.
  3. When refunding, though, you must still use the transactionReference parameter, which must refer to a charge.
  4. To re-iterate over (2) and (3) - you capture the payment intent, but you refund one of the charges that a captured payment intent contains.

Other than that, it all is pretty straightforward:

  • Attempt to pay using the token (setting confirm to true)
  • If the response is successful, you're done.
  • If the response is a redirect, just set the returnUrl to the same URL you would use for off-site gateways that require to fire completePurchase/Authorize().
  • Fire completePurchase/Authorize() and, if successful, you're done. Otherwise return to step 1.

FWIW, I tested it using Craft Commerce and it worked for me. Obviously, it would be ideal to get at least one confirmation in the field.

@barryvdh

This comment has been minimized.

Copy link
Member

barryvdh commented Jul 18, 2019

Could you update the readme to provide some example/explanation?

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 18, 2019

@barryvdh I'll try to find some time for that today.

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 18, 2019

@m2de

This comment has been minimized.

Copy link

m2de commented Jul 18, 2019

Had a skim, looks perfect in terms of instructions. Not sure I will get to testing this today but will try. Otherwise will definitely have time tomorrow. I'll report back then. Cheers!

@barryvdh barryvdh merged commit de12e27 into thephpleague:master Jul 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@barryvdh

This comment has been minimized.

Copy link
Member

barryvdh commented Jul 18, 2019

Merged this for now, also updated the branch alias to 3.1, so you should be able to test this with 3.1@dev

@maxnet

This comment has been minimized.

Copy link

maxnet commented Jul 18, 2019

If the response is a redirect, just set the returnUrl to the same URL you would use for off-site gateways
that require to fire completePurchase/Authorize().

What if the user does complete the payment, but is not redirected back to returnUrl for some reason?
(Recall there were banks in the past that did not redirect automatically, but showed some confirmation page first, and required user to actively click a link to return to shop)
Aren't you supposed to also implement Stripe's webhooks to catch that?

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 18, 2019

What if the user does complete the payment, but is not redirected back to returnUrl for some reason?
(Recall there were banks in the past that did not redirect automatically, but showed some confirmation page first, and required user to actively click a link to return to shop)
Aren't you supposed to also implement Stripe's webhooks to catch that?

That means that you never call the final $gateway->completePurchase() and the card never gets charged and the order never gets completed. In that case, clicking the link should be redirecting the user to the intended page.

@maxnet

This comment has been minimized.

Copy link

maxnet commented Jul 18, 2019

That means that you never call the final $gateway->completePurchase() and the card never gets charged
and the order never gets completed.

Ah.
That indeed may work for now for credit cards.

Do note that this still is likely to give problems for the other payment methods that Stripe plans to add to intent soonish?
I suspect that with other payment methods like iDeal, the money is collected from the customer, regardless if completePurchase() is called, since that does not support the concept of only authorizing first...

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 18, 2019

I suspect that with other payment methods like iDeal, the money is collected from the customer, regardless if completePurchase() is called, since that does not support the concept of only authorizing first...

If the current iDeal flow for the Charge API is any indicator, where you still "prime" the transfer on the client end and then charge it from your server (https://stripe.com/docs/sources/ideal#charge-request), the flow is pretty much the same as 3DS authentication, in which case you "prime" your card and then charge it from your server.

3DS using Charge API also required webhooks, but they no longer do, when using the manual flow (which this PR does).

As far as the grander argument "we should have webhooks" stands, I completely agree and I have nothing against other people pitching in.

This PR is so that people have a way of ensuring their stores don't shut down on Sept 14th, when the SCA deadline kicks in. Other bridges can be crossed when we get there.

@maxnet

This comment has been minimized.

Copy link

maxnet commented Jul 18, 2019

If the current iDeal flow for the Charge API is any indicator, where you still "prime" the transfer on the
client end and then charge it from your server (https://stripe.com/docs/sources/ideal#charge-request)

Ah, seems Stripe just refunds the payment back to the client's bank account in that case, to workaround that payment methods like iDeal do not natively understand the concept of authorization.

This PR is so that people have a way of ensuring their stores don't shut down on Sept 14th, when the SCA deadline kicks in.

Deadline is likely to be extended.

Other bridges can be crossed when we get there.

I understand your stance, and appreciate your work.
But if I need to make changes to my payment code I likely do so in one go, instead of make changes now, and making more changes later.

@domis86

This comment has been minimized.

Copy link

domis86 commented Jul 18, 2019

Guys - about problem of handling "non-credit-card payment methods" (described as "Planned" in https://stripe.com/docs/payments/payment-methods#transitioning ) i contacted Stripe support directly. Here i paste our conversation:

Jim
Stripe Support

me:
hi

Jim has joined.
Hey Dominik.
Jim here at your disposal, sir.
How can I help you today?

me:
Im a developer and our company is using Stripe. I want to ask about payment methods that are not yet supported via Payment Intents ( https://stripe.com/docs/payments/payment-methods?origin_team=TCGBDV0MA#transitioning ) In particular: iDeal, SEPA and SOFORT
1. If we currently use said payment methods what should we do if we migrate to "Payment Intents" (because of PSD2 regulation) ? Only option is to disable them until they are supported by Stripe Payment Intents?
2. When you plan to add support for those payment methods via Payment Intents?

J:
Hmmm, PI is only really card based, I don't think it's applicable to iDeal, SEPA or SOFORT, they'll go as normal for the time being. You wouldn't need to disable them. As for an ETA on when they'll be under the paymentintents system, that I'm not sure of. We'll be announcing more as time goes on.

me:
ok, so you sugegsting to use the "old Sources/Charges" way for iDeal, SEPA and SOFORT and for the credit cards we should use "new Payment Intents" ?

J
Absolutely, Dominik, sir.

me:
ok, we will try to do like this then
thank you

I then also went to Stripe IRC channel, asked same question and they responded similarily.
So, as you see above, in our applications most likely we need to use:

  • "new flow" using PaymentIntents for credit cards
  • "old flow" using Sources/Charges for payment methods like iDeal
@andris-sevcenko andris-sevcenko deleted the pixelandtonic:feature/payment-intents branch Jul 18, 2019
@m2de

This comment has been minimized.

Copy link

m2de commented Jul 19, 2019

Just a quick update on testing. I have managed to find a bit of time to test today but so far it hasn't been quite as straight forward as expected. Have made some progress and will provide a full update next week, possibly with some additions to the readme and maybe even a PR to add backwards compatibility with previous Stripe API version which use slightly different naming conventions.

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 19, 2019

I have managed to find a bit of time to test today but so far it hasn't been quite as straight forward as expected.

Curious to see what roadblock you hit.

@m2de

This comment has been minimized.

Copy link

m2de commented Jul 23, 2019

Thanks again @andris-sevcenko . Have managed to do a full test implementation now and all seems to be working. Here are some of the challenges I faced during implementation in case anyone else comes across this or you want to add to the readme ...

  • Previously I would initialise with Omnipay::create('Stripe'). I didn't realise this needed to be Omnipay::create('Stripe\PaymentIntents') now.
  • On the client-side I had to switch from createToken to createPaymentMethod. I could not get things to work with a token (possibly due to other issues, unsure)
  • Because our implementation still uses an earlier version of the API, the isRedirect() method did not work because Stripe returns requires_source_action instead of the expected requires_action.

Any other issues I had were more around our implementation, so all in all I would suggest this works as expected and documented.

@sfarkas1988

This comment has been minimized.

Copy link
Contributor

sfarkas1988 commented Jul 25, 2019

Any plans when this can be released?

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Jul 30, 2019

Any plans when this can be released?

No idea. @barryvdh?

6) Now we have to confirm the payment intent, to signal Stripe that everything is under control.

```php
$response = $gateway->confirm([

This comment has been minimized.

Copy link
@sfarkas1988

sfarkas1988 Jul 30, 2019

Contributor

The method is throwing an exception when cancelling the transaction inside the stripe webview for 3d secure:

Client error: POST https://api.stripe.com/v1/payment_intents/.../confirm resulted in a 400 Bad Request response:
{
"error": {
"code": "payment_intent_unexpected_state",
"doc_url": "https://stripe.com/docs/error-codes/paymen (truncated...)

The workflow should be mentioned here as well maybe?

}
$data['confirmation_method'] = 'manual';
$data['capture_method'] = 'manual';

This comment has been minimized.

Copy link
@sfarkas1988

sfarkas1988 Aug 9, 2019

Contributor

@andris-sevcenko Any reason why I can't set the capture_method to automatic like documented here:
https://stripe.com/docs/api/payment_intents/create#create_payment_intent-capture_method

Right now I'm missing a way to capture my done payment for PaymentIntent gateway

This comment has been minimized.

Copy link
@andris-sevcenko

andris-sevcenko Aug 9, 2019

Author Contributor

You can call the completePayment() method to complete the payment as that's how it's done for offsite Omnipay gateways.

Setting automatic didn't make much sense at the time, because automatic payment intents can be confirmed only using Stripe.js.

This PR by no means was perfect, but probably the best way to address its shortcomings is to open a PR that improves them :)

This comment has been minimized.

Copy link
@domis86

domis86 Aug 9, 2019

Setting automatic didn't make much sense at the time, because automatic payment intents can be confirmed only using Stripe.js.

Exactly - also AFAIK for the "automatic" you need to handle asynchronous Stripe Webhook

See:
https://stripe.com/docs/payments/payment-intents/migration/automatic-confirmation#saving-cards-checkout-step-4

Automatic confirmation is recommended if you are already using webhooks with your existing payments integration.
If you are migrating from an existing cards and Charges API integration, we recommend using manual confirmation, which is the fastest way to migrate.

vs:
https://stripe.com/docs/payments/payment-intents/migration

This comment has been minimized.

Copy link
@sfarkas1988

sfarkas1988 Aug 9, 2019

Contributor

ok, convinced! :)
But the readme file should explain that capture has to be called in my point of view.
After confirmation I do call now \Omnipay\Stripe\Message\PaymentIntents\CaptureRequest.

completePayment() is also not mentioned, only confirm(), but honestly I didn't check the difference. :)

@sharald

This comment has been minimized.

Copy link
Contributor

sharald commented Aug 14, 2019

Thanks for a great job, @andris-sevcenko.

Would you consider implementing Omnipay\Common\Message\RedirectResponseInterface in Omnipay\Stripe\Message\PaymentIntents\Response? All the required methods are implemented in AbstractResponse, so it is a matter of adding implements RedirectResponseInterface in the right place.

We have tested this PR in a Laravel-Payum app with payum/omnipay-v3-bridge, and the only remaining issue is that the bridge complains that $response->isRedirect() returns true, but the response instance is not RedirectResponseInterface.

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Aug 15, 2019

@sharald ah darn, I missed that it should be implemented. Sorry about that!

This PR is merged already, so you can probably just open a new one and add the functionality - it's pretty straightforward. I'm a little bit overwhelmed with other things at the moment to be able to look at it, sorry :/

@momic

This comment has been minimized.

Create payment request does not work because type parameter is required beside card. It works with $data initialization changed to:
$data = ['type' => 'card'];
https://stripe.com/docs/api/payment_methods/create

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko replied Aug 19, 2019

Ah, right. Since this PR is already merged, can you open a new PR to address this?

@btb-dimitar

This comment has been minimized.

Copy link

btb-dimitar commented Aug 19, 2019

Will this be ported to Omnipay 2?

@jeremyquinton

This comment has been minimized.

Copy link

jeremyquinton commented Aug 30, 2019

@andris-sevcenko @m2de

When testing this I had an issue with the status in the response returned from Stripe.

Its discussed in the review of the PR here
https://github.com/thephpleague/omnipay-stripe/pull/128/files#diff-63de4953a2a51a02f016189d14ee33edR128

The fact that you get different responses from Stripe dependent on the version you select in your dashboard means this library only handles a certain version of the Stripe Api correctly.

Is it not possible to change the way the response is handled

if ($this->getStatus() === 'requires_action') { 

to

if ($this->getStatus() === 'requires_action' || $this->getStatus() === 'requires_source_action') {

If the change above can't be made I think the documentation could at least make mention that the response is different based on the stripe version selected in the dashboard and this library is only compatible with that version.

This had be stumped for hours.

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Aug 30, 2019

@jeremyquinton The change probably can be made without any unwanted effects, but this PR is closed, so the way to make that change would be to make a new PR :)

@jeremyquinton

This comment has been minimized.

Copy link

jeremyquinton commented Aug 30, 2019

@andris-sevcenko I will create the PR over the week-end :-)

@barryvdh any idea when a new tag will be created?

@andris-sevcenko

This comment has been minimized.

Copy link
Contributor Author

andris-sevcenko commented Aug 30, 2019

@jeremyquinton much appreciated :)

@sharald

This comment has been minimized.

Copy link
Contributor

sharald commented Aug 30, 2019

@jeremyquinton That PR already exists: #142

@jeremyquinton

This comment has been minimized.

Copy link

jeremyquinton commented Aug 30, 2019

@sharald thanks for letting me know.

@barryvdh can we get #142 merged?

@gmoreira

This comment has been minimized.

@andris-sevcenko I discussed this with Stripe Support today and they informed me that while return_url is only available when confirm=true, it is not mandatory to specify. The documentation is actually not clear about this, if its required or optional when confirm=true, which they confirmed for me today is optional.

What I have found is that when confirm=true and a return_url is specified, this is triggering the Manually handling 3D Secure authentication with redirect workflow, which requires the browser client code to manually handle redirecting the the redirect url provided for the PaymentIntent.

If the return_url is actually not specified and confirm=true, the PaymentIntent response can contain a client_secret that (if applicable) is actually compatible with Dynamic 3D Secure workflow which uses the pre-built modal inside Stripe Elements to handle the process without manual redirects via stripe.handleCardAction.

This comment has been minimized.

Copy link

domis86 replied Sep 17, 2019

@gmoreira You are right - we started discussion about it already in following issue: #152 (comment) - can you add your comment there?

This comment has been minimized.

Copy link

gmoreira replied Sep 17, 2019

Should have looked there first! :) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.