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 json encoded attribute backend type when attribute value is null #11947

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

tkotosz
Copy link
Contributor

@tkotosz tkotosz commented Nov 2, 2017

When you create a new product attribute with JsonEncoded backend type then you always get the following error when you try to load a product which doesn't have value for that specific attribute yet:

Exception #0 (Magento\Eav\Model\Entity\Attribute\Exception): Unable to unserialize value.

The source of the problem is this method:

public function unserialize($string)
    {
        $result = json_decode($string, true);
        if (json_last_error() !== JSON_ERROR_NONE) {
            throw new \InvalidArgumentException('Unable to unserialize value.');
        }
        return $result;
    }

When you do json_decode(null, true), then json_last_error() will return 4 (JSON_ERROR_SYNTAX).

With the suggested fix it will set the attribute value to empty array when the attribute value is null.

@ihor-sviziev
Copy link
Contributor

@tkotosz could you cover your change with unit test in order to prevent regression in this part in future?

@dmanners
Copy link
Contributor

dmanners commented Nov 2, 2017

Thanks for the PR @tkotosz I am happy with the code but I would love to see a test case to cover this. Currently the method testAfterLoad only covers the case when the attribute has been set.

@tkotosz
Copy link
Contributor Author

tkotosz commented Nov 3, 2017

@dmanners updated

@@ -94,5 +94,13 @@ public function testAfterLoad()
);
$this->model->afterLoad($product);
$this->assertEquals([1, 2, 3], $product->getData('json_encoded'));

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this new code as separate method (test case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ihor-sviziev sure, done. Also I have squashed my commits.

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

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

Add the visibility on the new test method please.

/**
* Test after load handler with null attribute value
*/
function testAfterLoadWithNullAttributeValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

guess you also need to add the visibility on this method.

@tkotosz
Copy link
Contributor Author

tkotosz commented Nov 5, 2017

@dmanners fix pushed :)

@okorshenko okorshenko merged commit fadbf97 into magento:2.2-develop Nov 9, 2017
okorshenko pushed a commit that referenced this pull request Nov 9, 2017

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
… value is null #11947
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.

None yet

6 participants