MC-13180: Fetching invoice items with \Magento\Sales\Model\Order\Invoice getItems function sometimes returns a collection

[AleksLi]

Description (*)

Created an example of the solution. I think something like this would fix the issue since it is using a lot in the process of adding Item.

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes Fetching invoice items with \Magento\Sales\Model\Order\Invoice getItems function sometimes returns a collection #13180: Fetching invoice items with \Magento\Sales\Model\Order\Invoice getItems function sometimes returns a collection

Manual testing scenarios (*)

Well explained in the description of #13180

Questions or comments

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 @AleksLi. 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.

[lbajsarowicz]

Import class, fix the static tests 😀
Instead of changing the whole Interface, just return the array of Items. This is least backwards-incompatible way to achieve your goal.

[AleksLi]

@lbajsarowicz I was trying to resolve this another way but the logic here was constructed to return Magento\Sales\Model\ResourceModel\Order\Invoice\Item\Collection instead of getting InvoiceItemInterface as it says as the return type in the getItems() method.

Simply returning an array as that's written in the code won't make it work.
For example, we will get an error trying to addItem on
\Magento\Sales\Model\Order\Invoice::getItemsCollection line 553.

I debugged the issue for several hours trying to resolve it in an easy way.

Of course, method getItems() should return InvoiceItemInterface[] as it expected, but we should change everything connected to that. As you know that would affect other modules I guess and affect 3rd party modules.

Your suggestion?

This example below won't work, just for example.

[AleksLi]

@lbajsarowicz hello. Could you suggest any possible solution for that? How to do it appropriately?

[antonkril]

Changing getItems() to return hashmap and getItemCollection() to return collection instead of getItems() would solve your concern. It looks like getItemCollection() is the only client that depends on getItems() returning Collection. Everyone else seems to depend on iterable and countable.

This will, of course, be a breaking change, but only for people who depended on the unpublished interface (hacked).

Anything else I missed?

[AleksLi]

@antonkril thanks for your influence. Let me try to understand and try to apply your thought.

[AleksLi]

@antonkril Here is what I changed here.
Could you check this and tell me about getItems() method. I guess nothing should be changed there? Or I misunderstand something about your comment "Changing getItems() to hashmap .. "

[antonkril]

getItems is in the title of the issue, so we need to unify its behavior. Also, we need to modify getItemsCollectionX to not setItems

[AleksLi]

Alright, but what would be the behavior of the method getItemsCollection in this case? Of course, it should return ItemsCollection as expected from the name. As Collection is iterable it will contain the items? But items could be empty and what to return in that case?

[AleksLi]

@antonkril what if getItemsCollection would return only the collection as it expected from the name of the method?

[igorwulff]

Hi @AleksLi I have applied the latest changes in this PR and notice an issue when creating an invoice from at least the backend:

1 exception(s): Exception #0 (Exception): Warning: Invalid argument supplied for foreach() in vendor/magento/module-tax/Helper/Data.php on line 731

Apparently this change is breaking this code:
foreach ($salesItem->getItems() as $item) {

This is because $this->getId in the method getItems() returns null at this point:
public function getItems() { if ($this->getData(InvoiceInterface::ITEMS) === null && $this->getId()) {

A workaround could be calling getAllItems or getItemsCollection, but I'm uncertain sure of the impact. Also which other areas might also be affected by this? This all feels a bit like a can of worms.

[lfolco]

Hi, is there any plan to continue working on this and merge it?

[kenit]

Hi,
I also applied the change in this PR on my Magento 2.4.3-p1 instance.
I discovered that after I create a new invoice successfully, there is no invoice item exists.
Is anyone have any idea about how to fix it?