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

thomas-kl1 commented 1 year ago

Preconditions and environment

Any version of Magento
Anything else that would help a developer reproduce the bug

Steps to reproduce

Add additionalInformation to the \Magento\Quote\Api\Data\PaymentInterface object in place order action
When placing order, Magento copy quote payment to order payment
\Magento\Quote\Model\Quote\Payment\ToOrderPayment::convert
\Magento\Framework\DataObject\Copy::getDataFromFieldset

This method is used:

case $source instanceof DataObject:
$value = $source->getDataUsingMethod($code);
break;

\Magento\Quote\Api\Data\PaymentInterface & \Magento\Sales\Api\Data\OrderPaymentInterface shares the same definition for additional property:

    /**
     * Get payment additional details
     *
     * @return string[]|null
     */
    public function getAdditionalData();

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

Obviously, copying array to string will impact the save action of the order. When the order is saved, the payment relation is processed and throws an error of array to string conversion.

Expected result

Order is placed.

Actual result

An error occurred and the order is not placed.

Additional information

No response

Release note

No response

Triage and priority

[ ] Severity: S0 - Affects critical data or functionality and leaves users without workaround.
[ ] Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
[X] Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
[ ] Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
[ ] Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

m2-assistant[bot] commented 1 year ago

Hi @thomas-kl1. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release
For more details, review the Magento Contributor Assistant documentation.
Add a comment to assign the issue: @magento I am working on this
To learn more about issue processing workflow, refer to the Code Contributions.

Join Magento Community Engineering Slack and ask your questions in #github channel. :warning: According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting. :clock10: You can find the schedule on the Magento Community Calendar page. :telephone_receiver: The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

ia-gaurav commented 1 year ago

@magento give me 2.4.6-develop instance

magento-deployment-service[bot] commented 1 year ago

Hi @ia-gaurav. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @ia-gaurav, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

ia-gaurav commented 1 year ago

@magento give me 2.4-develop instance

magento-deployment-service[bot] commented 1 year ago

Hi @ia-gaurav. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @ia-gaurav, here is your Magento Instance: https://e4112445b9a73c84cfcf69126c87795c.instances-prod.magento-community.engineering Admin access: https://e4112445b9a73c84cfcf69126c87795c.instances-prod.magento-community.engineering/admin_84a8 Login: ca59fa83 Password: d1623b07d71e

thomas-kl1 commented 1 year ago

@magento give me 2.4.6-develop instance

magento-deployment-service[bot] commented 1 year ago

Hi @thomas-kl1. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @thomas-kl1, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

desaiuttam commented 1 year ago

@magento give me 2.4.6-develop instance

magento-deployment-service[bot] commented 1 year ago

Hi @desaiuttam. Thank you for your request. I'm working on Magento instance for you.

magento-deployment-service[bot] commented 1 year ago

Hi @desaiuttam, unfortunately there is no ability to deploy Magento instance at the moment. Please try again later.

maaarghk commented 1 year ago

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()
]);

m2-assistant[bot] commented 1 year ago

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
[ ] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

engcom-November commented 1 year ago

Hello @thomas-kl1, Thank you for the report and collaboration! Thank you @maaarghk for input!

We tried to reproduce this issue on 2.4-develop. We created a custom module and plugin to add additionalData but in our case we are able to place the order successfully. Please find the attached module and let us know if we are missing anything. DataVendor.zip

Thank you.

maaarghk commented 1 year ago

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?

m2-assistant[bot] commented 1 year ago

Hi @engcom-Hotel. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

[ ] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
[ ] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
[ ] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
[ ] 4. Verify that the issue is reproducible on 2.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 5. Add label Issue: Confirmed once verification is complete.
[ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

engcom-Hotel commented 1 year ago

Hello @thomas-kl1,

We are trying to reproduce this issue in 2.4-develop branch, meanwhile can you please elaborate the below step:

Add additionalInformation to the \Magento\Quote\Api\Data\PaymentInterface object in place order action

Thanks

engcom-Hotel commented 1 year ago

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

maaarghk commented 1 year ago

It's not us who need assistance. I don't see you even remotely engaging with the detail information that I left you in this issue @engcom-Hotel - you don't need to wait for @thomas-kl1 to act on it or attempt to understand it, do you?

engcom-Hotel commented 1 year ago

Hello @maaarghk,

Actually, we have a query related to this line in steps to reproduce:

Add additionalInformation to the \Magento\Quote\Api\Data\PaymentInterface object in place order action

which is mentioned here in this comment https://github.com/magento/magento2/issues/37295#issuecomment-1729330549

Let me give one more attempt to this issue. We are reopening this issue.

Thanks

engcom-Hotel commented 1 year ago

Hello @thomas-kl1,

I think we are able to reproduce the issue now in 2.4-develop branch. In order to reproduce the issue we create a plugin on Magento\Quote\Api\Data\PaymentInterface.

While making order, the below error occured:

In order to reproduce the issue we have made below custom module:

Magz.zip

Hence confirming the issue.

Thanks

github-jira-sync-bot commented 1 year ago

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-9652 is successfully created for this GitHub issue.

m2-assistant[bot] commented 1 year ago

:white_check_mark: Confirmed by @engcom-Hotel. Thank you for verifying the issue.
Issue Available: @engcom-Hotel, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

magento / magento2