Improve product loading in commerce_product_reference_entity_view()

The patch is making use of the fact that we only need to load all products for a product display if there's a module implementing hook_commerce_product_reference_default_delta_alter(). So the patch first checks this and if there's no implementation for this hook, it loads only the first product.

+++ b/modules/product_reference/commerce_product_reference.module
@@ -310,6 +310,96 @@ function commerce_product_reference_commerce_product_delete($product) {
+      if (!empty($product) && $product->status == 0) {

I see this is coming from the original code but in general should not this actually be an entity_access() check and not only a status check?

czigor, I think that'd be a good follow up. to check on the direct logic of ->status ==0

Fixing some coding standards and comment issues in the patch.

The patch should also replace commerce_product_reference_default_product() with commerce_product_reference_get_default_product() in commerce_cart_field_formatter_view().

+++ b/modules/product_reference/commerce_product_reference.api.php
@@ -22,7 +22,9 @@
 function hook_commerce_product_reference_default_delta_alter(&$delta, $products) {

+++ b/modules/product_reference/commerce_product_reference.module
@@ -310,6 +310,135 @@ function commerce_product_reference_commerce_product_delete($product) {
+  $product_id = current(array_keys($result['commerce_product']));

This could be replaced by
$product_id = key($result['commerce_product']);

I haven't tested it, but the code looks good.
However, I'm not sure we need to define a new API function for getting the "First product", commerce_product_reference_get_first_product() is only called once and not used anywhere else.

I think there are a few issues with the existing patch.

• It breaks current behavior. When all products are disabled, the price for the first disabled product is still displayed by purpose. From the user's perspective there is an indicative price and the "Add to cart form" says "Product not available". This is currently the case for both single-value and multi-value fields. It is not the case any longer with the patch.
• The field formatter for the "Add to cart form" still loads all products.
• Logical inconsistencies between the case where there are modules implementing "hook_commerce_product_reference_default_delta_alter" and the case where there are not. Specifically, single-value fields are treated differently, in some cases products are not filtered for active status while others do, and determining the default product and allowing the modules to alter it are put in different order.

I've created a new patch that hopefully resolves these issues.

I've created a new variable that allows the site owner to use only active products for the purpose, but it keeps the current behavior by default i.e. prefer an active product but use a disabled one if all products are disabled.

Please, review & test, and if this approach seems appropriate I can create an admin page for managing the new variable.

@jsacksick I've kept that function and added a new one as well, even if they are used only once because they definitely improve readability. They could be prefixed by an _ and considered for internal use only instead of API?

Marked #2747667: Respect default product in commerce_cart_field_attach_view_alter() as duplicate.

Overall #26 looks very good to me. Adding some minor comment fixes and testing this further.

We've been using #12 for over a year now in production. #28 (which is essentially the same as #26, which in turn is a better version of #12) also works for us. I think this would be an important addition to 1.14.

Reroll for latest dev.

I've reviewed the latest patch #30 and cleaned up a few minor items:

1. In commerce_product_reference_get_default_product() for fields with cardinality=1, we look up the product id using commerce_product_reference_get_first_product_id() just as we do for multi-cardinality fields. But then instead of checking whether the product id is null, we return the value of commerce_product_load(), which will be FALSE. Made a minor change make the single cardinality logic better match that of multi-value fields and to return NULL instead of FALSE.

2. In patch #26, the commerce_product_reference_get_first_product_delta_from_list() function was added. However, this function is only called once and only in the case of modules implementing the now-deprecated hook: hook_commerce_product_reference_default_delta_alter(). So I've removed this function and moved the code back to commerce_product_reference_get_default_product(), to keep things a little simpler.

3. In commerce_product_reference_entity_view(), we had:

// If no product is returned, continue to the next field.
if (!$product) {
  continue;
}

right before:

// If we found a product.
if (!empty($product)) {

And nothing after the conditional clause in the loop, so the excess condition was redundant and has now been removed.

4. Added a deprecated notice to commerce_product_reference_default_product() since it is deprecated along with hook_commerce_product_reference_default_product_id_alter()

Unfortunately, this patch changes the current behavior and cannot be committed as is. The problem lies in the new function commerce_product_reference_get_first_product_id(), because it determines the "first" active product without actually maintaining the sort order of the field value array. Instead of "first" being the first value of the field, "first" now becomes the lowest product ID.

You can confirm this by creating a multi-value product display and making the first value something other than the product with the lowest ID. In Commerce 1.x currently, whatever you put first should be the default, but after this patch, it always remains the lowest ID'ed product.

No quick solution, but I imagine a join could be added to the query in that function so the results could be sorted by delta value. Attached patch is just a reroll to ensure it applies after adding the new button text parameter to the form.

Of course I missed the attachment. : P

Added the join to the query.

->fetchField() does not return an array, so let's try it this way.

Suspiciously small patch size, try again.

+++ b/modules/product_reference/commerce_product_reference.module
@@ -309,11 +332,214 @@ function commerce_product_reference_commerce_product_delete($product) {
+  $options = array_merge(
+    array('status' => COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ACTIVE_PREFERRED),

Why not using += here?
Something like:

$options += array(
  'status' => COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ACTIVE_PREFERRED,
);

+++ b/modules/product_reference/commerce_product_reference.module
@@ -309,11 +332,214 @@ function commerce_product_reference_commerce_product_delete($product) {
+  $reference_table = reset($reference_table);

IMO it is safe to assume field_data_$field_name and there is also an API function for that:

_field_sql_storage_tablename (I know it's internal so we could just hardcode field_data_$field_name.

It's very unlikely to change after all these years I guess :).

Note that we could also use _field_sql_storage_columnname().

I'm referring to the $reference_ lines combined with the reset calls.

I haven't actually tested the patch, but by reading it, I wonder why COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ANY isn't the default method to not break the current behavior?

Also, I'd expect our calls to commerce_product_reference_get_default_product() to not pass the last parameter $options, but instead pull the value from the variable inside commerce_product_reference_get_default_product() (at the very beginning, where we provide the default options.

Something like:

$options += array(
  'status' => variable_get('commerce_product_reference_default_product_behavior', ..),
);

Without this change, if I'm calling commerce_product_reference_get_default_product(), the default behavior is hardcoded and isn't going to use the one actually set in the variable (it'll always be "COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ACTIVE_PREFERRED" with the latest patch)..

Made fixes according last @jsacksick's comments. Also I've added break; to find first not last active product in backward compatibility for hook_commerce_product_reference_default_delta_alter

COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ANY isn't the default method to not break the current behavior?

I think @jsacksick is right. I've used those value as default.

If I'm reading the patch right, there's one other place in commerce_product_reference_entity_view() we need to adjust the default variable value to. fwiw, I think we can condense that back to one line ... the split lines probably made it harder to detect.

Also, can we double check the current behavior against the default constant? In my local site, with 3 products referenced from a display, if I disable the first, then the second one becomes the default product for the sake of building the form. Wouldn't it still be the first, disabled product if we switch this to _ANY?

@rszrama I've improved the last patch by changing default value for 'commerce_product_reference_default_product_behavior' with COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ACTIVE_PREFERRED.

Also after additional testing I noticed that when we use COMMERCE_PRODUCT_REFERENCE_DEFAULT_PRODUCT_ANY and use for example 3 referenced product in one field, first product with Disabled status. Then display shows correct price of correct default product, but it absent in select widget of the Add to cart form and button of the form is active. I've fixed it. Maybe we will need to improve/fix tests for add to cart form.

How to test:
1. Create several products. Create node with product reference field and add products to the field.
Set 'Disabled' status for first product.
Go to view page of the node. It should display price of first found active product and it should be selected in field "Product" on "Add to cart"
form and button 'Add to cart should be active.'

2. Add $conf['commerce_product_reference_default_product_behavior'] = 0; to settings.php or use command 'drush vset
'commerce_product_reference_default_product_behavior' 0'.
Re-fresh the node view page and now it should display first product but with disabled button 'Product not available' on "Add to cart" form.

3. Change value of 'commerce_product_reference_default_product_behavior' to 2.
Re-fresh the node view page and now it should show first active product, select widget should contain only active products.

Tested. Patch from comment #46 works as expected.

Upon review, I've realized that the last patch actually changed the default behavior with respect to the Add to Cart form. Whereas previously, no disabled products would appear in the Add to Cart form, with the patch, disabled products would reappear, breaking a significant number of use cases for retaining disabled product references but actually hiding them from the Add to Cart form (e.g. limited products, one-offs, outdated variations that couldn't be deleted for backwards compatibility reasons, etc.).

I thought about creating "yet another variable" for differentiating the default product logic from the Add to Cart form behavior, but I just don't see any good reason to. We're trying to solve for performance in here, not invent new features. 😅

I simply removed the first hunk of the patch to resolve this.

I also realized that the $query->innerJoin() that was added in commerce_product_reference_get_first_product_id() was insufficient. If I reordered my field values, I was still getting the incorrect default. This join was only joining on the target product ID matching but also needed to require a matching source entity type, ID, and revision.

I updated the function accordingly and tested the crap outta this.

Finally, I added documentation with the constants indicating how to adjust the behavior for default product selection and why option 0 may produce inconsistencies on Add to Cart forms. Sites that want to use option 0 will need to make their own accommodations on the Add to Cart form.

And committed! Thanks everyone for all your help. 😊