Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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:
foreach (field_info_instances('commerce_product', $product->type) as $product_field_name => $product_field) {
...
}
Proposed version:
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 1352774-6.patch | 1.98 KB | roderik |
| #3 | 1352774-3.patch | 3.14 KB | amateescu |
| #2 | commerce-exclude-injected-product-fields-from-being-prepared-when-hidden.patch | 3.42 KB | das-peter |
Comments
Comment #1
pcambraAs Peter and I discussed in IRC, next step is rolling a patch against -dev and see what testbot answers.
Comment #2
das-peter CreditAttribution: das-peter commentedMuchas gracias to Pedro for taking the time to discuss this :)
Comment #3
amateescu CreditAttribution: amateescu commentedThe 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 andfield_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_modeto 'default' if the current view mode is not available. I haven't added this condition to the extra fields part becausefield_extra_fields_get_display()has it right at the top.Phew, that was a long story :D
Comment #4
rszrama CreditAttribution: rszrama commentedReviewed and committed. Thanks guys. : )
Comment #6
roderik@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.
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_modedone 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_contextget 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.Comment #7
rszrama CreditAttribution: rszrama commentedI 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
Comment #8.0
(not verified) CreditAttribution: commentedFixed proposed solution.