Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Obsolete code in block for configurable products #2246

Closed
theroch opened this issue Jun 22, 2022 · 11 comments · Fixed by #4008
Closed

Obsolete code in block for configurable products #2246

theroch opened this issue Jun 22, 2022 · 11 comments · Fixed by #4008
Labels
needs investigation

Comments

@theroch
Copy link
Contributor

theroch commented Jun 22, 2022 •
edited by kiatng
Loading

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?
@theroch theroch added the bug label Jun 22, 2022
@elidrissidev
Copy link
Member

elidrissidev commented Jun 22, 2022 •
edited
Loading

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
Copy link
Contributor

tmotyl commented Jun 22, 2022

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
Copy link
Member

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

magento-lts/app/code/core/Mage/Catalog/Helper/Product/Type/Composite.php

Lines 45 to 52 in 412bee0

public function preparePrice($product, $price, $isPercent = false, $storeId = null)
{
if ($isPercent && !empty($price)) {
$price = $product->getFinalPrice() * $price / 100;
}
return $this->registerJsPrice($this->convertPrice($price, true, $storeId));
}

@sreichel
Copy link
Contributor

$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
Copy link
Contributor Author

theroch commented Jun 23, 2022

What about custom code that sets $isPercent to true?

This makes no sense.
The code

magento-lts/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php

Lines 231 to 235 in 412bee0

foreach ($optionPrices as $optionPrice) {
foreach ($optionPrices as $additional) {
$this->_preparePrice(abs($additional-$optionPrice));
}
}

depends on

magento-lts/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php

Lines 196 to 198 in 412bee0

$currentProduct->setConfigurablePrice(
$this->_preparePrice($value['pricing_value'], $value['is_percent'])
);

So is percent is considered in lines 196-198 and the result will never be in percent.

@theroch
Copy link
Contributor Author

theroch commented Jun 23, 2022

Overwrites the current call of the loop

magento-lts/app/code/core/Mage/Catalog/Block/Product/View/Type/Configurable.php

Lines 231 to 235 in 412bee0

foreach ($optionPrices as $optionPrice) {
foreach ($optionPrices as $additional) {
$this->_preparePrice(abs($additional-$optionPrice));
}
}

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
Copy link
Member

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
Copy link
Contributor Author

theroch commented Jun 23, 2022 •
edited
Loading

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
Copy link
Member

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

@theroch
Copy link
Contributor Author

theroch commented Oct 11, 2022

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

We still use this code without any issues.

@elidrissidev
Copy link
Member

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

@Hanmac Hanmac mentioned this issue May 24, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs investigation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Configurable: remove unused double foreach loop b3-it/magento-lts
5 participants
@tmotyl @theroch @sreichel @addison74 @elidrissidev