Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions app/code/Magento/Catalog/Model/ProductRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ public function get($sku, $editMode = false, $storeId = null, $forceReload = fal
if (!isset($this->instances[$sku])) {
$sku = trim($sku);
}

if (!isset($this->instances[$sku])) {
throw new NoSuchEntityException(__('Requested product doesn\'t exist'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This code completely duplicates previous condition.

What is this issue all about? I see a trim but don't see any strtolower in logic like $this->instances[$product->getSku()][$cacheKey] = $product;.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur, ok, let me explain. We have product with sku "Test". We try to use "get" method of repository to get product with sku "test". Method resourceModel->getIdBySku($sku) will return to us id of "Test" and in $this->instances it put the sku "Test". So after trim we are checking if we have $this->instances["test"] and we don't have it, because we have $this->instances["Test"].

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur, what how? Trimming isn't my logic). It was before my changes. I only check if product sku("Test") is the same as requested sku("test"). And if it isn't the same exception will be thrown.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let me reword. Why product is stored with lower-cased SKU in the first place?

We must find a root cause so that exception will be thrown from line 240 instead of adding more exceptions throwing.

}

return $this->instances[$sku][$cacheKey];
}

Expand Down
19 changes: 19 additions & 0 deletions app/code/Magento/Catalog/Test/Unit/Model/ProductRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,25 @@ public function testGetBySkuFromCacheInitializedInGetById()
$this->assertEquals($this->productMock, $this->model->get($productSku));
}

/**
* @expectedException \Magento\Framework\Exception\NoSuchEntityException
*/
public function testGetBySkuWithCamelCaseException()
{
$productId = 22;
$fakeProductSku = 'testproduct';
$realProductSku = 'testProduct';
$this->productFactoryMock->expects($this->once())->method('create')
->will($this->returnValue($this->productMock));
$this->resourceModelMock->expects($this->once())->method('getIdBySku')
->with($fakeProductSku)->willReturn($productId);
$this->productMock->expects($this->once())->method('load')->with($productId);
$this->productMock->expects($this->atLeastOnce())->method('getId')->willReturn($productId);
$this->productMock->expects($this->once())->method('getSku')->willReturn($realProductSku);

$this->assertEquals($this->productMock, $this->model->get($fakeProductSku));
}

public function testSaveExisting()
{
$this->storeManagerMock->expects($this->any())->method('getWebsites')->willReturn([1 => 'default']);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Catalog\Model;

use Magento\TestFramework\Helper\Bootstrap;

/**
* Test for Magento\Catalog\Model\ProductRepository
*/
class ProductRepositoryTest extends \PHPUnit\Framework\TestCase
{
/**
* @var ProductRepository
*/
protected $model;

protected function setUp()
{
$this->model = Bootstrap::getObjectManager()->create(ProductRepository::class);
}

/**
* @magentoDataFixture Magento/Catalog/_files/product_simple_camel_case_sku.php
* @expectedException \Magento\Framework\Exception\NoSuchEntityException
*/
public function testGet()
{
$product = $this->model->get('camelcaseproduct');

$this->assertEquals('CamelCaseProduct', $product->getSku());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

\Magento\TestFramework\Helper\Bootstrap::getInstance()->reinitialize();

/** @var \Magento\TestFramework\ObjectManager $objectManager */
$objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();

/** @var $product \Magento\Catalog\Model\Product */
$product = $objectManager->create(\Magento\Catalog\Model\Product::class);
$product->isObjectNew(true);
$product->setTypeId(\Magento\Catalog\Model\Product\Type::TYPE_SIMPLE)
->setId(1)
->setAttributeSetId(4)
->setWebsiteIds([1])
->setName('Simple Product')
->setSku('CamelCaseProduct')
->setPrice(10)
->setWeight(1)
->setShortDescription("Short description")
->setTaxClassId(0)
->setDescription('Description with <b>html tag</b>')
->setMetaTitle('meta title')
->setMetaKeyword('meta keyword')
->setMetaDescription('meta description')
->setVisibility(\Magento\Catalog\Model\Product\Visibility::VISIBILITY_BOTH)
->setStatus(\Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_ENABLED)
->setStockData(
[
'use_config_manage_stock' => 1,
'qty' => 100,
'is_qty_decimal' => 0,
'is_in_stock' => 1,
]
);

/** @var \Magento\Catalog\Api\ProductRepositoryInterface $productRepositoryFactory */
$productRepository = $objectManager->create(\Magento\Catalog\Api\ProductRepositoryInterface::class);
$productRepository->save($product);
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

use Magento\Framework\Exception\NoSuchEntityException;

\Magento\TestFramework\Helper\Bootstrap::getInstance()->getInstance()->reinitialize();

/** @var \Magento\Framework\Registry $registry */
$registry = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(\Magento\Framework\Registry::class);

$registry->unregister('isSecureArea');
$registry->register('isSecureArea', true);

/** @var \Magento\Catalog\Api\ProductRepositoryInterface $productRepository */
$productRepository = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()
->get(\Magento\Catalog\Api\ProductRepositoryInterface::class);
try {
$product = $productRepository->get('CamelCaseProduct', false, null, true);
$productRepository->delete($product);
} catch (NoSuchEntityException $e) {
}
$registry->unregister('isSecureArea');
$registry->register('isSecureArea', false);