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 "each()" function call on potentially invalid data #8446

Merged
merged 1 commit into from
Feb 9, 2017
Merged

Fix "each()" function call on potentially invalid data #8446

merged 1 commit into from
Feb 9, 2017

Conversation

giacmir
Copy link
Member

@giacmir giacmir commented Feb 7, 2017

In some cases the $attributesData variable can be false at this point of execution. If this happens count($attributesData) returns 1 and then the code inside the if is evaluated. But then the call to each fails with error Variable passed to each() is not an array or object in /var/www/html/vendor/magento/module-catalog/Model/ResourceModel/AbstractResource.php

It is necessary to check that $attributesData is actually an array before attempt to count its content.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 7, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov
Copy link
Contributor

@giacmir Hi, could you please provide an example where $attributesData is going to be false?
Also, did you consider checking this way: $attributesData !== false?
Thanks!

@giacmir
Copy link
Member Author

giacmir commented Feb 7, 2017

@ishakhsuvarov It happens on an edge case if you call getAttributeRawValue with a nonexistent product id and a non-typed attribute (such as 'sku'). This is due to the fact that fetchRow ultimately calls PDOStatement::fetch that can return false when no results are found. Then, since the 'sku' is a non-typed attribute the following if statement is not executed and the $attributesData is false when it is checked with 'sizeof'.

$attributesData !== false could also work, and is probably faster, but since the following code uses each that is an array specific function checking with is_array seems safer to me.

@ishakhsuvarov
Copy link
Contributor

@giacmir
Thank you. Sounds really reasonable to me.
Could you please sign CLA so I can continue with the merge?
Thank you.

@giacmir
Copy link
Member Author

giacmir commented Feb 7, 2017

@ishakhsuvarov I tried, and it says "Thank you for signing the CLA" and sends me back to github but nothing happens..

@ishakhsuvarov
Copy link
Contributor

@giacmir Could you please check, that name and e-mail on the commit is the same as in Github profile? Currently Github shows a "question mark" near the name on your commit.

@ishakhsuvarov
Copy link
Contributor

@giacmir Thank you

@mmansoor-magento mmansoor-magento merged commit f0f61b7 into magento:develop Feb 9, 2017
@okorshenko
Copy link
Contributor

@giacmir that you for this PR. Your PR merged to develop branch

@okorshenko okorshenko added this to the February 2017 milestone Feb 11, 2017
@vrann vrann added the simple label Mar 11, 2017
@giacmir giacmir deleted the fix-abstractresource branch April 3, 2017 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants