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 check for boolean product attributes #8623

Merged

Conversation

TKlement
Copy link

@TKlement TKlement commented Feb 20, 2017

Based on that Issue #6634 here is a fix to add also boolean product attributes to display in frontend.

Magento2 handles translations with the Phrase instance. A product attribut with type boolean uses Phrase instance but the validation verify for strings.
This fix also includes the instance Phrase for the verification.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 20, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Timo Klement seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.


if (is_string($value) && strlen($value)) {
if ($value instanceof Phrase || (is_string($value) && strlen($value))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, from second look I have a little suggestion :) As Phrase accepts empty text, we need to check its length too.

So, it would be nice to add something like

$value = $value instanceof Phrase ? (string)$value : $value;

prior to if instead of additional condition.


Would be nice to add a unit test for this case, but it's up to you, I believe.


And please link commit to your GitHub account: https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/ ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a big difference between current solution and the one you proposed.
It's not mandatory to convert Phrase into string to pass data into template

Copy link
Contributor

Choose a reason for hiding this comment

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

@maghamed, right part of (is_string($value) && strlen($value)) is not checked in case of Phrase currently.

I didn't think deeper if this is a problem or not (like, maybe there is no need to check length at all).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ishakhsuvarov @maghamed so check for strlen could be simply removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This failure to check for empty string results in empty attributes displaying as N/A in product view versus suppressing lines that have no value. Please fix to 2.1-develop !

@maghamed maghamed self-requested a review February 21, 2017 17:39
@maghamed maghamed self-assigned this Feb 21, 2017
@okorshenko okorshenko added this to the March 2017 milestone Mar 1, 2017

if (is_string($value) && strlen($value)) {
if ($value instanceof Phrase || (is_string($value) && strlen($value))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a big difference between current solution and the one you proposed.
It's not mandatory to convert Phrase into string to pass data into template

@magento-team magento-team merged commit 4767e01 into magento:develop Mar 8, 2017
@ishakhsuvarov
Copy link
Contributor

@TKlement Fix has now been delivered to the develop branch. Thanks for contributing!

@onepack
Copy link

onepack commented Mar 9, 2017

I applied the fix and it shows the yes/ no values now with succes on the frontend.
However, the color attribute that is of type single select also shows value "No" now when no color is selected for a product.

@ventouris
Copy link

I have the community version 2.1.6 and still have this issue.

@mudithae
Copy link

In Related to community version 2.1.6 I think real reason for this kind of behaviour is lies in the logic of fetching attributes.
vendor/magento/module-eav/Model/ResourceModel/ReadHandler.php

foreach ($connection->fetchAll($unionSelect) as $attributeValue) { if (isset($attributesMap[$attributeValue['attribute_id']])) { $entityData[$attributesMap[$attributeValue['attribute_id']]] = $attributeValue['value']; } else { $this->getLogger()->warning( "Attempt to load value of nonexistent EAV attribute '{$attributeValue['attribute_id']}' for entity type '$entityType'." ); } }

I think $attributesMap array should be the one that need to be loop. If the result from all $unionSelect doesn't have any value for the $attributesMap element, then it should fetch the default value for the attribute.

@southerncomputer
Copy link
Contributor

This causes Empty EAV attributes to display as: N/A rather than line being suppressed on 2.1.7 - it also breaks my dynamically loaded attributes (via xml call) when used with && to include length check!

Had to back-out this change on 2.1.7 to function as normal!

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.