adyen-magento2: savePaymentInformationAndPlaceOrder plugins incorrectly changing return type

Magento version: 2.3.2-p2 Plugin version: 5.0.2 Description

This issue manifested in the checkout as the place order button showing various errors such as “The entity that was requested doesn’t exist. Verify the entity and try again” and "No such entity with cartId = " - however we found that the order was being placed and a refresh of the checkout would instead show no items in the shopping cart.

Screen Shot 2019-11-21 at 16 08 54

A stack trace on the exception revealed that another module was throwing a NoSuchEntityException with the order repository being unable to retrieve the entity by order id.

Various third party modules intercept the savePaymentInformationAndPlaceOrder method which is part of the \Magento\Checkout\Api\PaymentInformationManagementInterface class

The original class contains the following definition in the PHP file - take note of the return type

    /**
     * Set payment information and place order for a specified cart.
     *
     * @param int $cartId
     * @param \Magento\Quote\Api\Data\PaymentInterface $paymentMethod
     * @param \Magento\Quote\Api\Data\AddressInterface|null $billingAddress
     * @throws \Magento\Framework\Exception\CouldNotSaveException
     * @return int Order ID // RETURN TYPE INT / ORDERID
     */
      public function savePaymentInformationAndPlaceOrder

This means that any modules extending this class (including your own!) rely on the method to return an integer containing the new order id.

However, the plugins AdyenPaymentInformationManagementAddPaymentInfo and AdyenGuestPaymentInformationManagementAddPaymentInfo can change the return type to an encoded JSON string, breaking plugins further down the chain.

    /**
     * @param \Magento\Checkout\Api\GuestPaymentInformationManagementInterface $subject
     * @param $result
     * @return string
     * @throws \Magento\Framework\Exception\LocalizedException
     */
    public function afterSavePaymentInformationAndPlaceOrder(
        \Magento\Checkout\Api\GuestPaymentInformationManagementInterface $subject,
        $result
    ) {
        try {
            $order = $this->orderRepository->get($result);
            $payment = $order->getPayment();

            if ($payment->getMethod() === AdyenCcConfigProvider::CODE) {
                //THIS CHANGED THE RETURN TYPE
                return $this->adyenHelper->buildThreeDS2ProcessResponseJson(
                    $payment->getAdditionalInformation('threeDSType'),
                    $payment->getAdditionalInformation('threeDS2Token')
                );
            } else {
                return $result;
            }
        } catch (NoSuchEntityException $e) {
            $this->adyenLogger->error("Exception: " . $e->getMessage());
            throw new \Magento\Framework\Exception\LocalizedException(__('This order no longer exists.'));
        }

        return $result;
    }

This means that modules further down the chain will attempt to load an order from the order repository using your JSON 3DS object as the order ID.

I would suggest either changing the mechanism so that the return type is always the order ID, or at least setting the plugin to use a much higher value for the sort order

Here is an example of a plugin which is broken from the return type being changed.


  /**
     * @param \Magento\Checkout\Model\PaymentInformationManagement $subject
     * @return int Order ID.
     */
    public function afterSavePaymentInformationAndPlaceOrder(
        \Magento\Checkout\Model\PaymentInformationManagement $subject,
        $result
        )
    {
        if (!$this->helper->isEnabled()) {
            return $result;
        }

        $orderId = $result;

        //THE ORDERID IS NOW INVALID AS IT IS A JSON OBJECT BEING PASSED BY ADYEN
        $order = $this->orderRepository->get($orderId);
        ...

        return $result;
    }

I have attached a temporary workaround that we have implemented which raises the order of the Adyen plugin so that it is more likely to be executed last

AdyenDi.zip

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 17 (7 by maintainers)

Commits related to this issue

Most upvoted comments

HI @Maikel-Koek, @PascalBrouwers, @JeroenVanLeusden

Thank you all for your inputs in this issue. I will answer everyone below.

@Maikel-Koek

