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.

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

Fix bug breaking price cache keys used in getPriceStatic and priceCalculation #164

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

matthieu-rolland
Copy link
Contributor

Questions Answers
Description? The method getProductProperties was sent a product without an id_product_attribute specified, which caused a price cache key to not be build properly
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company PrestaShop SA
How to test?

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I don't see how is this a solution?

A) You can check few lines below that I send the parameter if it's available.

// Add information about attribute
        if (!empty($this->params['id_product_attribute'])) {
            $product['id_product_attribute'] = (int) $this->params['id_product_attribute'];
        }

B) You can check in Product::getProductProperties, that sending no id_product_attribute is a perfectly valid syntax and it will automatically retrieve default attribute inside using Product::getDefaultAttribute.

@matthieu-rolland
Copy link
Contributor Author

Sorry but I don't see how is this a solution?

A) You can check few lines below that I send the parameter if it's available.

// Add information about attribute
        if (!empty($this->params['id_product_attribute'])) {
            $product['id_product_attribute'] = (int) $this->params['id_product_attribute'];
        }

B) You can check in Product::getProductProperties, that sending no id_product_attribute is a perfectly valid syntax and it will automatically retrieve default attribute inside using Product::getDefaultAttribute.

It had to be done before the call of getProductProperties, getPriceStatic and priceCalculation methods need this information.

@Hlavtox
Copy link
Contributor

Hlavtox commented Mar 4, 2024

After internal discussion - the fix is legit because it provides a proper combination ID into the logic and provides a correct price for a product.

But the issue with failing tests lies inside Product::getProperties method, which returns null for a default combination in test context, for some reason.

@Hlavtox Hlavtox merged commit d70caf6 into PrestaShop:dev Mar 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants