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

[\Magento\ConfigurableProduct\Model\Product\Type\Configurable] PHP error unoticed #39641

Open
2 of 5 tasks
bhennesAdv opened this issue Feb 20, 2025 · 2 comments · May be fixed by #39642
Open
2 of 5 tasks

[\Magento\ConfigurableProduct\Model\Product\Type\Configurable] PHP error unoticed #39641

bhennesAdv opened this issue Feb 20, 2025 · 2 comments · May be fixed by #39642
Labels
Reported on 2.4.x Indicates original Magento version for the Issue report. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Comments

@bhennesAdv
Copy link

Summary

Hello,

My phpstorm detected an error in a Preference I had to declare copying the entire \Magento\ConfigurableProduct\Model\Product\Type\Configurable.
The following function has an issue because $product variable is used by the loop as looping element erasing the given variable.
The function is deprecated but I still think it's a quick correction.

public function getUsedProductIds($product) // <= $product is defined here
    {
        if (!$product->hasData($this->_usedProductIds)) {
            $usedProductIds = [];
            foreach ($this->getUsedProducts($product) as $product) {  // <= $product is overriden by loop
                $usedProductIds[] = $product->getId();
            }
            $product->setData($this->_usedProductIds, $usedProductIds); // $product here is the last used product of the given product
        }
        return $product->getData($this->_usedProductIds); // Returned list is OK but the "cache" effect never occurs as the data is not set on the given product
    }

Examples

Not sure if an example is relevant as the only thing that would change is the time consumed by the calls when multiple calls are made on the same object

Proposed solution

Just change the variable name is the loop ?

Release note

Correction of cache usage on configurable product models when getUsedProductIds is used multiple times on the same model

Triage and priority

  • Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.
@bhennesAdv bhennesAdv added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Feb 20, 2025
Copy link

m2-assistant bot commented Feb 20, 2025

Hi @bhennesAdv. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce.


Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
🕙 You can find the schedule on the Magento Community Calendar page.
📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

bhennesAdv added a commit to bhennesAdv/magento2 that referenced this issue Feb 20, 2025
…t on subsequent calls on the same $product object
@bhennesAdv
Copy link
Author

Hello,

I made a PR for the change but I don't know on which branch do you want me to create the PR to ?

For testing, the same, I did not invest time in creating automatic/manual testing for a variable name change in a deprecated function. Let me know if you need more.

Regards,
Baptiste

@engcom-Bravo engcom-Bravo added the Reported on 2.4.x Indicates original Magento version for the Issue report. label Feb 20, 2025
@github-project-automation github-project-automation bot moved this to Ready for Confirmation in Issue Confirmation and Triage Board Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reported on 2.4.x Indicates original Magento version for the Issue report. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Status: Ready for Confirmation
2 participants