[PHPDOC] Fix functions orderComment in Magento\Sales\Model\Order

[dimitriBouteille]

Description (*)

This PR fix the bad phpdoc in \Magento\Sales\Model\Order. The argument $comment in the following functions accept a Phrase object

• addStatusToHistory
• addStatusHistoryComment
• addCommentToStatusHistory

Here is an example where the type Phrase is used :

magento2/app/code/Magento/Paypal/Model/Ipn.php

Lines 220 to 226 in d06337a

	$message = __(
	'IPN "%1". A dispute has been resolved and closed. %2 Transaction amount %3.',
	ucfirst($reasonCode),
	$notificationAmount,
	$reasonComment
	);
	$this->_order->addStatusHistoryComment($message)->setIsCustomerNotified(false)->save();

With PHPSTAN is very complicated to setup the project with level 5 or higher :(

Related Pull Requests

None

Fixed Issues (if relevant)

None

Manual testing scenarios (*)

Setup PHPSTAN with level 5 or higher and run check.

Questions or comments

The quality of Magento code needs to be improved so that it is easier to use the code quality tools (phpstan, rector, ...) ❤️

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)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] [PHPDOC] Fix functions orderComment in Magento\Sales\Model\Order #39575: [PHPDOC] Fix functions orderComment in Magento\Sales\Model\Order

[m2-assistant]

Hi @dimitriBouteille. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

[dimitriBouteille]

@magento run all tests

[engcom-Hotel]

@magento create issue

[ihor-sviziev]

I don't really like this approach.
Instead, I would rather allow objects that implement a more generic\Stringable interface. https://www.php.net/manual/en/class.stringable.php
To make it happen, I think Magento\Framework\Phrase has to implement it.

I'm pretty sure Magento has similar issue in many places, so it would be logical to add support later to other places as well.

OR just to not support multiple types, and require the only string type, so that - we need to explicitly convert all places to string before passing it as a parameter

magento2/app/code/Magento/Paypal/Model/Ipn.php

Lines 220 to 226 in d06337a

	$message = __(
	'IPN "%1". A dispute has been resolved and closed. %2 Transaction amount %3.',
	ucfirst($reasonCode),
	$notificationAmount,
	$reasonComment
	);
	$this->_order->addStatusHistoryComment($message)->setIsCustomerNotified(false)->save();

@dimitriBouteille @hostep any thoughts on it?

---

Just a bit more research, and how have the following thoughts:
it works just because

• we don't have declare(strict_types=1); declaration (I'm pretty sure sooner or later, we'll have these declarations)
• and, we don't have defined artgument types (I'm pretty sure sooner or later, we'll have these declarations)

So, the code sample of the current state is like this:
https://3v4l.org/GNbYp

Suggested options

Option 1: allow \Stringable interface as an argument parameter and mark Phrase class to explicitly implement \Stringable class

Code sample:
https://3v4l.org/CQM7s

Option 2: Just allow \Stringable interface as an argument parameter

Code sample:
https://3v4l.org/VMduZ

After playing with online code editor, I realized that this interface is automatically gets implemented:

---

Having having that in mind, I think we should consider Option2.

[dimitriBouteille]

Hi @ihor-sviziev

I agree with you that this solution is not the cleanest, ideally we should use the Stringable interface as you propose.

However, adding the Stringable interface on the Phrase class seemed more risky to me, that’s why I didn’t take the initiative to do it here:)

In my opinion, this PR may be a quick fix and in the near future, we should review the code of Magento to use Stringable in Phrase, Block, ... but it is a correction that will take some time.

If you OK for you, I can do a second PR to use Stringable where there is need :)

For information, I propose similar PR for Magento\Framework\Message\ManagerInterface : #39153

[ihor-sviziev]

@dimitriBouteille,
you replied to my #39572 (review) while I was editing it :)

I added code samples and options that I see, I think Option 2 should fit the best for us.

About another PR - I also linked it to the "parent" issue

Spoiler: we may not add implements Stringable on Phrase class for now.

PS: I didn't check how phpstan works in this mode, but I think it should catch it correctly

[dimitriBouteille]

@ihor-sviziev Ok for option 2 :)

However, in the future, adding the \Stringable interface would be a good idea to apply the best PHP practices.

I also tested PHPStan, the option 2 works ❤️

[dimitriBouteille]

@ihor-sviziev I let you validate that it’s all good:)

[ihor-sviziev]

@magento run all tests