Skip to content

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

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
Open
2 of 5 tasks
bhennesAdv opened this issue Feb 20, 2025 · 7 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 · 7 comments · May be fixed by #39642
Assignees
@Sathakathulla-S
Labels
Area: Catalog Component: ConfigurableProduct Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch 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
@m2-assistant M2 Assistant
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
@bhennesAdv
magento#39641 Replace loop variable name to correct the "cache" effec…
80c0d08 
…t on subsequent calls on the same $product object
@bhennesAdv bhennesAdv linked a pull request Feb 20, 2025 that will close this issue
Open
5 tasks
@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
@engcom-Hotel engcom-Hotel self-assigned this Mar 11, 2025
@m2-assistant M2 Assistant
Copy link

m2-assistant bot commented Mar 11, 2025

Hi @engcom-Hotel. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
  • 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
  • 4. Verify that the issue is reproducible on 2.4-develop branch
    Details- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
  • 5. Add label Issue: Confirmed once verification is complete.
  • 6. Make sure that automatic system confirms that report has been added to the backlog.

@engcom-Hotel
Copy link
Contributor

Hello @bhennesAdv,

Thanks for the report and creating a fix for the same!

After going through the codebase mentioned in the main description, the issue seems reproducible for us:

magento2/app/code/Magento/ConfigurableProduct/Model/Product/Type/Configurable.php

Lines 551 to 561 in 11be3df

public function getUsedProductIds($product)
{
if (!$product->hasData($this->_usedProductIds)) {
$usedProductIds = [];
foreach ($this->getUsedProducts($product) as $product) {
$usedProductIds[] = $product->getId();
}
$product->setData($this->_usedProductIds, $usedProductIds);
}
return $product->getData($this->_usedProductIds);
}

There is no need to reproduce this issue as it is confirming via looking into the codebase only. Hence confirming the issue.

Thanks

@engcom-Hotel engcom-Hotel added Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Component: ConfigurableProduct Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Area: Catalog labels Mar 11, 2025
@github-jira-sync-bot
Copy link

✅ Jira issue https://jira.corp.adobe.com/browse/AC-14159 is successfully created for this GitHub issue.

@m2-assistant M2 Assistant
Copy link

m2-assistant bot commented Mar 11, 2025

✅ Confirmed by @engcom-Hotel. Thank you for verifying the issue.
Issue Available: @engcom-Hotel, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

@engcom-Hotel engcom-Hotel moved this to Ready for Development in High Priority Backlog Mar 11, 2025
@Sathakathulla-S
Copy link

@magento I'm working on this

@Sathakathulla-S Sathakathulla-S mentioned this issue Apr 12, 2025
Closed
5 tasks
engcom-Dash added a commit to bhennesAdv/magento2 that referenced this issue Apr 17, 2025
@engcom-Dash
Merge branch '2.4-develop' into magento#39641
79c4cd3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Sathakathulla-S Sathakathulla-S

Labels
Area: Catalog Component: ConfigurableProduct Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
High Priority Backlog
Status: Ready for Development
Milestone
No milestone
5 participants
@engcom-Bravo @engcom-Hotel @github-jira-sync-bot @bhennesAdv @Sathakathulla-S