Payment Integration about sylius HOT 34 CLOSED

Hey @dyjo!

Thanks for the good words, it's really motivating! :)
As I stated in the blog post on Sylius.com - the payment gateway handling is our next step.
We're 100% sure we'll use Omnipay for this - so yeah, the integration is a work in progress - feel free to share any ideas about that - I'd be very happy to hear them and discuss.

I'd love to handle several different flows for the payment out of the box, but to keep it flexible at the same time.
Handle standard CC info, Transparent Redirect etc. but it requires some work. :)

We can perhaps rename this issue to Omnipay integration and discuss everything here, what do you think?

Thanks for jumping in!

from sylius.

Narrowing it down to Omnipay Integration helps. My original question was about a checklist of items that would create a working deployment, so I've tried to create one here. I've also tried to keep the descriptions as simple as possible, to prevent incorrect assumptions about the intended architecture. Just tell me if any corrections should be made, but I think maintaining this list will be useful as a roadmap for end-users or new contributors.

• Integrate a CreditCard Entity to SyliusCoreBundle.
• Map an Association between CreditCard and Payment.
• Add a CreditCardType form type.
• Add the CreditCardType to the PaymentStepType of the CheckoutProcess.
• Add validation for the CreditCard Entity.
• Map an Association between Order and Payment.
• Integrate submission and capturing of payments to the PaymentStep of the CheckoutProcess.
• Persist captured payments to the database.

I'll be posting my suggestions for these items in separate comments, and hope others do as well.

from sylius.

Would be great if you can help Sylius move forward on these tasks!

from sylius.

Why do you want to integrate a CreditCard entity to the CoreBundle? There is already one in PaymentBundle, it is even linked to the User class defined in config.

from sylius.

@winzou Thanks for pointing that out; I've been having trouble understanding how the DI occurs. I see that credit_card_owner is defined in the config, but I didn't see the credit_card model defined, and I didn't see a default defined in the DI Configuration.

from sylius.

Everything is in PaymentBundle/Entity, and the doctrine mapping in PaymentBundle/Resources/config/doctrine. There is also the relation between CreditCard and Payment.

You can tick the first 2 items ;)

from sylius.

@winzou I've looked at both of those, but was still confused: how do i access the credit card entity? is it available in the service container as 'sylius.model.credit_card.class'? Because I couldn't find where the default was set to Sylius\Bundle\PaymentsBundle\Entity\CreditCard.

from sylius.

Does anyone have already use JMSPayment bundles?

@pjedrzejewski wouldn't be a better thing to rely on JMS' bundles as this is a quite complex feature? It seems that JMS' bundles are on an advanced stage, it would save Sylius a lot of effort. Have you ever thought about that?

from sylius.

@winzou I found Sylius when I was looking for a more full-featured alternative to JMSPayment. The big value added for me is integration with Omnipay, so there's an abstracted method for submitting and capturing payments, and you don't have to define a new provider every time you want to use a new Gateway.

from sylius.

Do you think this is possible to integrate omnipay in a JMsPaymentCore plugin? It's a real question.

from sylius.

@winzou I'm sure it's possible, though I don't know why you would. It seems like the JMSPaymentCore GatewayPlugin duplicates a lot of what Omnipay does. At any rate, it don't know that it really relates to the topic of the issue, which is how does one bridge the gaps that exist in SyliusPayments and SyliusOmnipay

from sylius.

@winzou also a real question: #48 (comment). Figuring that out would be of great help for me, and hopefully flick the switch that helps me start contributing.

from sylius.

Ok, so my proposals for checking off the boxes on the task list. Please keep in mind this is all messy, just ideas not working code. But I want to get feedback before writing a bunch of stuff that doesn't make sense to maintainers.

Integrate a CreditCard Entity to SyliusCoreBundle

1. Make Sylius\Bundle\PaymentsBundle\Entity\CreditCard a mapped-superclass, and extend it in the CoreBundle. This follows the same conventions as all entities defined in /Sylius/app/config/sylius.yml as model option. NOTE: may be covered per #48 (comment) above. I could not find where the CreditCard class had been passed into the service container.

