12073 product repo get different sku case #14019

josh-carter · 2018-03-08T23:54:37Z

Description

Take an example Product with SKU test_sku

In ProductRepository currently, if we're using get() and pass the SKU TesT_SKu we would get an undefined index as described inside #12073.

However It makes sense to be able to retrieve product using different cased SKU as the database is case insensitive and SKU is unique. But the PHP is case sensitive, therefore, to fix the problem we can lowercase the SKU in the PHP code.

Fixed Issues (if relevant)

magento/magento2#12073: ProductRepository fails to load product with camelcase SKU

Manual testing scenarios

Create Product with SKU test_sku
Make a call to ProductRepository::get('tESt_sKu')
Successfully retrieve created Product

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

- Then use lowercase sku in get() method which references the instances array via array key this way we make sure no matter which case is used to get the product we get it correctly - This makes sense as the DB is case insensitive but the PHP is not and currently would throw undefined index if trying to get product with sku: test and passing tESt to get()

orlangur · 2018-03-09T12:37:05Z

Hi @josh-carter, putting this PR on hold as hope to get fix done in scope of #12169 (slightly different approach: product is cached exactly with the same SKU how it was requested instead of strtolower call as it is done in this PR).

josh-carter · 2018-03-09T12:39:09Z

@orlangur Ok cool, my thoughts would be, it saves multiple loading of the same product and keeps functionality correct.

orlangur

Hi @josh-carter, let's continue in this PR :)

Please rework implementation of cacheProduct in cb3b5d1#diff-ff4893a5628d34910295bae32fbcddfdL241 so that code behavior remain unchanged.

Just change implementation of cacheProduct in such a way that we pass both productId and sku instead of obtaining them from product instance.

Another case is that MySQL does not distinguish not just lower/upper case but also е and ё, o and ö, a and ã and so on. Thus there is no need to complicate implementation with strtolower or other approaches for key normalization.

josh-carter · 2018-03-20T12:09:40Z

@orlangur Ok that's cool with me. I'll get it updated and get this ready for review.

- Lets us pass the searched for SKU and use as instance key

- This is no longer called everytime we call the get() method because we pass the sku searched for when using the cacheProduct method()

- New line at end of ProductRepository - Remove unused local variable in ProductRepositoryTest

orlangur · 2018-03-27T08:46:47Z

app/code/Magento/Catalog/Model/ProductRepository.php

     * @return void
     */
-    private function cacheProduct($cacheKey, \Magento\Catalog\Api\Data\ProductInterface $product)
+    private function cacheProduct($cacheKey, \Magento\Catalog\Api\Data\ProductInterface $product, $sku = null)


See: cb3b5d1#diff-ff4893a5628d34910295bae32fbcddfdL241

Please pass productId instead of product instance in both cases. Don't add default parameter and some logic into cacheProduct, pass both product ID and SKU.

- Also add expected methods to unit tests

orlangur · 2018-03-28T09:39:21Z

app/code/Magento/Catalog/Test/Unit/Model/ProductRepositoryTest.php

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

    public function testGetBySkuWithSpace()


This test does not seem to test initial case anymore. Please change so that it actually checks trimmed SKU.

orlangur · 2018-03-28T09:40:48Z

app/code/Magento/Catalog/Test/Unit/Model/ProductRepositoryTest.php

+        $this->resourceModelMock->expects($this->once())->method('getIdBySku')->with($differentCasedSku)
+            ->will($this->returnValue('test_id'));
+        $this->productMock->expects($this->once())->method('load')->with('test_id');
+        $this->productMock->expects($this->once())->method('getId')->willReturn('test_id');


Same here: it looks like we don't check getting product by 'test_sku' anymore.

ibecmattb · 2018-04-17T15:07:44Z

Facing this same issue. How would I go about patching a Magento 2.2.3 install to fix this?

ishakhsuvarov

