magento#26745 add method setAdditionalInformation to OrderPaymentInte…

[antoninobonumore]

Description (*)

No setter for additional_information in OrderPaymentInterface is defined: this is an inconsistency between its schema declaration and the real properties accepted.

Fixed Issues (if relevant)

• Fixes OrderPaymentInterface is missing setAdditionalInformation() #26745: OrderPaymentInterface is missing setAdditionalInformation()
• Fixes Magento\Sales\Api\Data\OrderPaymentInterface is missing a setter for 'additional_information' data #13222: Magento\Sales\Api\Data\OrderPaymentInterface is missing a setter for 'additional_information' data

Manual testing scenarios (*)

• create an API interface accepting OrderPaymentInterface
• try to set an object with a property additional_information with a value

Questions or comments

IMO this looks like an elephant in the room since this definition is missing from a long time in the definition of this interface

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @antoninobonumore. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[hostep]

Let's hope this one makes it in, a previous attempt for fixing this got stopped because it would be backwards incompatible, but now the code for Magento 2.4 is still open, it can probably be accepted?

@rogyar ?

[antoninobonumore]

@hostep this is also because I implemented this on this release line rather than 2.2 and 2.3

[rogyar]

Hi. For 2.4 (as a minor version release) we may make backward-incompatible changes except changes to the functional structures marked as @api. We may not modify @api classes or interfaces since such changes might be very critical for the existing setups.

[hostep]

@rogyar: The version 2.4 is not the correct one here, we should be talking about the version of the Magento Sales module, which is at version 102.0.4 in Magento 2.3.4
I'm assuming in Magento 2.4, that version will get bumped to 103.0.0 so it will be a major version bump, but I don't know that for certain?

[rogyar]

Hi @hostep. I don't have this information either.

But the main point is adding public methods to interface/API implementations is prohibited for both patch and minor releases (so it's prohibited in any case).

[hostep]

@rogyar: A bump from 102.0.4 to 103.0.0 is a major version bump, right?

[hostep]

@rogyar: after asking around a bit in #appdesign on the Slack workspace of Magento, it looks like this should be no problem to merge into 2.4-develop.
According to https://devdocs.magento.com/guides/v2.3/extension-dev-guide/versioning/codebase-changes.html, adding a new method to an interface marked with @api can even be done within a MINOR version bump (102.0.4 => 102.1.0 for example).

So I would like to ask you to reconsider, thanks! 🙂

[drew7721]

I would suggest implementing the setter and getter for as public functions in the Sales\Model\Order\Payment class using the constant OrderPaymentInterface::ADDITIONAL_INFORMATION as it's done for all the others. Also, I would like to see the documentation related to the difference between ADDITIONAL_DATA and ADDITIONAL_INFORMATION

[rogyar]

Hi @hostep. After a recent discussion, I must admit that you are right. I'm approving this change. Thank you

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-7037 has been created to process this Pull Request
✳️ @rogyar, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[engcom-Alfa]

✔️ QA Passed

Before:

After:

[engcom-Echo]

Failed functional tests not related to the changes in this PR

[m2-assistant]

Hi @antoninobonumore, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[kanduvisla]

It's good to see that this property is added, but how can we deal with the discrepancies between the 2 different API's that describe this implementation?

If you look at \Magento\Sales\Model\Order\Payment it extends 2 interfaces: \Magento\Payment\Model\InfoInterface and \Magento\Sales\Api\Data\OrderPaymentInterface which both have a different @api setup for these methods:

\Magento\Payment\Model\InfoInterface:

// @param string $key
public function setAdditionalInformation($key, $value = null);
public function getAdditionalInformation($key = null);

\Magento\Sales\Api\Data\OrderPaymentInterface:

// @param string[] $additionalInformation
public function setAdditionalInformation($additionalInformation);
public function getAdditionalInformation();

This might require some attention because it's quote confusing now. It looks like the InfoInterface is the API that we wanted on the OrderPaymentInterface all along...