Add a CreditCardType form type

1. Create CreditCardType in SyliusPaymentsBundle with dataClass of CreditCard.
2. Add validation rules for CreditCard

Map an Association between Order and Payment

1. Add a bidirectional one-to-one to both entities in the CoreBundle.
2. Make it nullable on Payment because it will need to be constructed and persisted before Order is built in finalize step; and not all payments necessarily come from an order. This could probably be handled better.

Map an Association between CreditCard and Payment
This one is complicated. Differentiating and defining relationships between Payment, Payment Method, Gateway, Payment Source, and Transaction is my biggest source of confusion right now. The approach outlined below allows for splitting sources of payments, e.g. in a situation where someone pays part of a bill with a credit card and part with a check, or when a bill is split amongst multiple people, like in a restaurant. It also clarifies and facilitates the most important aspect of a commerce codebase: payments!

1. Convert PaymentSourceInterface to TransactionSourceInterface.
2. Add a mapped $source property to Transaction.
3. Convert PaymentMethodInterface to TransactionMethodInterface and move $method property from Payment to Transaction; if the transaction is a record of exchanged payment, then it should have knowledge of the method of exchange.
4. Make $gateway nullable on TransactionMethod (or current PaymentMethod). Some transactions may occur outside of a gateway.
5. So in short, Source (e.g. Credit Card) -> Transaction -> Payment

Add the CreditCardType to the PaymentStepType of the CheckoutProcess

1. Create TransactionType in SyliusPaymentsBundle with dataClass of Transaction.
2. Use class of the $source property of the transaction to determine what form type to add to TransactionType, e.g. the CreditCardType type created above.

Integrate submission and capturing of payments to the PaymentStep of the CheckoutProcess

1. Create interface Sylius\Bundle\OmnipayBundle\Model\CreditCardInterface with a mapping method to convert any credit card entity to the Omnipay naming conventions.
2. Sylius\Bundle\CoreBundle\Entity\CreditCard ... implements CreditCardInterface
3. Add an obtain() method to TransactionMethodInterface which will be called to actually collect the exchange. Also allows for the differing use of logic between on-site and off-site gateways.
4. In forwardAction of Sylius\Bundle\CoreBundle\Checkout\Step\PaymentStep, after validation but before forward, add logic for confirming a payment has been captured.

For example:

    $omnipayCard = $form->get('creditCard')->mapToOmnipay();
    $omnipayGateway = $this->get('sylius.omnipay.gateway' . $this->getCurrentCart()->getPaymentMethod()->getGateway());
    $transaction = $omnipayGateway->purchase($omnipayCard);

Persist Captured Payments to Database

1. Before sending the user to the finalize step

Hopefully this is easy enough to follow, and hopefully others can help me understand the code that's already there to make this stuff happen. If any of these sounds reasonable, let me know and I'll get to work on a pull request.

from sylius.

Why do you want CreditCard entity to be in CoreBundle? It doesn't need any customization (a credit card is a credit card), so the entity can (and should) live in PaymentBundle. Like we do with many other entities. And it isn't available from the service container afaik.

I totally agree with all your following points. There is just one thing I want to raise: the use of credit card. In quite many cases I think end-users will choose to go through the bank interface: it means that the ecommerce website doesn't have to ask for and store the credit card information, avoiding some obligations. For us, it means that the payment process differs from one payment method to another, and we don't show the credit card form everytime.

from sylius.

I totally agree with winzou on the use of credit card. Storing credit card data is very very discouraged. If Sylius wants to store these kind of information, it should follow the PCI-DSS standard. Here is some documentation :

• PCI quick reference guide
• PCI DSS standard

I don't know if these requirements have been taken into consideration at the moment. But if not, we have to understand that :

1. this is mandatory
2. this can lead to quite a big amount of work :)

from sylius.

The actual use case for CreditCard entity is to store the name, 4 last digits and the token for gateways which support token billing. So the store does not store any sensitive data and customer can enter the info only once, and later just select his credit card from the list. There is no plan to save any cc information on our side, but of course it's up to developer what he'll do!

from sylius.

Re: #48 (comment)
...and other comments about where the credit card entity is made available in the core bundle. I just looked at the driver/doctrine/orm.xml file in Payments and saw that the default configuration occurs there. This solves a lot of my confusion regarding the handling of sources and credit cards. Accordingly, I've checked off the first item in the task list.

I've left the second item unchecked because I still don't see a concrete mapping between a payment and a credit card (not just association in schema, but forms and embedded forms) in the checkout flow.

@winzou re: #48 (comment).
I agree, if you look at my (admittedly long) set of suggestions, I propose abstracting the payment source so multiple sources can be supported (credit card, off-site transfer, electronic check, heck, even cash). I only mentioned credit card specifically because it is the only source available in Payments at the moment.

But, in the event a credit card is available as a payment source, persistence should definitely be available. Storing a token is very useful for recurring payments, storing the last 4 and a reference to the owner doesn't require PCI compliance, but does allow a user to select the method for a future payment. Further, someone using the bundle independently may want to store more info, and could develop PCI compliance.

from sylius.

Hello! I'd like to join in, plan to use Sylius for some jobs soon. I've been working on a pull request for credit card form type. I've run into a problem with where to get a list of supported brands/types. Different gateways support different brands depending on locale (https://stripe.com/us/help/faq#types-of-payments). So the question is how to get that list. Some ideas