You mentioned it was a bad practice what you did before, but I think this is an even more bad practice, changing the return type (and value!) of a core method.

Yes we are aware. This is why we want to run the after interceptor the latest so we leave the rest of the plugins untouched but as @ryanpalmerweb raised the issue that the sort order is not working as expected in Magento this approach seems to fail. I also found this thread regarding this issue https://github.com/magento/magento2/issues/25150 . Since this is only an issue in certain versions we cannot change the sort order for some and keep it as it is for the rest, that is why the custom module would be a solution but I also understand that it is not a possibility for everyone.

I’m not sure how to get around this, isn’t there another way for you to implement this?

The biggest issue that we have to do the 3DS2 validation after the order was created in Magento but before we redirect to the success or error page. We are going to try to find a better solution for this without messing the interceptor return value, but as a quick solution we chose this approach.

@PascalBrouwers The sort order suggestion is only for those who has the interceptor sort order issue described in the current ticket above. The ones with a version where the sort order works as expected this interceptor should run the last. I understand your concerns with the return type, we are going to try to find a better solution for this.

@JeroenVanLeusden

It should be implemented in a different way If interaction with the frontend is needed.

Do you have any suggestion for us to change the implementation without breaking the type constraint while staying on the checkout page?

Also sort order of after plugins seems to be broken, see magento/magento2#25678. Even the sort order ‘solution’ you provide will fail eventually if it’s fixed by Magento.

Yes the sort order of the after plugin is broken, we got quite a good proof from @ryanpalmerweb and we also found the issue already raised at Magento. The initial idea was if this plugin is running the latest then it shouldn’t fail in our case since the frontend will handle the modified response type, but we are really open to move for a better solution.

Thank you again for the input, I hope I answered well and we can fix it as soon as possible.

Best, Attila Adyen

Hi @ryanpalmerweb , @Maikel-Koek , @PascalBrouwers , @zetxek , @vtransy

Thank you all for the input and for the patience!

In the latest release (version 6.0.0) we released the fix for this issue, removing the interceptor which was overwriting the response. We introduced instead a new API endpoint to retrieve the current payment status. You can check the changes in the pull request #643

Please note:

  • For PWA integrations this is a breaking change because now you need to perform another API call before you can redirect the shopper to the success page. (The updated PWA integration documentation is coming soon for the new release on our docs page)
  • There are changes on the checkout frontend as well, so if you customised the frontend - extending or overwriting the plugin’s default behaviour it may break the customisation.

I will close this issue for now but please feel free to reopen it if you still experience the issue after the upgrade or have any additional feedback.

Best, Attila Adyen

please fix 😉

Hi @ryanpalmerweb ,

Thank you for reaching out. We use only the after method in the interceptor that is why we chose to use sortOrder=10 . On this link https://devdocs.magento.com/guides/v2.3/extension-dev-guide/plugins.html#prioritizing-plugins you can read about the prioritisation. It says if you want to run your after method the last, you should add the smallest sortOrder number for it. I’m surprised that changing the sortOrder for a larger number helped solving your issue, because it means the docs is not correct - or I misunderstood something 😃. Can you maybe share your regarding di.xml settings and maybe log the processing order of the other interceptors? That would be really interesting to see for us too.

Regarding the response type and content of the after method, unfortunately we cannot send the integer back because of the 3DS2 flow where we require multiple step interaction with the customer on the frontend. Before 5.0.0 we did it before placing the order so we didn’t need to use interceptors, but turned out that it was a bad practise because the order validation and creation could go south and then the payment was already done while the order was not created. That is why we decided to change the behaviour like this.

Let me know what do you think and of course I’m waiting for the lines and logs so we can check if the prioritisation is indeed wrong and we should use a higher value or not.

Best, Attila Adyen

@vtransy We know how to resolve this, but it’s a workaround, not a fix of the actual problem 😃

But it looks like Adyen is starting to correctly resolve this in this PR: https://github.com/Adyen/adyen-magento2/pull/643