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

re-implement payum payment interface #2164

Closed
wants to merge 7 commits into from

Conversation

solverat
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets --

This PR re-implements the \Payum\Core\Model\PaymentInterface. Otherwise, the Payum Actions won't work anymore. Example:

https://github.com/karser/PayumSaferpay/blob/master/Action/ConvertPaymentAction.php#L57

@solverat
Copy link
Contributor Author

solverat commented Jan 3, 2023

Ok, I think there are more things to discuss, @dpfaffenbauer.

Two ways:

Way I

CoreShop creates a valid PayumPayment object here:

However, there is another issue if we're going to keep it that way. I'm using your unzer gateway to show you the problem:

  1. This action will fail because in the first try, it is not a payum model: https://github.com/coreshop/payum-unzer/blob/a00e9a441221ed3f48ee8213b177703e18c2a405/Action/ConvertPaymentAction.php#L52
  2. Now the catch() method is going to execute the convert action again, this time with an payum model
  3. But now, the extension will fail here, because it's not an coreshop payment interface anymore: https://github.com/coreshop/PayumUnzerBundle/blob/339f99d7bb3630f2edbc33f1b030828d04f18198/Extension/PopulateUnzerExtension.php#L50

My suggestion:

  • Remove the try / catch and use the payum model instantly
  • Instead of the raw payum model, we could create Core\Model\CoreShopPayumPayment with an $order property

Way II

Finish this PR (re-implementing the payum interface) and remove the try/catch block where you're creating a dedicated payum model

WDYT?

@dpfaffenbauer
Copy link
Member

@solverat here is what you can do with how CoreShop currently is designed:

  • Package 1: Payum Unzer: No Dependency to CoreShop
  • Package 2: Payum Unzer Bundle: CoreShop Integration of the Payment Provider

So as was before.

Then, in the Bundle, you create a ConvertCoreShopPaymentAction that looks something like this:

final class ConvertCoreShopPaymentAction implements ActionInterface
{
    public function execute($request)
    {
        RequestNotSupportedException::assertSupports($this, $request);

        /** @var CoreShopPaymentInterface $payment */
        $payment = $request->getSource();
    }

    public function supports($request)
    {
        return
            $request instanceof Convert &&
            $request->getSource() instanceof CoreShopPaymentInterface &&
            $request->getTo() == 'array';
    }
}

and add it as service:

    CoreShop\Payum\UnzerBundle\Action\ConvertCoreShopPaymentAction:
        tags:
            - { name: payum.action, alias: unzer_convert_action, factory: unzer, gateway: unzer, prepend: true }

In there, you have complete Control over the CoreShop Order and can access anything and prepare data for the Payment Provider.

@solverat
Copy link
Contributor Author

Hey @dpfaffenbauer:

"So as it was before": Almost. In CS2, the Payum Gateway also was able to manage the request source, because it was a real Payum Payment Interface.

Within your proposal, the CoreShop bundle always needs to copy the content of the Gatway's ConvertPaymentAction, because it will never apply - something I don't want to deal with. Every CoreShop Payum Bundle needs to adopt those changes. Sometimes there can be huge, if you're looking at existing extensions:

https://github.com/search?l=PHP&q=%22class+ConvertPaymentAction%22&type=Code

So I'm far away from accepting this as a solution. But I can't do more than suggest an alternative solution (personally, I would go with "Way II").

@dpfaffenbauer
Copy link
Member

@solverat It's too late for that now, CoreShop 3.0 is out and thats the way it is now.

@solverat
Copy link
Contributor Author

solverat commented Feb 8, 2023

BTW: One of our devs (@Zodiarc) has found a much cleaner solution (And keep the extension as it is):

// ConvertPaymentExtension
$paymentEntity = $this->paymentRepository->createQueryBuilder('p')
    ->where('p.number = :orderNumber')
    ->setParameter('orderNumber', $payment->getNumber())
    ->getQuery()
    ->getSingleResult();

if (!$paymentEntity instanceof \Coreshop\Component\Core\Model\Payment) {
    return;
}

$order = $paymentEntity->getOrder();

// ... append data to $request 

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

Successfully merging this pull request may close these issues.

2 participants