Variable data store read_price_data bypassing transients when hooked using add_filter(..., [$this, 'hookname'])

midweste commented 1 year ago

Prerequisites

[X] I have carried out troubleshooting steps and I believe I have found a bug.
[X] I have searched for similar bugs in both open and closed issues and cannot find a duplicate.

Describe the bug

Bug:

Prices are recomputed in read_price_data for every request for every variant loaded in cases where woocommerce_variation_prices_price, woocommerce_variation_prices_regular_price, or woocommerce_variation_prices_sale_price are hooked via $this
```
add_filter('woocommerce_variation_prices_sale_price', [$this, 'product_sale_price'], 10, 3)
```
If any value in $this changes from request to request, read_price_data will recompute the prices over and over and never used the saved transient. This is because get_price_hash method used for storing the transient uses md5( wp_json_encode(... of an array of class objects.
read_price_data is a pretty resource intensive operation and causes severe performance issues in cases where this happens. Because this is ran for every variant, performance penalty is pretty severe
After workaround, page performance went from 200+ seconds to load, to under 3s on product single page that loads 1 main product with 79 variants, and three "related" products with 50+ variants each

Expected behavior

read_price_data does not recompute prices every time and reads from transient

Actual behavior

read_price_data recomputes price data each time because $price_hash changes every run

First run: $price_hash array on line 425 of woocommerce/includes/data-stores/class-wc-product-variable-data-store-cpt.php - evaluates to "56a3c2b4e9a476f6abd6426393fe6f74"

Second run - Same exact hooks - should be the same price_hash, read_price_data should not recompute all prices: $price_hash array on line 425 of woocommerce/includes/data-stores/class-wc-product-variable-data-store-cpt.php - evaluates to "3fa707c32a07b825b5380d9b38095cd6"

Line 274 of never uses cache

        if ( empty( $this->prices_array[ $price_hash ] ) ) {

IMO

get_price_hash - line 420 should compute md5 on only the namespace and class of the class if a class object is supplied and not the whole class object

foreach ( $wp_filter[ $filter_name ] as $priority => $callbacks ) {
$price_hash[ $filter_name ][] = array_values( wp_list_pluck( $callbacks, 'function' ) );
}

Steps to reproduce

Add class and make sure it runs:

<?php

class WooPricehashBug
{
    public $time;

    public function __construct()
    {
        $this->time = microtime(true);
        add_filter('woocommerce_variation_prices_sale_price', [$this, 'product_sale_price'], 10, 3);
    }

    public function product_sale_price($price, $variation, $product)
    {
        return $price;
    }
}

new WooPricehashBug();

Set a breakpoint on line 274 of woocommerce/includes/data-stores/class-wc-product-variable-data-store-cpt.php Observe transient cache never being used on line 274

WordPress Environment

Not needed

Isolating the problem

[X] I have deactivated other plugins and confirmed this bug occurs when only WooCommerce plugin is active.
[X] This bug happens with a default WordPress theme active, or Storefront.
[X] I can reproduce this bug consistently using the steps above.

github-actions[bot] commented 1 year ago

Hi @midweste,

Thank you for opening the issue! It requires further feedback from the WooCommerce Core team.

We are adding the needs developer feedback label to this issue so that the Core team could take a look.

Please note it may take a few days for them to get to this issue. Thank you for your patience.

jorgeatorres commented 1 year ago

Hi @midweste,

Thanks for reaching out and for all the details you've shared in the issue. We're definitely interested in improving performance, and so we're marking this as a high priority enhancement. Thanks again!

barryhughes commented 1 year ago

May be a good candidate to pair with #37629.

woocommerce / woocommerce