The $conditions parameter on the entity loader was deprecated 6 years ago, but for some reason Commerce was never updated to use EntityFieldQuery in its place instead. This has been alright as far as it goes - it seemed the change was largely cosmetic, a means to stop developers from requesting increasingly greater support through the $conditions parameter.

However, within Commerce itself, this causes an issue when product SKUs are near identical - if you attempt to load products by the SKUs '12345' and '012345', the entity loader will return the cached value from the first load to the second load request.

Since this is a deprecated core feature, let's just update all of our functions to drop using that parameter and use EFQs instead. This will result in an extra load, unfortunately, but one thing we can do is update commerce_product_load_by_sku() to first manually check the product cache for pre-loaded products using an identical operator to compare cached SKUs against the requested SKU.

CommentFileSizeAuthor
#6 commerce-test_cases_for_commerce_product_load_by_sku-2790037-6.patch4.67 KBnullkernel
#6 interdiff_3-6.txt1.08 KBnullkernel
#4 commerce-test_cases_for_commerce_product_load_by_sku-2790037-3.patch3.49 KBnullkernel
#4 interdiff_2-3.txt3.12 KBnullkernel
#2 commerce-test_cases_for_commerce_product_load_by_sku-2790037-2.patch2.48 KBnullkernel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama created an issue. See original summary.

nullkernel’s picture

nullkernel CreditAttribution: nullkernel commented
Status: Active » Needs review
FileSize
commerce-test_cases_for_commerce_product_load_by_sku-2790037-2.patch2.48 KB

I've recently been affected by this bug. So far, I've written test cases that reproduce the problem. Attaching a patch with the test cases.

The patch will fail at this time (due to test failures), but the test cases can be used to prove correctness of commerce_product_load_by_sku() moving forward.

Status: Needs review » Needs work

The last submitted patch, 2: commerce-test_cases_for_commerce_product_load_by_sku-2790037-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nullkernel’s picture

nullkernel CreditAttribution: nullkernel commented
Assigned: Unassigned » nullkernel
Status: Needs work » Needs review
FileSize
commerce-test_cases_for_commerce_product_load_by_sku-2790037-3.patch3.49 KB
interdiff_2-3.txt3.12 KB

The problem isn't limited to static cache. The test failures in my previous patch happened with static cache bypassed ($reset parameter set to TRUE). The entity loader is basically generating "sku IN" SQL, and does not use quotes when the SKU is an integer. At least for MySQL, the following queries have different results:

# Unexpectedly returns multiple results, e.g. "1234567A", "1234567", "1234567B", 
SELECT * FROM commerce_product WHERE sku IN (1234567);

# As expected, returns only the product with sku "1234567".
SELECT * FROM commerce_product WHERE sku IN ('1234567');

I've updated my testcases patch and attached it along with an interdiff. This patch will generate more failures with the current codebase. Key changes include:

  1. Include passes using static cache and not using static cache.
  2. Added a few more test products based on example in this issue's description (i.e. '012345').
  3. Testing loading products with both $sku (which can be an integer) and $string_sku.
  4. Updated to have more flexible messages (indicating when the SKU being passed is an integer or a string, and if static cache is being used).
  5. Fixed a couple of coding standards warnings.

Using EntityFieldQuery instead of entity loader $conditions should resolve these test failures. Careful matching of the cache key also needs to happen.

The last submitted patch, 4: commerce-test_cases_for_commerce_product_load_by_sku-2790037-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nullkernel’s picture

nullkernel CreditAttribution: nullkernel commented
FileSize
interdiff_3-6.txt1.08 KB
commerce-test_cases_for_commerce_product_load_by_sku-2790037-6.patch4.67 KB

I've patched function commerce_product_load_by_sku() to use EntityFieldQuery instead of commerce_product_load_multiple(). All test cases should pass now. When testing locally, all 72 out of 72 assertions pass. With the existing implementation there were 15 failed assertions with the patch I uploaded in my previous comment (#4). I realize I got my comment number wrong in my last patch's filename - sorry about that.

Status: Needs review » Needs work

The last submitted patch, 6: commerce-test_cases_for_commerce_product_load_by_sku-2790037-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.