magento/magento2#39884: Fix type compatibility in Invoice comments me…

[yaroslav-kozar]

Description (*)

Fixed a type compatibility issue in Magento\Sales\Model\Order\Invoice. The problem occurred when calling getComments() before addComment(). getComments() stores an array in the object's internal data, while getCommentsCollection() (called by addComment) expected a Collection object, leading to a fatal error.

Key changes:

• Updated getCommentsCollection() to check for the correct instance and handle existing array data.
• Refactored getComments() to consistently return an array, even if a Collection is currently stored.
• Added reload=true in addComment() to ensure data consistency.
• Added a unit test covering these scenarios to prevent future regressions.

Related Pull Requests

None

Fixed Issues (if relevant)

1. Fixes Invoice addComment can throw error when Invoice getComments is called first #39884

Manual testing scenarios (*)

1. Create a script to load an existing invoice and execute the following:

$invoice = $objectManager->get(\Magento\Sales\Api\InvoiceRepositoryInterface::class)->get($id);

// 1. Populates internal data with an array
$invoice->getComments(); 

// 2. Throws Fatal Error before fix; completes successfully after fix
$invoice->addComment('Reproduce Bug'); 

$objectManager->get(\Magento\Sales\Api\InvoiceRepositoryInterface::class)->save($invoice);

2. Expected Result (Before fix): Error: Call to a member function addItem() on array.
3. Expected Result (After fix): Comment is added, and the script finishes without errors.
4. Automated tests: Run vendor/bin/phpunit app/code/Magento/Sales/Test/Unit/Model/Order/InvoiceTest.php.

Questions or comments

The fix ensures that the model correctly manages the transition between the Data Interface (arrays) and the Resource Model (collections).

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)

[m2-assistant]

Hi @yaroslav-kozar. 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.

[yaroslav-kozar]

@magento run all tests

[yaroslav-kozar]

@magento run all tests

[engcom-Hotel]

@magento run all tests

[engcom-Hotel]

Hello @yaroslav-kozar,

Thank you for your contribution!

The changes seems good to us, but please look into the failed tests they seems to failing due to PR changes.

The failed functional tests are flaky.

Thank you

[engcom-Dash]

Hello @yaroslav-kozar,

Thank you for your contribution!

The changes seems good to us, but please look into the failed tests they seems to failing due to PR changes.

The failed functional tests are flaky.

Thank you

@engcom-Hotel @yaroslav-kozar I am taking care of the static test failure, as the PR has been pending since last week.

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@engcom-Hotel, I have fixed the Static tests failure. Could you please review the PR so we can proceed further?

[engcom-Hotel]

@magento run all tests

[engcom-Hotel]

Failed tests seems flaky to me hence approving this PR

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@yaroslav-kozar

Thanks for the collaboration & contribution!

✔️ QA Passed

Preconditions:
Install fresh Magento 2.4-develop

Manual Steps:
Create a script to load an existing invoice and execute the following:

$invoice = $objectManager->get(\Magento\Sales\Api\InvoiceRepositoryInterface::class)->get($id);
// 1. Populates internal data with an array
$invoice->getComments(); 

// 2. Throws Fatal Error before fix; completes successfully after fix
$invoice->addComment('Reproduce Bug'); 

$objectManager->get(\Magento\Sales\Api\InvoiceRepositoryInterface::class)->save($invoice);

Before
FAIL - Call to a member function addItem() on array

After
ALL TESTS PASSED - THE BUG IS FIXED!

Builds are failed. Hence, moving this PR to Extended Testing.

Thanks

[engcom-Dash]

@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE, Integration Tests

[engcom-Dash]

@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE, Integration Tests

[engcom-Dash]

Few Functional B2B, CE and EE failures are not consistent in recent 2 builds. They are neither part of this PR nor failing because of the PR changes, seems to be flaky. The consistent failures are known issues and have open JIRAs.

CE
Build 1
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/93c7b5bf78693875957ddae4f55013bd/Functional/allure-report-ce/index.html#categories

Build 2
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/d12f83bba632437052c2d7f894570de9/Functional/allure-report-ce/index.html#categories/dad3b5a7b0574462665553fcc880a350/9b378cc13485ed71/

EE
Build 1
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/ecb9f339e2e1d7768a25d774f207f255/Functional/allure-report-ee/index.html#categories

Build 2
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/e97e55a20b1619403eb8eca9d1e0e2bf/Functional/allure-report-ee/index.html#categories

B2b
Build 1
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/194502d9c3076369a54a003bf21fc8cb/Functional/allure-report-b2b/index.html

Build 2
https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/40428/8e9c4ad7c790aaba77966f4f8e51a2f2/Functional/allure-report-b2b/index.html

Integration test
The failure is occurring across all PRs and is not related to this specific PR.

Known Issue
AssertSuccessMessageAppearsAfterAddingProductToCartThatContainsOutOfStockProductTest ACQE-9359
PurchaseOrderEnableDisableE2ETest ACQE-9362
AdminDisplayingCartAndCatalogPriceRulesGridsTest ACQE-9406

Hence, Moving this PR Merge In Progress.