I would suggest covering this with different type of test, for instance api-functional.

sidolov · 2018-06-18T14:48:36Z

@josh-carter , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for collaboration!

orlangur · 2018-06-18T14:56:05Z

Sorry @josh-carter, my bad, I was supposed to help you with test here, I remember we started a talk in Slack. Will do it this week.

ishakhsuvarov · 2018-06-19T13:54:31Z

@orlangur I'm reopening then, to keep track of it

magento-cicd2 · 2018-06-19T13:54:38Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

orlangur

Nowadays all changes must be applied to 2.3-develop first (see https://devdocs.magento.com/guides/v2.3/contributor-guide/contributing.html#rules). Please prepare a new pull request, this one should be on hold until the latter is merged.

orlangur · 2018-11-19T09:31:49Z

Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on this pull request and it will be reopened.
Thanks for collaboration!

josh-carter added 2 commits March 8, 2018 23:39

Add Test to ProductRepositoryTest to test get with different cased sku

4b62676

josh-carter mentioned this pull request Mar 9, 2018

ProductRepository fails to load product with camelcase SKU #12073

Closed

orlangur self-assigned this Mar 9, 2018

orlangur added the Progress: on hold label Mar 9, 2018

orlangur requested changes Mar 14, 2018

View reviewed changes

orlangur added Progress: needs update and removed Progress: on hold labels Mar 14, 2018

josh-carter added 4 commits March 26, 2018 17:21

Revert lowecase logic introduced in previous commit

4d6fac0

Add optional Sku parameter in cacheProduct function

76bb145

- Lets us pass the searched for SKU and use as instance key

Fix Unit Tests - Remove Unused getSku() expectated calls

89fc9b2

- This is no longer called everytime we call the get() method because we pass the sku searched for when using the cacheProduct method()

Fix Static tests

a020305

- New line at end of ProductRepository - Remove unused local variable in ProductRepositoryTest

orlangur requested changes Mar 27, 2018

View reviewed changes

josh-carter added 3 commits March 27, 2018 14:00

Alter cacheProduct method to always pass ID and SKU

286b858

- Also add expected methods to unit tests

Fix static tests move cacheProduct function to multiline arguments

032f141

Fix Unit tests make sure getSku methods will return sku string

c8c8b09

orlangur requested changes Mar 28, 2018

View reviewed changes

ishakhsuvarov requested changes Apr 26, 2018

View reviewed changes

sidolov closed this Jun 18, 2018

orlangur added this to the Release: 2.2.6 milestone Jun 18, 2018

ishakhsuvarov reopened this Jun 19, 2018

orlangur mentioned this pull request Aug 2, 2018

12073: ProductRepository fails to load product with camelcase SKU #12169

Closed

4 tasks

sidolov modified the milestones: Release: 2.2.6, Release: 2.2.7 Sep 13, 2018

sidolov added the Release Line: 2.2 label Sep 14, 2018

orlangur requested changes Nov 7, 2018

View reviewed changes

orlangur added the Progress: on hold label Nov 7, 2018

orlangur closed this Nov 19, 2018

12073 product repo get different sku case #14019

12073 product repo get different sku case #14019

Uh oh!

Conversation

josh-carter commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

orlangur commented Mar 9, 2018

Uh oh!

josh-carter commented Mar 9, 2018

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

josh-carter commented Mar 20, 2018

Uh oh!

orlangur Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

orlangur Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

orlangur Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

ibecmattb commented Apr 17, 2018

Uh oh!

ishakhsuvarov left a comment

Choose a reason for hiding this comment

Uh oh!

sidolov commented Jun 18, 2018

Uh oh!

orlangur commented Jun 18, 2018

Uh oh!

ishakhsuvarov commented Jun 19, 2018

Uh oh!

magento-cicd2 commented Jun 19, 2018

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

orlangur commented Nov 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

josh-carter commented Mar 8, 2018 •

edited

Loading