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

Naming question #4

Open
diimpp opened this issue Sep 11, 2019 · 4 comments
Open

Naming question #4

diimpp opened this issue Sep 11, 2019 · 4 comments

Comments

@diimpp
Copy link

diimpp commented Sep 11, 2019

Hi, just wanted to clarify few things.

Original Payum Stripe Checkout now offered by Stripe as Legacy Checkout (Charge API), new Stripe Checkout is a hosted solution (Session API), but at the same time they now offer PaymentIntent API with manual web flow, which is very similar to previous charge api in terms of user experience (Self hosted payment page).

What StripeDirectGatewayFactory and StripeJsGatewayFactory do -- I have no idea.

So what I think is StripeCheckoutGatewayFactory should become StripeLegacyCheckoutGatewayFactory with previous Charge API and stripe/stripe-php lock as in original.

New StripeCheckoutGatewayFactory should consist of Session API which will redirect to Stripe hosted page and your work should go into StripePaymentIntentGatewayFactory with sca and without.

I'm not sure about web (self hosted), android/ios differences, should they be in same GatewayFactory or not. Maybe that's what Direct and JS gateways are about.

What do you think?

By the way, I've tried out new implementation (Inside Sylius) and I'm having this issue.
A Card token may not be passed in as a PaymentMethod. Instead, use payment_method_data with type=card

My model is not constructed right, but I'm sure problem is either with my html template or related to Sylius.
Screenshot_2019-09-11 Screenshot

Anyway, my company decided to go with Session API and hosted checkout page, therefore I'll be developing it now. I've saw https://github.com/Combodo/CombodoPayumStripe using Session API, but their code is not idiomatic to Payum and it doesn't look like they plan to return changes to Payum. So maybe we will be able to support both flows inside your fork and eventually PR in master.

@lolmx
Copy link
Owner

lolmx commented Sep 12, 2019

Hello @diimpp !

About your error, I think you forgot to use the new js method to create a token front side
Stripe.createPaymentMethod('card', $form, stripeResponseHandler);

It replaces old one createToken.

About the different factories, I would need to deep further as I only really used myself the JS one, but it looks like a good idea.

Once we got a better idea of the difference then we can definitively support both flows in the fork. :)

@diimpp
Copy link
Author

diimpp commented Sep 12, 2019

I've pushed my work so far on Stripe Checkout Server flow into PR
#6

P.S. I've gained some insights since my last message, I will write tomorrow.
P.P.S. Check out new StatusAction especially, I've dropped legacy api support and wrote it only for new api format. I'm not 100% sure on all cases, but for basic success it works correctly

@diimpp
Copy link
Author

diimpp commented Sep 12, 2019

By the way, apparently 6.9 and 7.0 have bigger differences, than I've expected. For examle toArray definition.

Attempted to call an undefined method named "__toArray" of class "Stripe\Checkout\Session".
Did you mean to call "toArray"?

@lolmx
Copy link
Owner

lolmx commented Sep 21, 2019

Thanks for your pr, i'm looking at it 👍

I also checked differences between Stripe 6.9 and 7, reading their migration guide, there are two main issues regarding our usage:

  • __toArray() became toArray() like you said
  • all exceptions have been renamed
    Since it's a pretty new version we need to still support both version. Without thinking i see two ways to supports both versions, we can add a new config parameter to ask for used sdk version (i'm not fan) or add checks with class_exists and method_exists, what do you think about it?

I open a new issue regarding this since the fork will be unusable for anyone using stripe v7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants