Sylius: [Payum Bundle] Integration and extendability issues discussion

[psihius]

Hello,

DISCLAIMER

This is a discussion issue, as I can provide a proper PR in a short time, as I did around 80-90% of the actual implementation already for my project. What I want to know if this stuff is acceptable, is inline with the vision and has place in the Sylius core (I think yes, as this is pretty basic low-level functionality that if done on a per-project basis, overrides quite a few very important and sensitive places).

I had a fun week with implementing a fairly common thing with a custom gateway - a "Hold => Charge" pattern for credit cards, when you first reserve funds on the clients credit card and then when you confirm with the client that everything is in order, you can do the charge on the transaction during next 28 days (depends on the processing you are using, but our allows for 28 days of hold). And here is where I stumbled into very rigid, and very limited way, the Payum bundle is actually integrated.
Almost everything in the Sylius'es PayumBundle is declared as final - I had to completely replace hard coded class usage via "use" statements doing a whole file "copy-paste" just to change a single line:

GetStatus.php
PaymentInterface.php
PayumController.php
ResolveNextRouteAction.php
TransactCardProcessing.php
UpdatePaymentStateExtension.php

Right now the only workflow that is supported by Sylius is "Capture => Status => Notify". There is no "Authorize" support (this is what I used for my "hold" gateway action, as that's the closest think Payum has to a "hold" and it seemed other community gateway packages did the same, although I'm more or less sure we can add a dedicated "hold" status), there is no "Cancel" or "Refund" action support either - both are supported by the gateway I use and are required by the client to work.

Here is what I needed to just extend:

PaymentInterface
But I had to modify some of the classes to start using this new interface and some of them I had to hard copy-paste and do a use because they where final

namespace AppBundle\PayumOverride;

use Sylius\Component\Payment\Model\PaymentInterface as BasePaymentInterface;

interface PaymentInterface extends BasePaymentInterface
{

    const STATE_AUTHORIZED = 'authorized';
    const STATE_CHARGING = 'charging';
}

Here are per-file reasons, why I had to override them via copy-paste:

GetStatus overrides Sylius\Bundle\PayumBundle\Request\GetStatus
I need to do this just because I needed to change "markAuthorized" method

    public function markAuthorized()
    {
        $this->status = PaymentInterface::STATE_PROCESSING;
    }

to

    public function markAuthorized()
    {
        $this->status = PaymentInterface::STATE_AUTHORIZED;
    }

ResolveNextRouteAction to override Sylius\Bundle\PayumBundle\Action\ResolveNextRouteAction
To import the new PaymentInterface from the override folder and adjust the execute method's to include the OverridePaymentInterface::STATE_AUTHORIZED

if (in_array($payment->getState(), [PaymentInterface::STATE_COMPLETED, OverridePaymentInterface::STATE_AUTHORIZED])) {
            $request->setRouteName(
                'sylius_shop_order_thank_you'
            );

            return;
        }

PayumController to override Sylius\Bundle\PayumBundle\Controller\PayumController
This, actually, is done via changing the service config, but because PayumController is final, I had to copy-paste the whole thing.
This one I had to override so I could create a different type of token when needed, because otherwise it created only capture tokens.
The usage of $gatewayConfig['use_authorize'] is my solution right now so I don't have to hardcode an if into the code and do it via the gateway config param. My thinking that there has to be developed some way of knowing what workflows the gte

    public function prepareCaptureAction(Request $request, $tokenValue)
    {
        $configuration = $this->requestConfigurationFactory->create($this->orderMetadata, $request);

        /** @var OrderInterface $order */
        $order = $this->orderRepository->findOneByTokenValue($tokenValue);

        if (null === $order) {
            throw new NotFoundHttpException(sprintf('Order with token "%s" does not exist.', $tokenValue));
        }

        $request->getSession()->set('sylius_order_id', $order->getId());
        $options = $configuration->getParameters()->get('redirect');

        $payment = $order->getLastPayment(PaymentInterface::STATE_NEW);

        if (null === $payment) {
            $url = $this->router->generate('sylius_shop_order_thank_you');
            return new RedirectResponse($url);
        }

        $captureToken = $this->getTokenFactory()->createCaptureToken(
            $payment->getMethod()->getGatewayConfig()->getGatewayName(),
            $payment,
            isset($options['route']) ? $options['route'] : null,
            isset($options['parameters']) ? $options['parameters'] : []
        );

        $view = View::createRedirect($captureToken->getTargetUrl());

        return $this->viewHandler->handle($configuration, $view);
    }

to

public function prepareCaptureAction(Request $request, $tokenValue)
    {
        $configuration = $this->requestConfigurationFactory->create($this->orderMetadata, $request);

        /** @var OrderInterface $order */
        $order = $this->orderRepository->findOneByTokenValue($tokenValue);

        if (null === $order) {
            throw new NotFoundHttpException(sprintf('Order with token "%s" does not exist.', $tokenValue));
        }

        $request->getSession()->set('sylius_order_id', $order->getId());
        $options = $configuration->getParameters()->get('redirect');

        $payment = $order->getLastPayment(PaymentInterface::STATE_NEW);

        if (null === $payment) {
            $url = $this->router->generate('sylius_shop_order_thank_you');
            return new RedirectResponse($url);
        }

        $gatewayConfig = $payment->getMethod()->getGatewayConfig()->getConfig();

        if (isset($gatewayConfig['use_authorize']) && $gatewayConfig['use_authorize'] == true) {
            $token = $this->getTokenFactory()->createAuthorizeToken(
                $payment->getMethod()->getGatewayConfig()->getGatewayName(),
                $payment,
                isset($options['route'])
                    ? $options['route']
                    : null,
                isset($options['parameters'])
                    ? $options['parameters']
                    : []
            )
            ;
        } else {
            $token = $this->getTokenFactory()->createCaptureToken(
                $payment->getMethod()->getGatewayConfig()->getGatewayName(),
                $payment,
                isset($options['route'])
                    ? $options['route']
                    : null,
                isset($options['parameters'])
                    ? $options['parameters']
                    : []
            )
            ;
        }

        $view = View::createRedirect($token->getTargetUrl());

        return $this->viewHandler->handle($configuration, $view);
    }

UpdatePaymentStateExtension overrides Sylius\Bundle\PayumBundle\Extension\UpdatePaymentStateExtension
This is done to just override the injected via use GetStatus from one in the Sylius\Bundle\PayumBundle to one I overrided above to have the proper makrAuthorized() method.

Of course I also overridden payment and order state machine graphs to include the new states, but that part is actually just "follow the manual"

Thoughts?

[makasim]

Great work.

a "Hold => Charge" pattern

That's indeed a must have feature.

We can replace currently used capture term with more generic purchase. Introduce a purchase request and action (in terms of payum). Add use one or another strategy internally. In any case it must be supported out of the box.

[makasim]

In terms of Payum GetStatus request is extendable.

The valid case: One creates a new GetStatus with a markXXX and isXXX methods. The class extends the one from Payum (or Sylius). There is an action that supports this extended request.

[psihius]

The problem with GetStatus is not that it's not extendable - it is. It's the fact that in Sylius\PayumBundle\Controller\PayumController it is loaded via use, so it always uses the one from the bundle and to use your extended one, you have to override the controller completely due to it being final

[makasim]

I see. There must be a factory for it.

[gdecorbiac]

I agree with @psihius.
I had a very hard time debugging my custom gateway because of the GetStatus request. I think that the factory option for providing a custom GetStatus is a good solution.

Another thing that bothers me in Sylius payum integration is offline gateways. If I create an offline "Bank check" payment method, when the customer finalizes its order, payment should be marked as "pending" and not "new".

[gdecorbiac]

There is another thing I would like to discuss about payum integration :

I created a gateway for the paybox solution.
With this gateway, when a customer submit the payment form on the bank server, a request is sent by the bank server allowing me to check the payment status (I never rely on user action to check if payment is valid or not, I use IPN).
The actual payum integration is that if payment is not valid, a new payment is created for this order (I know that it allows the customer to retry payment from order page).
Let's imagine the given scenario :

• the customer submit the payment form (on bank server) and payment is not valid because of a typo
• an IPN is sent by the server and the payment is marked as failed and a new payment is created
• then the customer, without returning on my website, fill again the payment form and payment is valid
• an IPN is sent by the server and the payment is marked as completed (because I changed state machine allowing a failed payment to be completed)

The issue is that because Notify token was the same for the two IPN requests, I get two payments for the order : a completed one and a new one.

I resolved this by deleting any payment that has not been completed but I'm not completetly satisfied with this solution. (You can see my code here : https://github.com/gdecorbiac/SyliusPayboxBundle/blob/master/src/Action/StatusAction.php)

Any thought about it?

[psihius]

That is the regular Notify for the IPN, this works as expected. There are two routes for that - payum_notify_do and payum_notify_do_unsafe

[psihius]

@pjedrzejewski any comments on this? Or from other team members? I'd like to do a proper PR on this.

[gdecorbiac]

@psihius Sorry, I submit my first comment by error and was editing it when you replied

[psihius]

@gdecorbiac i'd say that sending a Notify when user has an error in his payment form is definetly not a correct thing on the payment services side. If I make a mistake in my cards number, it will send 10 fails via IPN?

[gdecorbiac]

@psihius yes that's right. But this behaviour is the one from paybox server. Not mine.
Maybe that in Notify action, I could handle only valid payments but then I won't know, in my shop backoffice, if the customer tried to pay the order

[psihius]

@gdecorbiac I think we can't really adapt the core that that specific case, but we can make it easier to extend the core and adjust the workflow for that specific case. Also, you could just add a new state for the paybox and set it when you get "failed", so it does not trigger the whole state machine stuff. If you payment is "success" in the end - then business as usual. The payments in the new state you would add can be processed via cron job and set to failed after, say, half a day - just trigger the "failed" state machine transition.

[gdecorbiac]

@psihius I think I will keep my implementation as is for now. I don't want to add cron jobs for something that should be handled by Sylius or Payum logic.

[psihius]

Tagging you guys because I don't remember (or it's not available for non-team members) how to tag the whole team @pjedrzejewski @lchrusciel @pamil

I have some free time coming up and I'd like to put some proper work into this, so I'd like to know your thoughts you may have on your own or what to do. This is going to require some major changes and testing, because I want to try and support full Payum capabilities, that means supporting various workflows, refunds, cancellations, fund reservation and subsequent manual and/or automatic charging and things like that.

[pjedrzejewski]

Hey @psihius, this seems like something that definitely has place in the core. Thanks for your time on researching this and if you are able to contribute a PR - that would be super important for the stable release. Here is my idea: let's do a brainstorm on Slack next week and see what kind of changes and BC breaks are needed to make it happen, then decide what we do and what can be skipped. I can organize the meeting. What do you think?

[psihius]

@pjedrzejewski Sounds what I wanted to do since I created this issue, so just let me know the time and day :) I'm UTC+2

[openconcept]

This is very important functionality for a number of reasons, not least legal ones in certain countries. For example, in Norway the order amount is reserved at the time of purchase but not charged until the actual time of shipment.

Cancel/Refund actions are obviously also important.

[pjedrzejewski]

8553 Fixes the most important part of the problem and we take a look at refunds other improvements post 1.0 release. A lot can be done without BC breaks. :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.