Obsolete code in block for configurable products

[theroch]

We have inherited a development that implements a booking function via a calendar with time selection. This is realized via configurable products. Depending on the available time slots, there can be as many as 800 options. We know that this is not the best solution to this problem. But that should not be the topic here.

Preconditions (*)

1. 8xCPUs Intel Xeon 1,8 GHz, 16GB RAM, SATA-SSD
2. OpenMage 19.4.16 and earlier and also 20.0.14 and earlier

Steps to reproduce (*)

1. Create attribute for configurable product with at least 200 options
2. Create configurable product for this attribute
3. Add subproducts to this attribute option
4. View the product in frontend

Expected result (*)

1. The request to view this product should be completed in under 1s

Actual result (*)

1. The request to view this product takes a long time. Per 100 options, the request will take 1s more to complete
2. With 800 options the request will take about 8s to complete.

Cause of the problem

The Problem is located in app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php starting at line 227:

/**
 * Prepare formated values for options choose
 */
foreach ($optionPrices as $optionPrice) {
    foreach ($optionPrices as $additional) {
        $this->_preparePrice(abs($additional-$optionPrice));
    }
}

Because _preparePrice and all sub calls to all other methods operate with return values and not with objects or session data, the result of this method is never considered. We think the loops and the call to to _preparePrice starting at line 227 is not needed for correct functionality. This function is a performance brake and only burns time. Can this part of code safely be removed?

Can someone please verify our assessment?

[elidrissidev]

From the first glance it does seem useless as it doesn't cause any side-effects, just returning stuff.

Have you tried removing that code and reloading the page? What was the loading time?

[tmotyl]

If I see correctly Its not an no-brainer. The code has several side effects. $this->_preparePrice calls internally Mage_Catalog_Helper_Product_Type_Composite::preparePrice which calls $product->getFinalPrice() and Mage_Catalog_Helper_Product_Type_Composite::registerJsPrice

and getFinalPrice sets 'final_price' field on product which is calculated in Mage_Catalog_Model_Product_Type_Price::getFinalPrice (which also triggers a catalog_product_get_final_price event)

So more digging/profiling is needed to optimize the code without breaking.

[elidrissidev]

$isPercent is set to false by default so the code inside the block is never executed. https://github.com/OpenMage/magento-lts/blob/412bee01b9cc77f50b4333b098e25d116bf9e820/app/code/core/Mage/Catalog/Helper/Product/Type/Composite.php#L45-L52

[sreichel]

$isPercent is set to false by default so the code inside the block is never executed.

What about custom code that sets $isPercent to true?

[theroch]

What about custom code that sets $isPercent to true?

This makes no sense. The code https://github.com/OpenMage/magento-lts/blob/412bee01b9cc77f50b4333b098e25d116bf9e820/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php#L231-L235 depends on https://github.com/OpenMage/magento-lts/blob/412bee01b9cc77f50b4333b098e25d116bf9e820/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php#L196-L198 So is percent is considered in lines 196-198 and the result will never be in percent.

[theroch]

Overwrites the current call of the loop https://github.com/OpenMage/magento-lts/blob/412bee01b9cc77f50b4333b098e25d116bf9e820/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php#L231-L235 not all preceding ones? Thus it would be sufficient to call _preparePrice only once? However, this would also be pointless, since then only

$this->_preparePrice(0);

would remain?

[elidrissidev]

What about custom code that sets $isPercent to true?

I was only talking about the execution path mentioned in OP, I don't know about the usage in other places.

[theroch]

Have you tried removing that code and reloading the page? What was the loading time?

We commented out that code block and the request completed in under a second. We have not noticed any side effects but we only tested our special test case and no other ones.

[elidrissidev]

@theroch Hey, are you still using your workaround in production? Any issues noticed?

[theroch]

@theroch Hey, are you still using your workaround in production? Any issues noticed?

We still use this code without any issues.

[elidrissidev]

@theroch Thank you! Feel free to open a PR to be reviewed if you wish, otherwise I'll do it later.