[BUGFIX] Forward-port of #14861 for Magento 2.3

[kanduvisla]

This PR is a forward fork for PR #19079 for Magento 2.3 and fixes an issue (#14861) when products are programmatically assigned to a category, or are swapped with positions.

Description

This PR changes the way how the key is calculated in Magento\Catalog\Model\ResourceModel\Product\CategoryLink::processCategoryLinks(). It also improves the unit test by not just verifying if the proper category ID's are touched, but also if the proper insert, update or delete action is performed.

Fixed Issues

This PR fixes #14861

Manual testing scenarios

Steps to reproduce are described in detail in #14861

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 on Travis CI are green)

[magento-engcom-team]

Hi @kanduvisla. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me $VERSION instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[orlangur]

@rogyar processing should start from 2.3-develop.

[kanduvisla]

@magento-engcom-team : I'm puzzled, is there any action required from me for this PR?

[orlangur]

@kanduvisla not yet: this PR was assigned but not reviewed yet.

[rogyar]

Hi @kanduvisla. This point is not clear enough for me.
As far as I see, the new approach based on the loop reflects absolutely the same logic that was previously based on array_search. Is the difference in the strict types comparison (if yes, we could use strict parameter in array_search)?
I might miss something, so any additional information is appreciated.
Thank you.

[kanduvisla]

Hi @rogyar ,

The problem with the previous implementation is not the fact that array_search() was used, but that array_column() was used to get the $key of $oldCategoryPositions. array_column() recreates the index, so the second time the loop was run, it could possibly return an index of 0, which could already have been unset() on $oldCategoryPositions(), triggering an undefined index.

This bug can be exposed by running the extra testcase I added to the unit test:

[
    [
        ['category_id' => 3, 'position' => 10],
        ['category_id' => 4, 'position' => 20],
    ],
    [
        ['category_id' => 3, 'position' => 20], // swapped positions
        ['category_id' => 4, 'position' => 10], // swapped positions
    ],
    [
        'update' => [3, 4],
        'insert' => [],
        'delete' => [],
    ],
]

The above testcase is perfectly valid, because it is a simple swap of position, so it should trigger 2 updates. On the first run (in $insertUpdate = $this->processCategoryLinks($categoryLinks, $oldCategoryLinks);), the index of category_id => 3 on $oldCategoryPositions is 0. Coincidentally, this is also the same index returned by the array_column()-function. So $key = 0 and later on unset($oldCategoryPositions[$key]); is done.

Now comes the bug: on the second run (in $deleteUpdate = $this->processCategoryLinks($oldCategoryLinks, $categoryLinks);), the array_column() would still return 0 (because of the unset() earlier). However, since we've already unset() this one, the unset($oldCategoryPositions[$key]); will now throw a notice, which in turn will trigger an exception further on.

Even if you don't merge this PR, you can replicate this by adding the extra testcase to the unit test, and replace the last line:

    [
        'update' => [3, 4],
        'insert' => [],
        'delete' => [],
    ],

to:

[3,4]

But I also updated the unit test to see if the proper CRUD operations are done.

Now as for your question: I don't know if array_search() can still be used since we're dealing with a multidimensional array. Perhaps some other array_*-function could be used with something like a callback, but because the operation is quite a simple check and we do want to maintain our index I choose for a foreach in a foreach (forgive me lord for I have sinned).

[kanduvisla]

Hi @rogyar ,

Any update on this?

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-3399 has been created to process this Pull Request

[magento-engcom-team]

Hi @kanduvisla. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.