ProductRepository fails to load product with camelcase SKU
Closed this issue · 17 comments
Preconditions
- Magento 2.1 | 2.2 | 2.3.0-dev
- PHP 7.1.9
- MySQL 5.7.19
Steps to reproduce
- Create a simple product with the camel case sku (for example "Test")
- Use get() method of \Magento\Catalog\Model\ProductRepository to get this product by sku, and specify the sku in lower case.
Expected result
- NoSuchEntityException is returned
Actual result
- Notice: Undefined index: test in /www/magento2/app/code/Magento/Catalog/Model/ProductRepository.php on line 254
@maximgubar, thank you for your report.
We've created internal ticket(s) MAGETWO-83383 to track progress on the issue.
I can confirm this bug.
It's a regression in 2.2.0, still present as of 2.2.2. The problem was introduced right here: cb3b5d1#diff-ff4893a5628d34910295bae32fbcddfdL241
Specifically, products are cached on ProductRepository by $this->instances[$sku]. Previously, products were loaded and stored using the input value $sku (from ProductRepository->get($sku)). That meant different cases could result in multiple loads, but each load would still work without error.
But now, following the change above, the cache is checked by $sku, but the product is added to it by $product->getSku() (in cacheProduct).
The SQL product load is case-insensitive, but the PHP array key comparison is not. Ergo, the product loads successfully (no NoSuchEntityException exception thrown), but if the case differs, it is not found within $this->instances and you get this undefined index error.
@rhoerr I've just opened a PR to propose a fix for this.
We actually had a similar issue recently surrounding SKU case during Magento 2 MSI development and after a discussion with @maghamed I came across this issue and thought its best we fix it whilst on subject!
What I've proposed is that we take whichever SKU searched for using get() and lowercase it at the top of the function. Then when we're creating the Cache Key for the product and assigning it to the instances array, we change to use strtolower($product->getSku()) as the array key in $this->instances[].
This will fix the undefined index error as we're bringing a common case to the SKU throughout the code.
As you stated:
The SQL product load is case-insensitive, but the PHP array key comparison is not
Therefore this allows us to just use a common lowercase for the SKU searched for and the instances array key.
@magento-engcom-team For me this happens if sku contains space for example "My Sku" so in rest api it is needed to use "/rest/V1/products/My%20Sku" so the url is valid, but magento should decode the url before looking for it in instances cache.
Magento Support has a patch MDVA-10443 to address this issue by adding strtolower() around the SKU on storage to and load from $this->instances, in get() and cacheProduct(). The changes do not appear to be on the development branch for 2.2 or 2.3 at this time.
I've also learned this bug can be replicated through the Magento UI, with no custom code needed: Add a product to an admin order, by SKU, with different capitalization than the actual product SKU.
My fix was to add the following code after the call to $this->cacheProduct($cacheKey, $product); in ProductRepository:
$sku = $product->getSku();
I did this with a custom module by creating a preference for my version of the ProductRepository class.
The 2.2-develop branch has a fix for this properly, but until that goes in, my fix appears to work.
You can get it from https://github.com/indefinitedevil/m2-catalog-fix if you want.
@indefinitedevil I noticed an issue with your module where when I called the REST API endpoint "/rest/all/V1/products" in some circumstances, it would trigger parent class behavior and therefore not have access to some of the dependencies set in your override class.
Simply adding an invocation of the parent constructor seemed to resolve the issue: indefinitedevil/m2-catalog-fix#1
@nlwstein Did rather throw it together in a hurry then replaced it with the patch from support. Thanks for the patch though
@RomaKis if this issue was solved already, could you please properly link it and close it ?
@maximgubar, no, this issue wasn't fixed. My PR was closed.
@magento-engcom-team is this issue not yet fixed ?
@magento-engcom-team the issue can be reproduced with Magento 2.2.5.
Why was this issue closed? Is it fixed?
@victortodoran seems like this fixed in 2.2 and 2.3 branches and will be available on 2.3.1 release 45da022 , In my case sku -> '1234a' and '1234A' returns the same product id
@engcom-backlog-nazar
A product can have only one sku. it's either 1234a either 1234A
Loading by two different skus can not return the same product/product_id
You are describing what is wrong with the feature as a success test.
@victortodoran hmm... seem like this is very difficult to discuss according to #12169, i'm will ask a product owner for this feature