From 8e2959de1740f78ba9a57aa0e21e23ecca79dcc6 Mon Sep 17 00:00:00 2001 From: Jeroen Date: Mon, 2 Dec 2019 12:25:28 +0100 Subject: [PATCH] Prevent endless loop when duplicating product --- .../Magento/Catalog/Model/Product/Copier.php | 81 ++++---- .../Test/Unit/Model/Product/CopierTest.php | 177 +++++++++++++++--- 2 files changed, 185 insertions(+), 73 deletions(-) diff --git a/app/code/Magento/Catalog/Model/Product/Copier.php b/app/code/Magento/Catalog/Model/Product/Copier.php index a7f7bad1a5167..d10bf085a4a25 100644 --- a/app/code/Magento/Catalog/Model/Product/Copier.php +++ b/app/code/Magento/Catalog/Model/Product/Copier.php @@ -8,7 +8,11 @@ use Magento\Catalog\Api\Data\ProductInterface; use Magento\Catalog\Model\Attribute\ScopeOverriddenValue; use Magento\Catalog\Model\Product; +use Magento\Catalog\Model\Product\Option\Repository as OptionRepository; use Magento\Catalog\Model\ProductFactory; +use Magento\Framework\App\ObjectManager; +use Magento\Framework\EntityManager\MetadataPool; +use Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException; /** * Catalog product copier. @@ -35,9 +39,10 @@ class Copier protected $productFactory; /** - * @var \Magento\Framework\EntityManager\MetadataPool + * @var MetadataPool */ protected $metadataPool; + /** * @var ScopeOverriddenValue */ @@ -47,30 +52,38 @@ class Copier * @param CopyConstructorInterface $copyConstructor * @param ProductFactory $productFactory * @param ScopeOverriddenValue $scopeOverriddenValue + * @param OptionRepository|null $optionRepository + * @param MetadataPool|null $metadataPool */ public function __construct( CopyConstructorInterface $copyConstructor, ProductFactory $productFactory, - ScopeOverriddenValue $scopeOverriddenValue + ScopeOverriddenValue $scopeOverriddenValue, + OptionRepository $optionRepository = null, + MetadataPool $metadataPool = null ) { $this->productFactory = $productFactory; $this->copyConstructor = $copyConstructor; $this->scopeOverriddenValue = $scopeOverriddenValue; + $this->optionRepository = $optionRepository ?: ObjectManager::getInstance()->get(OptionRepository::class); + $this->metadataPool = $metadataPool ?: ObjectManager::getInstance()->get(MetadataPool::class); } /** * Create product duplicate * * @param \Magento\Catalog\Model\Product $product + * * @return \Magento\Catalog\Model\Product + * + * @throws \Exception */ public function copy(Product $product) { $product->getWebsiteIds(); $product->getCategoryIds(); - /** @var \Magento\Framework\EntityManager\EntityMetadataInterface $metadata */ - $metadata = $this->getMetadataPool()->getMetadata(ProductInterface::class); + $metadata = $this->metadataPool->getMetadata(ProductInterface::class); /** @var \Magento\Catalog\Model\Product $duplicate */ $duplicate = $this->productFactory->create(); @@ -88,7 +101,7 @@ public function copy(Product $product) $this->copyConstructor->build($product, $duplicate); $this->setDefaultUrl($product, $duplicate); $this->setStoresUrl($product, $duplicate); - $this->getOptionRepository()->duplicate($product, $duplicate); + $this->optionRepository->duplicate($product, $duplicate); $product->getResource()->duplicate( $product->getData($metadata->getLinkField()), $duplicate->getData($metadata->getLinkField()) @@ -123,13 +136,16 @@ private function setDefaultUrl(Product $product, Product $duplicate) : void * * @param Product $product * @param Product $duplicate + * * @return void + * @throws UrlAlreadyExistsException */ private function setStoresUrl(Product $product, Product $duplicate) : void { $storeIds = $duplicate->getStoreIds(); $productId = $product->getId(); $productResource = $product->getResource(); + $attribute = $productResource->getAttribute('url_key'); $duplicate->setData('save_rewrites_history', false); foreach ($storeIds as $storeId) { $useDefault = !$this->scopeOverriddenValue->containsValue( @@ -141,20 +157,23 @@ private function setStoresUrl(Product $product, Product $duplicate) : void if ($useDefault) { continue; } - $isDuplicateSaved = false; + $duplicate->setStoreId($storeId); $urlKey = $productResource->getAttributeRawValue($productId, 'url_key', $storeId); + $iteration = 0; + do { + if ($iteration === 10) { + throw new UrlAlreadyExistsException(); + } + $urlKey = $this->modifyUrl($urlKey); $duplicate->setUrlKey($urlKey); - $duplicate->setData('url_path', null); - try { - $duplicate->save(); - $isDuplicateSaved = true; - // phpcs:ignore Magento2.CodeAnalysis.EmptyBlock - } catch (\Magento\Framework\Exception\AlreadyExistsException $e) { - } - } while (!$isDuplicateSaved); + $iteration++; + } while (!$attribute->getEntity()->checkAttributeUniqueValue($attribute, $duplicate)); + $duplicate->setData('url_path', null); + $productResource->saveAttribute($duplicate, 'url_path'); + $productResource->saveAttribute($duplicate, 'url_key'); } $duplicate->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID); } @@ -168,38 +187,8 @@ private function setStoresUrl(Product $product, Product $duplicate) : void private function modifyUrl(string $urlKey) : string { return preg_match('/(.*)-(\d+)$/', $urlKey, $matches) - ? $matches[1] . '-' . ($matches[2] + 1) - : $urlKey . '-1'; - } - - /** - * Returns product option repository. - * - * @return Option\Repository - * @deprecated 101.0.0 - */ - private function getOptionRepository() - { - if (null === $this->optionRepository) { - $this->optionRepository = \Magento\Framework\App\ObjectManager::getInstance() - ->get(\Magento\Catalog\Model\Product\Option\Repository::class); - } - return $this->optionRepository; - } - - /** - * Returns metadata pool. - * - * @return \Magento\Framework\EntityManager\MetadataPool - * @deprecated 101.0.0 - */ - private function getMetadataPool() - { - if (null === $this->metadataPool) { - $this->metadataPool = \Magento\Framework\App\ObjectManager::getInstance() - ->get(\Magento\Framework\EntityManager\MetadataPool::class); - } - return $this->metadataPool; + ? $matches[1] . '-' . ($matches[2] + 1) + : $urlKey . '-1'; } /** diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php index 809fa0225278c..1d5abd817deb5 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php @@ -6,7 +6,6 @@ namespace Magento\Catalog\Test\Unit\Model\Product; use Magento\Catalog\Api\Data\ProductInterface; -use Magento\Catalog\Model\Attribute\ScopeOverriddenValue; use Magento\Catalog\Model\Product; use Magento\Catalog\Model\Product\Copier; @@ -40,17 +39,17 @@ class CopierTest extends \PHPUnit\Framework\TestCase /** * @var \PHPUnit_Framework_MockObject_MockObject */ - protected $productMock; + private $scopeOverriddenValueMock; /** * @var \PHPUnit_Framework_MockObject_MockObject */ - protected $metadata; + protected $productMock; /** - * @var ScopeOverriddenValue|\PHPUnit_Framework_MockObject_MockObject + * @var \PHPUnit_Framework_MockObject_MockObject */ - private $scopeOverriddenValue; + protected $metadata; protected function setUp() { @@ -59,13 +58,14 @@ protected function setUp() \Magento\Catalog\Model\ProductFactory::class, ['create'] ); + $this->scopeOverriddenValueMock = $this->createMock( + \Magento\Catalog\Model\Attribute\ScopeOverriddenValue::class + ); $this->optionRepositoryMock = $this->createMock( \Magento\Catalog\Model\Product\Option\Repository::class ); - $this->optionRepositoryMock; $this->productMock = $this->createMock(Product::class); $this->productMock->expects($this->any())->method('getEntityId')->willReturn(1); - $this->scopeOverriddenValue = $this->createMock(ScopeOverriddenValue::class); $this->metadata = $this->getMockBuilder(\Magento\Framework\EntityManager\EntityMetadata::class) ->disableOriginalConstructor() @@ -74,20 +74,16 @@ protected function setUp() ->disableOriginalConstructor() ->getMock(); $metadataPool->expects($this->any())->method('getMetadata')->willReturn($this->metadata); - $this->_model = new Copier( $this->copyConstructorMock, $this->productFactoryMock, - $this->scopeOverriddenValue + $this->scopeOverriddenValueMock ); - $this->setProperties( - $this->_model, - [ - 'optionRepository' => $this->optionRepositoryMock, - 'metadataPool' => $metadataPool, - ] - ); + $this->setProperties($this->_model, [ + 'optionRepository' => $this->optionRepositoryMock, + 'metadataPool' => $metadataPool + ]); } /** @@ -115,12 +111,10 @@ public function testCopy() ]; $this->productMock->expects($this->atLeastOnce())->method('getWebsiteIds'); $this->productMock->expects($this->atLeastOnce())->method('getCategoryIds'); - $this->productMock->expects($this->any())->method('getData')->willReturnMap( - [ - ['', null, $productData], - ['linkField', null, '1'], - ] - ); + $this->productMock->expects($this->any())->method('getData')->willReturnMap([ + ['', null, $productData], + ['linkField', null, '1'], + ]); $entityMock = $this->getMockForAbstractClass( \Magento\Eav\Model\Entity\AbstractEntity::class, @@ -205,11 +199,9 @@ public function testCopy() $this->metadata->expects($this->any())->method('getLinkField')->willReturn('linkField'); - $duplicateMock->expects($this->any())->method('getData')->willReturnMap( - [ - ['linkField', null, '2'], - ] - ); + $duplicateMock->expects($this->any())->method('getData')->willReturnMap([ + ['linkField', null, '2'], + ]); $this->optionRepositoryMock->expects($this->once()) ->method('duplicate') ->with($this->productMock, $duplicateMock); @@ -218,6 +210,137 @@ public function testCopy() $this->assertEquals($duplicateMock, $this->_model->copy($this->productMock)); } + /** + * @SuppressWarnings(PHPMD.ExcessiveMethodLength) + */ + public function testUrlAlreadyExistsExceptionWhileCopyStoresUrl() + { + $stockItem = $this->getMockBuilder(\Magento\CatalogInventory\Api\Data\StockItemInterface::class) + ->getMock(); + $extensionAttributes = $this->getMockBuilder(\Magento\Catalog\Api\Data\ProductExtension::class) + ->setMethods(['getStockItem', 'setData']) + ->getMock(); + $extensionAttributes + ->expects($this->once()) + ->method('getStockItem') + ->willReturn($stockItem); + $extensionAttributes + ->expects($this->once()) + ->method('setData') + ->with('stock_item', null); + + $productData = [ + 'product data' => ['product data'], + ProductInterface::EXTENSION_ATTRIBUTES_KEY => $extensionAttributes, + ]; + $this->productMock->expects($this->atLeastOnce())->method('getWebsiteIds'); + $this->productMock->expects($this->atLeastOnce())->method('getCategoryIds'); + $this->productMock->expects($this->any())->method('getData')->willReturnMap([ + ['', null, $productData], + ['linkField', null, '1'], + ]); + + $entityMock = $this->getMockForAbstractClass( + \Magento\Eav\Model\Entity\AbstractEntity::class, + [], + '', + false, + true, + true, + ['checkAttributeUniqueValue'] + ); + $entityMock->expects($this->exactly(11)) + ->method('checkAttributeUniqueValue') + ->willReturn(true, false); + + $attributeMock = $this->getMockForAbstractClass( + \Magento\Eav\Model\Entity\Attribute\AbstractAttribute::class, + [], + '', + false, + true, + true, + ['getEntity'] + ); + $attributeMock->expects($this->any()) + ->method('getEntity') + ->willReturn($entityMock); + + $resourceMock = $this->getMockBuilder(\Magento\Catalog\Model\ResourceModel\Product::class) + ->disableOriginalConstructor() + ->setMethods(['getAttributeRawValue', 'duplicate', 'getAttribute']) + ->getMock(); + $resourceMock->expects($this->any()) + ->method('getAttributeRawValue') + ->willReturn('urk-key-1'); + $resourceMock->expects($this->any()) + ->method('getAttribute') + ->willReturn($attributeMock); + + $this->productMock->expects($this->any())->method('getResource')->will($this->returnValue($resourceMock)); + + $duplicateMock = $this->createPartialMock( + Product::class, + [ + '__wakeup', + 'setData', + 'setOptions', + 'getData', + 'setIsDuplicate', + 'setOriginalLinkId', + 'setStatus', + 'setCreatedAt', + 'setUpdatedAt', + 'setId', + 'getEntityId', + 'save', + 'setUrlKey', + 'setStoreId', + 'getStoreIds', + ] + ); + $this->productFactoryMock->expects($this->once())->method('create')->will($this->returnValue($duplicateMock)); + + $duplicateMock->expects($this->once())->method('setOptions')->with([]); + $duplicateMock->expects($this->once())->method('setIsDuplicate')->with(true); + $duplicateMock->expects($this->once())->method('setOriginalLinkId')->with(1); + $duplicateMock->expects( + $this->once() + )->method( + 'setStatus' + )->with( + \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_DISABLED + ); + $duplicateMock->expects($this->atLeastOnce())->method('setStoreId'); + $duplicateMock->expects($this->once())->method('setCreatedAt')->with(null); + $duplicateMock->expects($this->once())->method('setUpdatedAt')->with(null); + $duplicateMock->expects($this->once())->method('setId')->with(null); + $duplicateMock->expects($this->atLeastOnce())->method('getStoreIds')->willReturn([1]); + $duplicateMock->expects($this->atLeastOnce())->method('setData')->willReturn($duplicateMock); + $this->copyConstructorMock->expects($this->once())->method('build')->with($this->productMock, $duplicateMock); + $duplicateMock->expects( + $this->exactly(11) + )->method( + 'setUrlKey' + )->with( + $this->stringContains('urk-key-') + )->willReturn( + $duplicateMock + ); + $duplicateMock->expects($this->once())->method('save'); + + $this->scopeOverriddenValueMock->expects($this->once())->method('containsValue')->willReturn(true); + + $this->metadata->expects($this->any())->method('getLinkField')->willReturn('linkField'); + + $duplicateMock->expects($this->any())->method('getData')->willReturnMap([ + ['linkField', null, '2'], + ]); + + $this->expectException(\Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException::class); + $this->_model->copy($this->productMock); + } + /** * @param $object * @param array $properties