commerce_product_reference_entity_view() calls some functions multiple times with the same input

Giving it a test run with CK2's Behat https://travis-ci.org/mglaman/commerce_kickstart/builds/97034950

This looks good to me. Tests pass on CK2, manually reviewed and found no issues. Moving calls out of loops that don't need to be is always a win.

Is this two patches combined in one? Didn't see an explanation for the switch in wrapper usage - and it's kinda funky to see something like this:

  $products = $product;

Ok, I'll review. I was originally under the impression the real trick here was moving drupal_get_path() out of the foreach, but I see it's also these other function calls, including not invoking the wrapper in the foreach. I'll probably clean up the variable names before committing.

Courtesy CK2 Behat test run https://travis-ci.org/mglaman/commerce_kickstart/builds/97526318

Ok, think I wrapped my head around all the changes. Two observations:

1. I've further differentiated $products from $product by using $reference_field_value instead of $products. In retrospect, I kinda wished I'd used $default_product throughout here, but this is the simpler fix. (I suppose it's not too late, but that's for a follow-up patch. : )
2. We can move the check for an empty field value up above the call to entity_metadata_wrapper(), potentially saving more time.
3. The patch as written introduces a regression where no default product will be selected in the event that all referenced products on an entity are disabled. The current behavior is to still use the normal default product if all referenced products are disabled, so I've reverted to that.

Patch attached.

Attached interdiff of #13 to #15

C'mon testbot!

+++ b/modules/product_reference/commerce_product_reference.module
@@ -325,6 +325,9 @@ function commerce_product_reference_entity_view($entity, $entity_type, $view_mod
+  // Resolve the product module path now to avoid repeating it in the loop.
+  $product_module_path = drupal_get_path('module', 'commerce_product');

Patch looks good to me, only one tiny nitpick, this comment really only makes sense in the context of this patch, once this is committed, the comment seems unnecessary and slightly confusing, it's not like we explain the scope of every variable we set.

Committed.