Problem/Motivation

Currently there's an option to disable / enable the field_injection globally by using the setting provided by the commerce_product_reference field. (Looks like this was introduced by this commit.)
If field_injection is enabled, all fields are prepared for display in the function commerce_product_reference_entity_view(), regardless if a field is configured as visible or hidden in the "Manage Display" section of the entity (node) where the commerce_product_reference field is located.
I'm wondering if it would be possible to prepare only the fields configured as "visible". In my case this would save quite some processing time because of skipping price calculation.
In my current case (old commerce version, very expensive price calculation) I was able to save 50% processing time by adding the below proposed change.

Proposed resolution

I run on a very old version of commerce ;) Thus for now I just describe the possible change. If you wish I'll create a patch.
Line to act on is 312 of commerce_product_reference.module.
Original:

<?php
foreach (field_info_instances('commerce_product', $product->type) as $product_field_name => $product_field) {
  ...
}
?>

Proposed version:
<?php
foreach (field_info_instances('commerce_product', $product->type) as $product_field_name => $product_field) {
  if (
$product_field['display'][$reference_view_mode]['type'] != 'hidden') {
    ...
  }
?>

Remaining tasks

  • Extensive testing to find possible drawbacks. I couldn't find any so far.

User interface changes

none.

API changes

none.

Files: 
CommentFileSizeAuthor
#6 1352774-6.patch1.98 KBroderik
PASSED: [[SimpleTest]]: [MySQL] 3,570 pass(es).
[ View ]
#3 1352774-3.patch3.14 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 3,546 pass(es).
[ View ]
#2 commerce-exclude-injected-product-fields-from-being-prepared-when-hidden.patch3.42 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 3,511 pass(es).
[ View ]

Comments

As Peter and I discussed in IRC, next step is rolling a patch against -dev and see what testbot answers.

Status:Needs work» Needs review
StatusFileSize
new3.42 KB
PASSED: [[SimpleTest]]: [MySQL] 3,511 pass(es).
[ View ]

Muchas gracias to Pedro for taking the time to discuss this :)

Issue tags:+Performance
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 3,546 pass(es).
[ View ]

The patch from #2 looks like a good performance improvement, but it has one small problem. Actually, it's a bug that affects the entire function, but it's hidden by field_view_field() for regular fields and field_extra_fields_get_display() for extra fields.

At the top of commerce_product_reference_entity_view(), we set
$reference_view_mode = $entity_type . '_' . $view_mode;. Let's assume we're viewing a node page, that gives us $reference_view_mode = 'node_full'.

Now, if we don't have custom settings enabled for 'Node: full' in the product type display, we are actually viewing this entity in it's default view mode. As a result, the condition added by the patch in #2 will always be FALSE, because $product_field['display']['node_full'] is not set.

Hence, my patch adds another check which sets $reference_view_mode to 'default' if the current view mode is not available. I haven't added this condition to the extra fields part because field_extra_fields_get_display() has it right at the top.

Phew, that was a long story :D

Status:Needs review» Fixed

Reviewed and committed. Thanks guys. : )

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Needs review
StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 3,570 pass(es).
[ View ]

@amateescu: UCR (Unsolicited Code Review) here. Happened while stumbling over a related issue. This doesn't fix any particular bug that I know of, so feel free to re-close. It's just an urge to share thoughts that hit me.

I haven't added this condition to the extra fields part because field_extra_fields_get_display() has it right at the top.

Well, you don't have to add it there, because you already set $reference_view_mode = 'default' for the remainder when the the extra fields part gets executed :) And above quote seems like that wasn't on purpose.

Future readers of this code will ask themselves:
- why is the check on $reference_view_mode done inside a loop? Now you're repeatedly checking a variable that you already changed (and at a moment that is 'theoretically random', though in practice it will always be the first iteration...)
- why does $product->display_context get the original value of $reference_view_mode, a few lines up? It's the only place where the original value is used.

Suggestion below, borrowed from field_extra_fields_get_display(). If there's a reason for the last point, then this patch is wrong.

Status:Needs review» Fixed

I actually think what we're doing for the $product->display_context is wrong; I'm thinking that should always be the actual $view_mode of the entity being viewed, not the corresponding view mode we'd expect to find on the product. I do agree with you, though, that we could be smarter about checking to see if the view mode is default or the specific view mode earlier. But we can't do it that early - your patch is based on the settings for the entity being viewed, not the settings for the referenced product type like we'd need.

I also changed the code to lazy-load the instances array like we do for the wrapper to reduce its memory impact when altering the view of entities that don't use product reference fields.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/b18f778

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

Issue summary:View changes

Fixed proposed solution.