• a config variable set by end user (seems most flexible but most prone to error)
• a Gateway method like getSupportedBrands(); (seems flawed because it's only for credit cards).
• a new class, maybe SourceTypeResolver that takes a locale, a source type (cc, ACH), and a gateway and spits out a list of supported brands.

In the meantime I'm just using the constants from Omnipay Credit Card class (https://github.com/adrianmacneil/omnipay/blob/master/src/Omnipay/Common/CreditCard.php)

from sylius.

I think this is up to end-users, they can choose to enable or not certain types of credit cards in their country.

from sylius.

Hey guys, thanks for all the input. 👍

Just to clarify confusion about Payment -> CreditCard association - it's there https://github.com/Sylius/SyliusPaymentsBundle/blob/master/Model/Payment.php#L63.

But there are no dedicated get/setters, you can set and get it via get|setSource methods.
Also, credit card is associated with User (which needs to implement CreditCardOwnerInterface.

This may be confusing as it's hidden behind Doctrine RTEL magic. See this cookbook entry.

This is how all Sylius relations work, so if you want to change one entity class (via configuration) there is no need to remap or redefine all entities. Relation are updated automatically.

This also works for CreditCard.user association, if you configure FOSUserBundle User entity class (or any other user entity you want) it will be associated with CC. I think this is sensible default.

Yes, Order should be associated with Payment. And I agree that Payments do not always have to come from Order, so let's keep the foreign key on Order side.

The plan was to create payment via PaymentFactoryInterface service, which would take Order & PaymentMethod as argument.

The PaymentSourceInterface is handly not only for CC, I also used it for recurring payments via PayPal, ELV, BankTransfer. Where each of these had a entity similar to CreditCard. holding some data and token for recurring billing.

@dyjo Thanks for your research, I fully agree that we should be able to set method/source per transaction, not only for payment.

My idea would be to remove Transaction model completely, and use offset payments like Spree does it.
Payment.offsets holds collection of Payment, so each Payment can (but does not need to) have a 1-level-tree structure. This solves the problem of renaming PaymentMethod to TransactionMethod, as each Payment already can hold the method source. All is described here.

@dyjo What do you think about handling this like Spree? We'd get rid of Transaction model, which otherwise would become very similar to Payment itself.

from sylius.

@pjedrzejewski Thanks for clearing all that up! The way Spree handles this looks perfect; certainly clears up the confusion about what is a transaction and what is a payment.

I'm not too familiar with Doctrine Extensions; I have worked with multi-level trees but not 1-level trees. Is there any special configuration required to limit a tree to a single level?

Additionally, curious about integrating two things you mentioned:

1. recurring payments
2. BankTransfer

Is there any plan to incorporate these into the payments bundle? They would be very helpful for standardizing transactions. I know that Omnipay has "off-site" gateways, so that should make using PayPal easy enough, but I haven't seen anything about Bank Transfer, ACH, etc.

from sylius.

Actually, I think we do not really need to use DoctrineExtensions (we use it for taxonomies) for this. I think that Payment.parent -> Payment association will be enough. With reverse side on Payment.offsets -> many Payments. We can simply perform a check to disallow setting Payment as parent if it is already a "offset" (or child) payment. This way we'll limit it to just one level tree.

Actually I was using Adyen payments system directly (via SyliusPaymentsBundle models), not Omnipay. And I think that Omnipay currently does not support token billing (or maybe it's in dev).

Nevertheless, we can simply contribute to Omnipay with features we need!

Forgive me guys but I need a bit of clarification who is who haha! @dyjo @Eponymi @dylanjohnson? :D
I'm not sure who should I ping. I think the Spree solution is the way to go, I'm currently focused on products refactoring and Order & Cart duplication. I'd be more than happy to help you with a PR for these change though, feel free to post any questions you may have during work on this.

from sylius.

Fair confusion! I'm Carl, I'm the boss :) . I just hired Dylan a couple weeks ago as a contractor, and he's now working with us full-time. So he's been using this account to make comments sometimes, but I've asked him to use his personal account (thought it was @dyjo, don't know why he has @dylanjohnson) for comments/issues now. He does all the code writing through this account. So pinging this account is probably best way to go.

So in short: any comments/issues coming from this account will be me, any code writing is Dylan.

And @dyjo @dylanjohnson pick one personal account name!

from sylius.

Woops. I deleted the other one. But now I'm also Ghost (#48 (comment)). Sorry about that!

from sylius.

from sylius.

@dyjo My plan is to work hard to finish the 2 important things I mentioned (product + cart&order) by the end of this week. Then I can code the Payments stuff with you if you wish, but of course you can start whenever you want and I'll be happy to help/discuss/review a PR! That'd be awesome help. 👍 Thanks for pushing this important feature forward!

from sylius.

@pjedrzejewski I'll have a PR up on PaymentsBundle in the morning tomorrow following the Spree model, and I'll make sure to put in all specs. Hopefully that will be good enough to merge, and if so, I'll get the stuff together for CoreBundle tomorrow afternoon.

from sylius.

@dyjo That's great but don't set so hard deadlines for yourself. :) Take your time to implement it!

from sylius.

Fair enough, thanks for the help!

from sylius.

@pjedrzejewski Any ideas on sources that don't require a form? Off-site sources like PayPal for example. I have a couple ideas but wanted to get your thoughts.

1. on radio select a button to appears instead of a form.
2. instead of a radio button, if a method is off-site, a button for that method is rendered.

Number one would be quite a bit easier to implement with current code base. Number two seems like a much better UI/UX, but would require a lot more work and would mean that any off-site payment method would have to add a button view, or at the very least, an URL pointing to the gateway.

from sylius.

I think we should do number 1 in a first iteration, and then improve UI over time.

from sylius.

@dyjo @winzou I'd vote for number 1 too, let's do smaller steps, we have a lot to do in terms of UI anyway.

from sylius.

Ok, I've had a lot of trouble figuring out how to implement this abstraction. I'm going to defer to others on this integration and just use some hard-coding in the PaymentStep for the time being. Hopefully someone will be able to put something together for this, I'll be looking for it!

EDIT: If anyone is interested in seeing the route I was going, you can check here: https://github.com/Eponymi/SyliusPaymentsBundle/tree/payment_refactor_branch

from sylius.

Closed in favor of: #112

from sylius.