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

TierPriceManagement: add() fails randomly because of invalid argument supplied for foreach() #30908

Open
1 of 5 tasks
HenriNijborg opened this issue Nov 12, 2020 · 9 comments · May be fixed by #30941
Open
1 of 5 tasks
Labels
Issue: needs update Additional information is require, waiting for response Priority: P3 May be fixed according to the position in the backlog. Progress: PR in progress Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Comments

@HenriNijborg
Copy link

HenriNijborg commented Nov 12, 2020

File: app/code/Magento/Catalog/Model/Product/TierPriceManagement.php
Function: add()

Error Invalid argument supplied for foreach() on line foreach ($tierPrices as &$item)

        ...
        $tierPrices = $product->getData('tier_price');
        ...

        foreach ($tierPrices as &$item) {
            if ('all' == $customerGroupId) {
                $isGroupValid = ($item['all_groups'] == 1);
            } else {
                $isGroupValid = ($item['cust_group'] == $customerGroupId);
            }

            if ($isGroupValid && $item['website_id'] == $websiteIdentifier && $item['price_qty'] == $qty) {
                $item['price'] = $price;
                $found = true;
                break;
            }
        }
       ...

The behaviour is random. Strongly related to issue #24410 and bug fix 114ac4b

Preconditions (*)

  1. Magento 2.3.5-p1

Steps to reproduce (*)

  1. Add a tier price programmaticaly using the TierPriceManagement class.
  2. foreach ($tierPrices as &$item) randomly throws Invalid argument supplied for foreach()

Expected result (*)

  1. $tierPrices should be checked for being null just as within bug fix 114ac4b
  2. Tier Price is added.

Actual result (*)

  1. $tierPrices isn't checked for being null causing the error.
  2. Tier Prices aren't added

Suspicions

I think the result of a cached product has tier_price: null and the non cached product is tier_price: [] as the behaviour is randomly and caching might be not for all products and not always valid anymore.

Suggested fix

public function add($sku, $customerGroupId, $price, $qty)
    {
        if (!is_float($price) && !is_int($price) && !\Zend_Validate::is((string)$price, 'Float')
            || !is_float($qty) && !is_int($qty) && !\Zend_Validate::is((string)$qty, 'Float')
            || $price <= 0
            || $qty <= 0
        ) {
            throw new InputException(__('The data was invalid. Verify the data and try again.'));
        }
        $product = $this->productRepository->get($sku, ['edit_mode' => true]);
        $websiteIdentifier = 0;
        $value = $this->config->getValue('catalog/price/scope', \Magento\Store\Model\ScopeInterface::SCOPE_WEBSITE);
        if ($value != 0) {
            $websiteIdentifier = $this->storeManager->getWebsite()->getId();
        }
        $found = false;

        $tierPrices = $product->getData('tier_price');
        if($tierPrices !== null) {
            foreach ($tierPrices as &$item) {
                if ('all' == $customerGroupId) {
                    $isGroupValid = ($item['all_groups'] == 1);
                } else {
                    $isGroupValid = ($item['cust_group'] == $customerGroupId);
                }

                if ($isGroupValid && $item['website_id'] == $websiteIdentifier && $item['price_qty'] == $qty) {
                    $item['price'] = $price;
                    $found = true;
                    break;
                }
            }
        }
        
        if (!$found) {
            $mappedCustomerGroupId = 'all' == $customerGroupId
                ? $this->groupManagement->getAllCustomersGroup()->getId()
                : $this->groupRepository->getById($customerGroupId)->getId();

            $tierPrices[] = [
                'cust_group' => $mappedCustomerGroupId,
                'price' => $price,
                'website_price' => $price,
                'website_id' => $websiteIdentifier,
                'price_qty' => $qty,
            ];
        }

        $product->setData('tier_price', $tierPrices);
        $errors = $product->validate();
        if (is_array($errors) && count($errors)) {
            $errorAttributeCodes = implode(', ', array_keys($errors));
            throw new InputException(
                __('Values in the %1 attributes are invalid. Verify the values and try again.', $errorAttributeCodes)
            );
        }
        try {
            $this->productRepository->save($product);
        } catch (\Exception $e) {
            if ($e instanceof TemporaryStateExceptionInterface) {
                // temporary state exception must be already localized
                throw $e;
            }
            throw new CouldNotSaveException(__("The group price couldn't be saved."));
        }
        return true;
    }

Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

  • 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”.
@m2-assistant
Copy link

m2-assistant bot commented Nov 12, 2020

Hi @HenriNijborg. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

Please, add a comment to assign the issue: @magento I am working on this


⚠️ 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, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@m2-community-project m2-community-project bot added this to Ready for Confirmation in Issue Confirmation and Triage Board Nov 12, 2020
@engcom-Alfa engcom-Alfa added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Nov 13, 2020
@chiranjeevi-cj
Copy link
Contributor

@magento I am working on this

@chiranjeevi-cj
Copy link
Contributor

@engcom-Alfa Should I proceed with the suggested solution by @HenriNijborg ?

@HenriNijborg
Copy link
Author

I think there is a second line that needs fixing:
$product = $this->productRepository->get($sku, ['edit_mode' => true]);

should be retrieving the products without using the cache as it will be used to compare the new rules to the existing ones. If the cache isn't valid then my suggested fix will cause no error and it will proceed and overide possibly tier prices which aren't in the cache but are in the db.

$product = $this->productRepository->get($sku, ['edit_mode' => true, 'force_reload' => true]);
or
$product = $this->productRepository->get($sku, ['edit_mode' => true], null, true);

I'm not a Magento 2 developer so I'm not sure which one it should be.

@m2-community-project m2-community-project bot added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Nov 26, 2020
@stale
Copy link

stale bot commented Feb 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

@stale stale bot added the stale issue label Feb 10, 2021
@m2-community-project m2-community-project bot added this to Pull Request In Progress in Low Priority Backlog Feb 10, 2021
@m2-community-project m2-community-project bot removed this from Ready for Confirmation in Issue Confirmation and Triage Board Feb 10, 2021
@HenriNijborg
Copy link
Author

Yes, this issue is still relevant. Pull Request author @chiranjeevi-cj asked for help from @magento-engcom-team in related pull request #30941

@stale stale bot removed the stale issue label Feb 11, 2021
@vehgroshop
Copy link

What is the status of this issue? We would also like to have it fixed.

@PatrickJager
Copy link

@magento-engcom-team @chiranjeevi-cj
What is the status on this issue? Did you manage to fix it?
We currently have another customer having this exact same issue.
Some tierprices work some do not.

@engcom-Bravo
Copy link
Contributor

engcom-Bravo commented Jan 27, 2023

Hi @HenriNijborg,

Thank you for reporting and collaboration.

Verified the issue on Magento 2.4-develop instance and the issue is not reproducible.Kindly refer the screenshots.

1.Added a tier price programmaticaly using the TierPriceManagement class.
2.foreach ($tierPrices as &$item) randomly throws Invalid argument supplied for foreach()

Screenshot 2023-01-27 at 5 54 13 PM

The issue is no more reproducible.it is already fixed in 2.4-develop.

Thanks.

@engcom-Bravo engcom-Bravo added the Issue: needs update Additional information is require, waiting for response label Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: needs update Additional information is require, waiting for response Priority: P3 May be fixed according to the position in the backlog. Progress: PR in progress Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Low Priority Backlog
  
Pull Request In Progress
7 participants