$line_item_types = variable_get('commerce_discount_line_item_types', array_diff(commerce_product_line_item_types(), array('product_discount')));

and then 

if (in_array($line_item_wrapper->type->value(), $line_item_types)) {

Does work as a valid check because the FAPI saves an array element as 'shipping' => 0 which will pass the in_array check.

CommentFileSizeAuthor
#9 2640018-9-pass.patch4.6 KBjoelpittet
#9 2640018-9-test-only-fail.patch2.06 KBjoelpittet
#6 commerce_discount-commerce_discount_line_item_types_check-2640018-6.patch1.31 KBdmurphy1
#2 commerce_discount-commerce_discount_line_item_types_check-2640018.patch1.15 KBandyg5000

Comments

andyg5000 created an issue. See original summary.

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Status: Active » Needs review
StatusFileSize
new commerce_discount-commerce_discount_line_item_types_check-2640018.patch1.15 KB

This issue happens because in_array passes whenever there are values of 0 in the array.

$line_item_types = variable_get('commerce_discount_line_item_types', array_diff(commerce_product_line_item_types(), array('product_discount')));
$line_item_types['some_type'] = 0;
if (in_array('type_that_doesnt_exist_in_array', $line_item_types)) {
  dpm(TRUE);
}

Here's a patch that uses empty checks instead of in_array so that we don't have to worry about sites with the array set with 0 values (from FAPI).

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
+++ b/commerce_discount.rules.inc
@@ -924,7 +924,7 @@ function commerce_discount_percentage(EntityDrupalWrapper $wrapper, $discount_na
+        if (!empty($line_item_types[$line_item_wrapper->type->value()])) {

@@ -950,7 +950,7 @@ function commerce_discount_percentage(EntityDrupalWrapper $wrapper, $discount_na
+      if (empty($line_item_types[$wrapper->getBundle()])) {

Won't work in PHP 5.3, return on context or some crazy thing.

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Status: Needs review » Needs work

Crap you're right

The last submitted patch, 2: commerce_discount-commerce_discount_line_item_types_check-2640018.patch, failed testing.

dmurphy1’s picture

dmurphy1 commented
StatusFileSize
new commerce_discount-commerce_discount_line_item_types_check-2640018-6.patch1.31 KB

I ran into the same problem today after I unchecked a few "Line item types to use for discounts" at admin/commerce/discounts/settings. They still passed the in_array check because they're stored as 'shipping' => 0 as mentioned above. I believe simply passing the optional strict parameter set to TRUE for in_array fixes the problem. Per http://php.net/manual/en/function.in-array.php "if the third parameter strict is set to TRUE then the in_array() function will also check the types of the needle in the haystack."

dmurphy1’s picture

dmurphy1 commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Needs tests

Just to confirm you I understand the problem. You are seeing this right?
https://3v4l.org/TjuYO

Would any of you mind writing a test so this doesn't regress in the future?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -Needs tests
StatusFileSize
new 2640018-9-test-only-fail.patch2.06 KB
new 2640018-9-pass.patch4.6 KB

Here's a test only and the fix. I split the variable from getBundle() out because it's easier to debug.

The last submitted patch, 9: 2640018-9-test-only-fail.patch, failed testing.

  • joelpittet committed 061ade2 on 7.x-1.x authored by dmurphy1 
    Issue #2640018 by joelpittet, andyg5000, dmurphy1, mglaman:...
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

Thanks everyone, I've pushed #9 to -dev.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented