Skip to content

Commit 2d2c77d

Browse files
engcom-Echoengcom-Echo
committed
MC-38593: Complex products children ids are not cached in Type class leading to multiple unnecessary db requests
1 parent c935f5e commit 2d2c77d

File tree

8 files changed

+99
-21
lines changed

8 files changed

+99
-21
lines changed

‎app/code/Magento/Bundle/Model/Plugin/Product.php renamed to ‎app/code/Magento/Bundle/Model/Plugin/ProductIdentitiesExtender.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,14 @@
33
* Copyright © Magento, Inc. All rights reserved.
44
* See COPYING.txt for license details.
55
*/
6+
declare(strict_types=1);
7+
68
namespace Magento\Bundle\Model\Plugin;
79

810
use Magento\Bundle\Model\Product\Type as BundleType;
911
use Magento\Catalog\Model\Product as CatalogProduct;
1012

11-
class Product
13+
class ProductIdentitiesExtender
1214
{
1315
/**
1416
* @var BundleType

‎app/code/Magento/Bundle/Model/Product/Type.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,10 @@ public function getChildrenIds($parentId, $required = true)
311311
*/
312312
public function getParentIdsByChild($childId)
313313
{
314+
if (is_array($childId) && count($childId) > 1) {
315+
return $this->_bundleSelection->getParentIdsByChild($childId);
316+
}
317+
314318
$cacheKey = is_array($childId) ? implode('-', $childId) : $childId;
315319
if (!isset($this->cacheParentIdsByChild[$cacheKey])) {
316320
$this->cacheParentIdsByChild[$cacheKey] = $this->_bundleSelection->getParentIdsByChild($childId);

‎app/code/Magento/Bundle/Test/Unit/Model/Plugin/ProductTest.php renamed to ‎app/code/Magento/Bundle/Test/Unit/Model/Plugin/ProductIdentitiesExtenderTest.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@
88
namespace Magento\Bundle\Test\Unit\Model\Plugin;
99

1010
use Magento\Bundle\Model\Product\Type;
11-
use Magento\Bundle\Model\Plugin\Product as Plugin;
11+
use Magento\Bundle\Model\Plugin\ProductIdentitiesExtender as Plugin;
1212
use Magento\Catalog\Model\Product;
1313
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
1414
use PHPUnit\Framework\MockObject\MockObject;
1515
use PHPUnit\Framework\TestCase;
1616

17-
class ProductTest extends TestCase
17+
class ProductIdentitiesExtenderTest extends TestCase
1818
{
1919
/** @var Plugin */
2020
private $plugin;
@@ -25,6 +25,9 @@ class ProductTest extends TestCase
2525
/** @var MockObject|Product */
2626
private $product;
2727

28+
/**
29+
* @inheritdoc
30+
*/
2831
protected function setUp(): void
2932
{
3033
$objectManager = new ObjectManager($this);
@@ -46,6 +49,11 @@ protected function setUp(): void
4649
);
4750
}
4851

52+
/**
53+
* Verify after get identities
54+
*
55+
* @return void
56+
*/
4957
public function testAfterGetIdentities()
5058
{
5159
$baseIdentities = [

‎app/code/Magento/Bundle/Test/Unit/Model/Product/TypeTest.php

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
use Magento\Bundle\Model\ResourceModel\BundleFactory;
1313
use Magento\Bundle\Model\ResourceModel\Option\Collection;
1414
use Magento\Bundle\Model\ResourceModel\Selection as ResourceSelection;
15+
use Magento\Catalog\Helper\Product as ProductHelper;
16+
use Magento\Catalog\Model\Product\Option as OptionProduct;
1517
use Magento\CatalogRule\Model\ResourceModel\Product\CollectionProcessor;
1618
use Magento\Bundle\Model\ResourceModel\Selection\Collection as SelectionCollection;
1719
use Magento\Bundle\Model\ResourceModel\Selection\CollectionFactory;
@@ -72,7 +74,7 @@ class TypeTest extends TestCase
7274
protected $bundleCollectionFactory;
7375

7476
/**
75-
* @var \Magento\Catalog\Helper\Data|MockObject
77+
* @var Data|MockObject
7678
*/
7779
protected $catalogData;
7880

@@ -97,7 +99,7 @@ class TypeTest extends TestCase
9799
protected $stockState;
98100

99101
/**
100-
* @var \Magento\Catalog\Helper\Product|MockObject
102+
* @var ProductHelper|MockObject
101103
*/
102104
private $catalogProduct;
103105

@@ -132,7 +134,7 @@ class TypeTest extends TestCase
132134
private $bundleSelection;
133135

134136
/**
135-
* @return void
137+
* @inheritdoc
136138
*/
137139
protected function setUp(): void
138140
{
@@ -171,7 +173,7 @@ protected function setUp(): void
171173
->setMethods(['getStockQty'])
172174
->disableOriginalConstructor()
173175
->getMock();
174-
$this->catalogProduct = $this->getMockBuilder(\Magento\Catalog\Helper\Product::class)
176+
$this->catalogProduct = $this->getMockBuilder(ProductHelper::class)
175177
->setMethods(['getSkipSaleableCheck'])
176178
->disableOriginalConstructor()
177179
->getMock();
@@ -238,7 +240,7 @@ public function testGetChildrenIds()
238240
$parentId = 1;
239241
$childrenIds = [
240242
[
241-
1 => [
243+
0 => [
242244
26 => "26",
243245
39 => "39",
244246
],
@@ -256,14 +258,17 @@ public function testGetChildrenIds()
256258
$this->assertEquals($childrenIds, $this->model->getChildrenIds($parentId, true));
257259
}
258260

261+
/**
262+
* @return void
263+
*/
259264
public function testGetParentIdsByChild()
260265
{
261266
$this->assertClassHasAttribute('cacheParentIdsByChild', Type::class);
262267

263-
$childId = 10;
268+
$childId = 1;
264269
$parentIdsByChild = [
265270
[
266-
1 => [
271+
0 => [
267272
26 => "26",
268273
39 => "39",
269274
],
@@ -301,7 +306,7 @@ public function testPrepareForCartAdvancedWithoutOptions()
301306
)
302307
->disableOriginalConstructor()
303308
->getMock();
304-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
309+
/* @var MockObject|OptionProduct $option */
305310
$option = $this->getMockBuilder(Option::class)
306311
->setMethods(['groupFactory', 'getType', 'getId', 'getRequired', 'isMultiSelection'])
307312
->disableOriginalConstructor()
@@ -412,7 +417,7 @@ public function testPrepareForCartAdvancedWithShoppingCart()
412417
)
413418
->disableOriginalConstructor()
414419
->getMock();
415-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
420+
/* @var MockObject|OptionProduct $option */
416421
$option = $this->getMockBuilder(Option::class)
417422
->setMethods(
418423
[
@@ -656,7 +661,7 @@ public function testPrepareForCartAdvancedEmptyShoppingCart()
656661
)
657662
->disableOriginalConstructor()
658663
->getMock();
659-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
664+
/* @var MockObject|OptionProduct $option */
660665
$option = $this->getMockBuilder(Option::class)
661666
->setMethods(
662667
[
@@ -881,7 +886,7 @@ public function testPrepareForCartAdvancedStringInResult()
881886
)
882887
->disableOriginalConstructor()
883888
->getMock();
884-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
889+
/* @var MockObject|OptionProduct $option */
885890
$option = $this->getMockBuilder(Option::class)
886891
->setMethods(
887892
[
@@ -1100,7 +1105,7 @@ public function testPrepareForCartAdvancedWithoutSelections()
11001105
)
11011106
->disableOriginalConstructor()
11021107
->getMock();
1103-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
1108+
/* @var MockObject|OptionProduct $option */
11041109
$option = $this->getMockBuilder(Option::class)
11051110
->setMethods(['groupFactory', 'getType', 'getId', 'getRequired', 'isMultiSelection'])
11061111
->disableOriginalConstructor()
@@ -1200,7 +1205,7 @@ public function testPrepareForCartAdvancedSelectionsSelectionIdsExists()
12001205
)
12011206
->disableOriginalConstructor()
12021207
->getMock();
1203-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
1208+
/* @var MockObject|OptionProduct $option */
12041209
$option = $this->getMockBuilder(Option::class)
12051210
->setMethods(['groupFactory', 'getType', 'getId', 'getRequired', 'isMultiSelection'])
12061211
->disableOriginalConstructor()
@@ -1327,7 +1332,7 @@ public function testPrepareForCartAdvancedSelectRequiredOptions()
13271332
)
13281333
->disableOriginalConstructor()
13291334
->getMock();
1330-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
1335+
/* @var MockObject|OptionProduct $option */
13311336
$option = $this->getMockBuilder(Option::class)
13321337
->setMethods(['groupFactory', 'getType', 'getId', 'getRequired', 'isMultiSelection'])
13331338
->disableOriginalConstructor()
@@ -1491,7 +1496,7 @@ public function testPrepareForCartAdvancedAllRequiredOption()
14911496
)
14921497
->disableOriginalConstructor()
14931498
->getMock();
1494-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
1499+
/* @var MockObject|OptionProduct $option */
14951500
$option = $this->getMockBuilder(Option::class)
14961501
->setMethods(['groupFactory', 'getType', 'getId', 'getRequired'])
14971502
->disableOriginalConstructor()
@@ -1592,7 +1597,7 @@ public function testPrepareForCartAdvancedSpecifyProductOptions()
15921597
)
15931598
->disableOriginalConstructor()
15941599
->getMock();
1595-
/* @var \PHPUnit\Framework\MockObject\MockObject|\Magento\Catalog\Model\Product\Option $option */
1600+
/* @var MockObject|OptionProduct $option */
15961601
$option = $this->getMockBuilder(Option::class)
15971602
->setMethods(['groupFactory', 'getType', 'getId'])
15981603
->disableOriginalConstructor()

‎app/code/Magento/Bundle/etc/di.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@
8686
<plugin name="bundle" type="Magento\Bundle\Model\Plugin\PriceBackend" sortOrder="100" />
8787
</type>
8888
<type name="Magento\Catalog\Model\Product">
89-
<plugin name="bundle" type="Magento\Bundle\Model\Plugin\Product" sortOrder="100" />
89+
<plugin name="add_bundle_parent_identities" type="Magento\Bundle\Model\Plugin\ProductIdentitiesExtender" sortOrder="100" />
9090
</type>
9191
<type name="Magento\Sales\Model\Order\Item">
9292
<plugin name="bundle" type="Magento\Bundle\Model\Sales\Order\Plugin\Item" sortOrder="100" />

‎app/code/Magento/ConfigurableProduct/Model/Product/Type/Configurable.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,11 @@ public function getChildrenIds($parentId, $required = true)
369369
*/
370370
public function getParentIdsByChild($childId)
371371
{
372-
$cacheKey = is_array($childId) ? implode('-', $childId) : $childId;
372+
if (is_array($childId) && count($childId) > 1) {
373+
return $this->_catalogProductTypeConfigurable->getParentIdsByChild($childId);
374+
}
375+
376+
$cacheKey = is_array($childId) ? array_shift($childId) : $childId;
373377
if (!isset($this->cacheParentIdsByChild[$cacheKey])) {
374378
$this->cacheParentIdsByChild[$cacheKey] = $this->_catalogProductTypeConfigurable->getParentIdsByChild($childId);
375379
}

‎app/code/Magento/ConfigurableProduct/Test/Unit/Model/Plugin/ProductIdentitiesExtenderTest.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
use PHPUnit\Framework\MockObject\MockObject;
1515
use PHPUnit\Framework\TestCase;
1616

17+
/**
18+
* Unit test for Magento\ConfigurableProduct\Model\Plugin\ProductIdentitiesExtender class.
19+
*/
1720
class ProductIdentitiesExtenderTest extends TestCase
1821
{
1922
/**
@@ -31,6 +34,9 @@ class ProductIdentitiesExtenderTest extends TestCase
3134
*/
3235
private $plugin;
3336

37+
/**
38+
* @inheritdoc
39+
*/
3440
protected function setUp(): void
3541
{
3642
$this->configurableTypeMock = $this->getMockBuilder(Configurable::class)
@@ -42,6 +48,11 @@ protected function setUp(): void
4248
$this->plugin = new ProductIdentitiesExtender($this->configurableTypeMock, $this->productRepositoryMock);
4349
}
4450

51+
/**
52+
* Verify after get identities
53+
*
54+
* @return void
55+
*/
4556
public function testAfterGetIdentities()
4657
{
4758
$productId = 1;

‎app/code/Magento/ConfigurableProduct/Test/Unit/Model/Product/Type/ConfigurableTest.php

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable\Attribute\Collection;
2222
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable\Attribute\CollectionFactory;
2323
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable\Product\Collection as ProductCollection;
24+
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable as TypeConfigurable;
2425
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\Configurable\Product\CollectionFactory
2526
as ProductCollectionFactory;
2627
use Magento\ConfigurableProduct\Model\ResourceModel\Product\Type\ConfigurableFactory;
@@ -45,6 +46,8 @@
4546
use Psr\Log\LoggerInterface;
4647

4748
/**
49+
* Unit tests for Magento\ConfigurableProduct\Model\Product\Type\Configurable class.
50+
*
4851
* @SuppressWarnings(PHPMD.LongVariable)
4952
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
5053
* @SuppressWarnings(PHPMD.TooManyFields)
@@ -146,6 +149,12 @@ class ConfigurableTest extends TestCase
146149
private $catalogConfig;
147150

148151
/**
152+
* @var TypeConfigurable
153+
*/
154+
private $catalogProductTypeConfigurable;
155+
156+
/**
157+
* @inheritdoc
149158
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
150159
*/
151160
protected function setUp(): void
@@ -215,6 +224,10 @@ protected function setUp(): void
215224
->setMethods(['create'])
216225
->disableOriginalConstructor()
217226
->getMock();
227+
$this->catalogProductTypeConfigurable = $this->getMockBuilder(TypeConfigurable::class)
228+
->onlyMethods(['getParentIdsByChild'])
229+
->disableOriginalConstructor()
230+
->getMock();
218231

219232
$this->salableProcessor = $this->createMock(SalableProcessor::class);
220233

@@ -242,6 +255,7 @@ protected function setUp(): void
242255
'salableProcessor' => $this->salableProcessor,
243256
'metadataPool' => $this->metadataPool,
244257
'productFactory' => $this->productFactory,
258+
'catalogProductTypeConfigurable' => $this->catalogProductTypeConfigurable,
245259
]
246260
);
247261
$refClass = new \ReflectionClass(Configurable::class);
@@ -250,6 +264,36 @@ protected function setUp(): void
250264
$refProperty->setValue($this->model, $this->metadataPool);
251265
}
252266

267+
/**
268+
* Verify get parent ids by child
269+
*
270+
* @return void
271+
*/
272+
public function testGetParentIdsByChild()
273+
{
274+
$this->assertClassHasAttribute('cacheParentIdsByChild', Configurable::class);
275+
276+
$childId = 1;
277+
$parentIdsByChild = [
278+
[
279+
0 => [
280+
26 => "26",
281+
39 => "39",
282+
],
283+
],
284+
];
285+
286+
$this->catalogProductTypeConfigurable->expects($this->once())
287+
->method('getParentIdsByChild')
288+
->with($childId)
289+
->willReturn($parentIdsByChild);
290+
$this->assertIsArray($this->model->getParentIdsByChild($childId));
291+
292+
$this->catalogProductTypeConfigurable->expects($this->never())
293+
->method('getParentIdsByChild');
294+
$this->assertEquals($parentIdsByChild, $this->model->getParentIdsByChild($childId));
295+
}
296+
253297
public function testHasWeightTrue()
254298
{
255299
$this->assertTrue($this->model->hasWeight(), 'This product has not weight, but it should');

0 commit comments

Comments
 (0)