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

getAttributeRawValue return false if value is '0' #572

Merged
merged 5 commits into from
Jan 12, 2023

Conversation

bastienlm
Copy link
Contributor

Something I think crazy
When you call
Mage::getResourceModel('catalog/product')->getAttributeRawValue($productId, 'attribute', $storeId);
if the value is 0 in database (the real value), this function return FALSE (boolean) ⁉️

Yes, for this function empty value and 0 return the same thing 'FALSE'.
I think It's more logic to return 0 if value is 0 (string or numeric). I know it's not a little update and can be affected many thing, but... 0 != false... No ?! :)

@Flyingmana
Copy link
Contributor

huh, thats a risky field to act on.

php > var_dump(false == '0');
bool(true)

which then still leads to return false, and its returning false for null and ""

In the end it still seems to lead to the expected result for any cases I can imagine. But I wonder if we should handle also the empty string case to return an empty string?
What does the database abstraction layer return in general?

This may also have a minimal impact on performance for some usecases.

@colinmollenhour
Copy link
Member

I think the problem is with the fact that one return statement deals with two cases: single attribute and multiple attributes. It is better to handle each separately.

        if (sizeof($attributesData) == 1) {
            $_data = each($attributesData);
            return $_data[1];
        }
        return $attributesData ? $attributesData : false;

@bastienlm
Copy link
Contributor Author

One simple example for understand the problems :
If you have one product with 'attribute_1' = 0;
$product->getAttribute1() => 0
Mage::getResourceModel('catalog/product')->getAttributeRawValue($productId, 'attribute_1', $storeId); => false

with getAttributeRawValue you can't know if the real value is empty or 0. I think it's not a problem for the Core of Magento, but for custom devlopment (in my case ^^, for example, you need different process if value is empty or if value is 0) this 'issue' can cause trouble.

@bastienlm
Copy link
Contributor Author

I think the problem is with the fact that one return statement deals with two cases: single attribute and multiple attributes. It is better to handle each separately.

        if (sizeof($attributesData) == 1) {
            $_data = each($attributesData);
            return $_data[1];
        }
        return $attributesData ? $attributesData : false;

More lite :) better !

@sreichel sreichel added the bug label Nov 27, 2018
@sreichel
Copy link
Contributor

I think it's not a problem for the Core of Magento, but for custom devlopment (in my case ^^, for example, you need different process if value is empty or if value is 0) this 'issue' can cause trouble.

Could reproduce with one of my modules ... I do nut use 0 value so it doesnt appeared.


Bit oftopic ...

Should we care about impact on other people code? If someone expects that 0 is treated as false it could break his code ...

I agree with a fix, but we should document it. (and maybe add unit tests for it).

@colinmollenhour colinmollenhour changed the base branch from 1.9.3.x to 1.9.4.x December 11, 2018 17:22
@tbaden tbaden added the review needed Problem should be verified label Jun 1, 2019
@colinmollenhour colinmollenhour added the backwards compatibility Might affect backwards compatibility for some users label Apr 23, 2020
@sreichel sreichel added the Component: Catalog Relates to Mage_Catalog label Jun 5, 2020
luigifab
luigifab previously approved these changes Jan 1, 2023
colinmollenhour
colinmollenhour previously approved these changes Jan 5, 2023
@sreichel
Copy link
Contributor

sreichel commented Jan 5, 2023

With 1.9.4.4 (after this PR) code changed from ...

        if (sizeof($attributesData) == 1) {
            $_data = each($attributesData);
            $attributesData = $_data[1];
        }

to

        if (sizeof($attributesData) == 1 && !$returnArray) {
            $_data = reset($attributesData);
            $attributesData = $_data;
        }

@colinmollenhour did you verify that this change is still needed?

@kiatng kiatng dismissed stale reviews from colinmollenhour and luigifab via 440d012 January 5, 2023 05:06
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Tested. It now

Retrieve attribute's raw value from DB.

as originally intended. But note that int 0 becomes "0". It return null if the value in DB is null. If the attribute doesn't exist, it returns false.

@tmotyl
Copy link
Contributor

tmotyl commented Jan 5, 2023

I consider it a bug and had to work around it in my projects several times.

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

tested, I don't know how we lived without this until now

@fballiano
Copy link
Contributor

I'm merging it and opening a new PR to document it in the README

@fballiano fballiano merged commit bbe69a4 into OpenMage:1.9.4.x Jan 12, 2023
@fballiano
Copy link
Contributor

cherry-picked to v20 with no conflicts

fballiano added a commit that referenced this pull request Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards compatibility Might affect backwards compatibility for some users bug Component: Catalog Relates to Mage_Catalog review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants