Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 19, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3905580
[BUGFIX] Forward-port of #14861 for Magento 2.3
kanduvisla Nov 6, 2018
c2100db
[REFACTOR] Split the method to lower cyclomatic complexity
kanduvisla Nov 6, 2018
d904697
ENGCOM-3399: Static tests fixed.
p-bystritsky Dec 10, 2018
9b919f9
Merge branch '2.3-develop' into GITHUB-14861-for-2.3
p-bystritsky Dec 10, 2018
9cb7a89
ENGCOM-3399: Static tests fixed.
p-bystritsky Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/code/Magento/Catalog/Model/ResourceModel/Product/CategoryLink.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,15 @@ private function processCategoryLinks($newCategoryPositions, &$oldCategoryPositi
$result = ['changed' => [], 'updated' => []];

$oldCategoryPositions = array_values($oldCategoryPositions);
$oldCategoryList = array_column($oldCategoryPositions, 'category_id');
foreach ($newCategoryPositions as $newCategoryPosition) {
$key = array_search($newCategoryPosition['category_id'], $oldCategoryList);
$key = false;

foreach ($oldCategoryPositions as $oldKey => $oldCategoryPosition) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rogyar ,

Any update on this?

if ((int)$oldCategoryPosition['category_id'] === (int)$newCategoryPosition['category_id']) {
$key = $oldKey;
break;
}
}

if ($key === false) {
$result['changed'][] = $newCategoryPosition;
Expand Down
106 changes: 101 additions & 5 deletions app/code/Magento/Catalog/Test/Unit/Model/ResourceModel/Product/CategoryLinkTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Catalog\Test\Unit\Model\ResourceModel\Product;

use Magento\Catalog\Model\ResourceModel\Product\CategoryLink;
Expand Down Expand Up @@ -129,9 +130,20 @@ public function testSaveCategoryLinks($newCategoryLinks, $dbCategoryLinks, $affe
);
}

$expectedResult = [];

foreach ($affectedIds as $type => $ids) {
$expectedResult = array_merge($expectedResult, $ids);
// Verify if the correct insert, update and/or delete actions are performed:
$this->setupExpectationsForConnection($type, $ids);
}

$actualResult = $this->model->saveCategoryLinks($product, $newCategoryLinks);

sort($actualResult);
$this->assertEquals($affectedIds, $actualResult);
sort($expectedResult);

$this->assertEquals($expectedResult, $actualResult);
}

/**
Expand All @@ -151,7 +163,11 @@ public function getCategoryLinksDataProvider()
['category_id' => 3, 'position' => 10],
['category_id' => 4, 'position' => 20],
],
[], // Nothing to update - data not changed
[
'update' => [],
'insert' => [],
'delete' => [],
],
],
[
[
Expand All @@ -162,7 +178,11 @@ public function getCategoryLinksDataProvider()
['category_id' => 3, 'position' => 10],
['category_id' => 4, 'position' => 20],
],
[3, 4, 5], // 4 - updated position, 5 - added, 3 - deleted
[
'update' => [4],
'insert' => [5],
'delete' => [3],
],
],
[
[
Expand All @@ -173,16 +193,92 @@ public function getCategoryLinksDataProvider()
['category_id' => 3, 'position' => 10],
['category_id' => 4, 'position' => 20],
],
[3, 4], // 3 - updated position, 4 - deleted
[
'update' => [3],
'insert' => [],
'delete' => [4],
],
],
[
[],
[
['category_id' => 3, 'position' => 10],
['category_id' => 4, 'position' => 20],
],
[3, 4], // 3, 4 - deleted
[
'update' => [],
'insert' => [],
'delete' => [3, 4],
],
],
[
[
['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' => [],
],
]
];
}

/**
* @param $type
* @param $ids
*/
private function setupExpectationsForConnection($type, $ids): void
{
switch ($type) {
case 'insert':
$this->connectionMock
->expects($this->exactly(empty($ids) ? 0 : 1))
->method('insertArray')
->with(
$this->anything(),
$this->anything(),
$this->callback(function ($data) use ($ids) {
$foundIds = [];
foreach ($data as $row) {
$foundIds[] = $row['category_id'];
}
return $ids === $foundIds;
})
);
break;
case 'update':
$this->connectionMock
->expects($this->exactly(empty($ids) ? 0 : 1))
->method('insertOnDuplicate')
->with(
$this->anything(),
$this->callback(function ($data) use ($ids) {
$foundIds = [];
foreach ($data as $row) {
$foundIds[] = $row['category_id'];
}
return $ids === $foundIds;
})
);
break;
case 'delete':
$this->connectionMock
->expects($this->exactly(empty($ids) ? 0 : 1))
->method('delete')
// Verify that the correct category ID's are touched:
->with(
$this->anything(),
$this->callback(function ($data) use ($ids) {
return array_values($data)[1] === $ids;
})
);
break;
}
}
}