ProductRepository sku cache is corrupted when cacheLimit is reached

[heldchen]

Numeric SKUs are renumbered when array_slice is used to purge parts of the products-by-sku cache array. Per php documentation:

Note that array_slice() will reorder and reset the numeric array indices by default.

PHP considers numeric-looking values like '123' as numeric even when used as array indices. so although the $product->getSku() returns a string, the string might still look like a number and is then renumbered during array_slice.

as $this->instances has the SKUs as keys, the resulting shortened array might contain indexes that now point to a wrong product, which can lead to the ProductRespository returning the wrong product for a given SKU.

Description

Manual testing scenarios

load >1000 products by sku, where the skus are numeric via $productRepository->get($sku);, then observe the $productRepository->instance array

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)

[magento-cicd2]

All committers have signed the CLA.

[heldchen]

Ideally, this pull request should be backported to 2.2, as we were hit pretty hard by this bug during daily product imports for one of our larger 2.2 shop instance.

[orlangur]

Interesting why 76de251 didn't cover this.

@heldchen thanks for your contribution! Basically 2.2-develop is the new default branch for any contribution, 2.3-develop is not used currently (but it is equal to develop). Maybe it is already documented somewhere of will be documented soon.

[heldchen]

@heldchen thanks for your contribution! Basically 2.2-develop is the new default branch for any contribution, 2.3-develop is not used currently (but it is equal to develop). Maybe it is already documented somewhere of will be documented soon.

ah ok, sorry. should I recreate a new pull request for 2.2-develop or can you move it?

Interesting why 76de251 didn't cover this.

it is not throwing an error but silently returning wrong cache entries if you're "lucky" enough.

in our case, we had >1000 articles loaded with high numeric SKUs (like '123456'). then an article with a low numeric SKU (128) was requested in our import script code. the repository by then had sliced & renumbered the instances array and returned a wrong article with a completely different SKU from the cache instead of loading the real 128 one.

[orlangur]

should I recreate a new pull request for 2.2-develop or can you move it?

Let's just process this one into develop and then you can create another one for 2.2-develop.

it is not throwing an error but silently returning wrong cache entries if you're "lucky" enough.

Yeah, I understand this, just there are two array_slice calls in consecutive lines and only one was fixed while it is obvious that both require array keys preserving.

[orlangur]

@heldchen thanks for your contribution! You can now do a backport into 2.2-develop.

[heldchen]

as I'm not familiar with the magento github workflow and don't want to cause trouble: with backport you mean a cherry pick of 3d6f738 into 2.2-develop, right?

[orlangur]

absolutely right.

[heldchen]

#11553 - I hope I did everything correctly, if not, I'd appreciate some pointers to avoid the overhead for future PRs. cheers

[tufahu]

Haha i just wanted to fix it now, we have a shop with all numeric skus ;> thank you @heldchen