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

dimitriBouteille · 2025-01-28T11:02:19Z

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:

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

m2-assistant · 2025-01-28T11:02:23Z

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:

Database Compare
Functional Tests CE
Functional Tests EE
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
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 · 2025-01-28T11:02:58Z

@magento run all tests

engcom-Hotel · 2025-01-28T11:07:55Z

@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 · 2025-01-31T11:45:43Z

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 · 2025-01-31T12:03:32Z

@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

app/code/Magento/Sales/Model/Order.php

dimitriBouteille · 2025-01-31T13:09:10Z

@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 ❤️

…to\Sales\Model\Order

dimitriBouteille · 2025-01-31T14:52:21Z

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

ihor-sviziev · 2025-01-31T15:17:20Z

@magento run all tests

Fix bad phpdoc for function orderComment in Magento\Sales\Model\Order

3d56049

m2-assistant bot mentioned this pull request Jan 28, 2025

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

Open

5 tasks

engcom-Hotel added the Priority: P3 May be fixed according to the position in the backlog. label Jan 28, 2025

ihor-sviziev requested changes Jan 31, 2025

View reviewed changes

ihor-sviziev mentioned this pull request Jan 31, 2025

PHPStan returns false positives because of incoherent PHPDocs on functions/parameters that accept Phrase objects #36063

Open

5 tasks

ihor-sviziev requested changes Jan 31, 2025

View reviewed changes

app/code/Magento/Sales/Model/Order.php Outdated Show resolved Hide resolved

app/code/Magento/Sales/Model/Order.php Outdated Show resolved Hide resolved

app/code/Magento/Sales/Model/Order.php Outdated Show resolved Hide resolved

ihor-sviziev mentioned this pull request Jan 31, 2025

[PHPDOC] Fix bad phpdoc for Magento\Framework\Message\ManagerInterface #39153

Open

6 tasks

PHPDoc: Use \Stringable instead of \Magento\Framework\Phrase in Magen…

9e8b873

…to\Sales\Model\Order

dimitriBouteille added 2 commits February 4, 2025 09:10

Merge branch '2.4-develop' into phpdoc/sales-order-history

5493815

Merge branch '2.4-develop' into phpdoc/sales-order-history

ddde5d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dimitriBouteille commented Jan 28, 2025 •

edited by m2-assistant bot

Loading

m2-assistant bot commented Jan 28, 2025

dimitriBouteille commented Jan 28, 2025

engcom-Hotel commented Jan 28, 2025

ihor-sviziev left a comment •

edited

Loading

dimitriBouteille commented Jan 31, 2025 •

edited

Loading

ihor-sviziev commented Jan 31, 2025 •

edited

Loading

dimitriBouteille commented Jan 31, 2025

dimitriBouteille commented Jan 31, 2025

ihor-sviziev commented Jan 31, 2025

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

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

Are you sure you want to change the base?

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

Conversation

dimitriBouteille commented Jan 28, 2025 • edited by m2-assistant bot Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Jan 28, 2025

dimitriBouteille commented Jan 28, 2025

engcom-Hotel commented Jan 28, 2025

ihor-sviziev left a comment • edited Loading

Choose a reason for hiding this comment

Suggested options

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

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

dimitriBouteille commented Jan 31, 2025 • edited Loading

ihor-sviziev commented Jan 31, 2025 • edited Loading

dimitriBouteille commented Jan 31, 2025

dimitriBouteille commented Jan 31, 2025

ihor-sviziev commented Jan 31, 2025

dimitriBouteille commented Jan 28, 2025 •

edited by m2-assistant bot

Loading

ihor-sviziev left a comment •

edited

Loading

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

dimitriBouteille commented Jan 31, 2025 •

edited

Loading

ihor-sviziev commented Jan 31, 2025 •

edited

Loading