magento2: \Magento\Quote\Model\Quote\Payment\ToOrderPayment Failure if payment has additionalData

Hello @thomas-kl1,

We have noticed that this issue has not been updated for a period of long time. Hence we assume that this issue is fixed now, so we are closing it. Please raise a fresh ticket or reopen this ticket if you need more assistance on this.

Regards

let us know if we are missing anything.

Apparently very many things, for example, your plugin is for Magento\Quote\Model\Quote\Payment whereas the interface being discussed here is Magento\Quote\Api\Data\PaymentInterface.

Your docblock says this:

    /**
     *
     * @param \Magento\Quote\Model\Quote\Payment $subject
     * @param array $additionalData
     * @return array
     */

However Magento\Quote\Model\Quote\Payment implements Magento\Quote\Api\Data\PaymentInterface where setAdditionalData has the docblock

/**
 * Set payment additional details
 *
 * @param string $additionalData
 * @return $this
 */

As you can see the string typehint for $additionalData is a mismatch with what the array typehint you have used (and is used in the implementation).

The reason this is a problem is the API - and apparently also the place order action - via Magento\Framework\DataObject\Copy - will use the typehint from PaymentInterface when setting additionalData and therefore will pass a string to setAdditionalData when the class Payment expects an array and throw an error.

Since the above explanation is not really related to the bug at hand, and more related to basic comprehension of programming in PHP, might I suggest escalating the bug to someone more senior?

Not sure if this is a related issue or part of the same problem, but I did spot an issue with the API here. Note the docblock in Magento\Quote\Api\Data\PaymentInterface as quoted above:

/**
 * Set payment additional details
 *
 * @param string $additionalData
 * @return $this
 */
public function setAdditionalData($additionalData);

which suggests setAdditionalData expects a string.

Therefore we can expect $paymentInterface->getData(‘additional_data’) to return a string after calling this function. getAdditionalData() function implements conversion of serialised string back to an array hence docblock there states string[].

Given this interface API, a user will set additionalData to JSON as follows:

$paymentData->setAdditionalData(json_encode([
    "public_hash" => $token->getPublicHash(),
    "customer_id" => $token->getCustomerId()
]));
$orderId = $this->paymentInformationManagement->savePaymentInformationAndPlaceOrder($quoteId, $paymentData);

n.b. savePaymentInformationAndPlaceOrder param 2 does expect a PaymentInterface directly and not some implementation of it.

Consider the following stack trace (interceptors cleaned for legibility) - line numbers are from 2.4.6-p1 (perhaps with some quality patches)

#0 magento/module-quote/Model/Quote/Payment.php(225): array_merge()
#1 magento/module-quote/Model/Quote/Payment.php(172): Magento\Quote\Model\Quote\Payment->convertPaymentData(Array)
#2 magento/module-quote/Model/PaymentMethodManagement.php(79): Magento\Quote\Model\Quote\Payment->importData(Array)
#7 magento/module-checkout/Model/PaymentInformationManagement.php(204): Magento\Quote\Model\PaymentMethodManagement->set('864465', Object(Magento\Quote\Model\Quote\Payment))
#12 magento/module-checkout/Model/PaymentInformationManagement.php(147): Magento\Checkout\Model\PaymentInformationManagement->savePaymentInformation('864465', Object(Magento\Quote\Model\Quote\Payment), NULL)
#17 Magento\Checkout\Model\PaymentInformationManagement->savePaymentInformationAndPlaceOrder('864465', Object(Magento\Quote\Model\Quote\Payment))

In set function (param 2 expects PaymentInterface):

$paymentData = $method->getData();
$payment = $quote->getPayment();
$payment->importData($paymentData);

In convertPaymentData function:

foreach (array_keys($rawData) as $requestKey) {
    if (!array_key_exists($requestKey, $paymentData)) {
        $paymentData[PaymentInterface::KEY_ADDITIONAL_DATA][$requestKey] = $rawData[$requestKey];
    } elseif ($requestKey === PaymentInterface::KEY_ADDITIONAL_DATA) {
        $paymentData[PaymentInterface::KEY_ADDITIONAL_DATA] = array_merge(
            $paymentData[PaymentInterface::KEY_ADDITIONAL_DATA],
            (array) $rawData[$requestKey]
        );
    } else {
        $paymentData[$requestKey] = $rawData[$requestKey];
    }
}

See the second clause is calling array_merge on essentially the result of $paymentInterface->getData('additional_data') even though we would expect that to return a string. I think there are a few clean solutions - in order of preference: (a) PaymentInterface takes an array in setAdditionalData and it is down to the resource / repository to serialize it/unserialize it when saving/loading. Then getAdditionalData override could be removed and there is consistency between the result of that function and the result of getData. (b) Magento\Quote\Model\Quote\Payment->importData is updated to take a PaymentInterface instead of an array as its parameter, and uses the getter functions where appopriate so that it has some confidence in the data types / static analysis picks up bugs like this (right now mixed[] is about as good as a static analyser can see). Maybe someone ought to git blame this code and write a phpcs rule to disallow passing arrays around where data objects could be used instead as well.

For any mage users like myself coming up against it the workaround is instead of using the interface method setAdditionalData on PaymentInterface, use setData instead:

$paymentData->setAdditionalData(json_encode([
    "public_hash" => $token->getPublicHash(),
    "customer_id" => $token->getCustomerId()
]));

becomes:

// don't use setAdditionalData because it forces us to pass a string when mage internal code expects an array
// todo cleanup after fixed - https://github.com/magento/magento2/issues/37295
$paymentData->setData('additional_data', [
    "public_hash" => $token->getPublicHash(),
    "customer_id" => $token->getCustomerId()
]);