Unused product attributes display with value N/A or NO on storefront

[thomasnordkvist]

Preconditions

1. Magenti 2.1.7

Steps to reproduce

1. Create product attribute of type text field and set visible on catalog storefront.
2. Add attribute to attribute set default.
3. Create product with attribute set default and do not

Expected result

1. Attribute should not be seen on product page in More Informatin tab

Actual result

1. Attribute shows on product page with value N/A

This "Fix" introduced this behavior

[southerncomputer]

app/code/Magento/Catalog/Block/Product/View/Attributes.php
`-

•            if (is_string($value) && strlen($value)) {
  

•            if ($value instanceof Phrase || (is_string($value) && strlen($value))) {`
  

remove ($value instance Phrase || and return to prior value to fix this!

[AlexWorking]

Hello, @thomasnordkvist. Based on Your issue an internal ticket was created - MAGETWO-69919. You'll be informed when the issue is fixed. Thank's for applying.

[lingwooc]

If you don't want to create a module and inject a block you can fix it in the template too. It needs some rather ugly looking code.

Override module-catalog/view/frontend/templates/product/view/attributes.phtml and adjust the appropriate bit to look like this.

            <?php foreach ($_additional as $_data):
                $value = $_helper->productAttribute($_product, $_data['value'], $_data['code']);
                if(!(($value instanceof Magento\Framework\Phrase) && ($value->__toString() === 'N/A' || $value->__toString() === 'No'))): ?>
                <tr>
                    <th class="col label" scope="row"><?php echo $block->escapeHtml(__($_data['label'])) ?></th>
                    <td class="col data" data-th="<?php echo $block->escapeHtml(__($_data['label'])) ?>"><?php /* @escapeNotVerified */ echo $value; ?></td>
                </tr>
            <?php endif;
            endforeach; ?>

Delete the file, if the patch ever makes it into a release.

[PieterCappelle]

Any update on this one?

[AlexWorking]

Hi. Not yet.

[hostep]

It looks like this problem is being addressed in: #10619

The patch to apply to Magento 2.1.7/2.1.8 is slightly different then the one in #10619, because Magento 2.1.7 had already a more correct fix (introduced in d977dc3#diff-8bb5f6b700bf55d266640da3251af32cL89) then the one in the develop branch (introduced in #8623):

diff --git a/app/code/Magento/Catalog/Block/Product/View/Attributes.php b/app/code/Magento/Catalog/Block/Product/View/Attributes.php
index 6960ff0b8e5..1da3f1d66e0 100644
--- a/app/code/Magento/Catalog/Block/Product/View/Attributes.php
+++ b/app/code/Magento/Catalog/Block/Product/View/Attributes.php
@@ -81,13 +81,15 @@ class Attributes extends \Magento\Framework\View\Element\Template

                 if (!$product->hasData($attribute->getAttributeCode())) {
                     $value = __('N/A');
+                } elseif ($value instanceof Phrase) {
+                    $value = (string)$value;
                 } elseif ((string)$value == '') {
                     $value = __('No');
                 } elseif ($attribute->getFrontendInput() == 'price' && is_string($value)) {
                     $value = $this->priceCurrency->convertAndFormat($value);
                 }

-                if (($value instanceof Phrase || is_string($value)) && strlen($value)) {
+                if (is_string($value) && strlen($value)) {
                     $data[$attribute->getAttributeCode()] = [
                         'label' => __($attribute->getStoreLabel()),
                         'value' => $value,

Testing by applying the patch above to Magento 2.1.7 seems to fix the problem.

It still feels kind of strange that Phrases aren't being outputted, so maybe the two Phrases (N/A & No) created above should just be replaced by $value = ''; ?

[orlangur]

and set visible on catalog storefront

Expected result
Attribute should not be seen on product page in More Informatin tab

Expected result does not seem to be valid to me. Why "visible on catalog storefront" needs to be set to Yes in this scenario?

The fact such attribute was not displayed before due to a bug does not justify the expected result.

[hostep]

Because sometimes a product doesn't have a value set for a certain attribute on product level, and we (in our case, not sure if everybody wants this) want to see the attribute when it is filled in, and not see it when it is empty.

Thinking about it, it's probably more correct like you are saying and still display N/A even though the attribute is not filled in on a product.
We should probably write a small module to change the behavior for our particular client, to have it behave like they want.

I think the reason why it surprised so many people is that the behavior changed between 2.1.6 and 2.1.7 and this change of behavior was considered a new bug, but that's probably not true thinking it through.

I think you are correct @orlangur, thanks for persisting :)

[orlangur]

Ok, it becomes more and more clear.

Not sure where the requirement to display 'N/A' came from, removing it would be more like a requirements change than a regular bugfix. As a workaround we can have logic with non-empty strlen check and 'N/A' translated to '' but not sure it's not too dirty.

Let's get back to current PR to understand the necessary changes.

[dnadle]

I think it's better to hide the attributes without values, like 2.1.6 and earlier versions did. Please make that an option.

[magento-engcom-team]

@thomasnordkvist, thank you for your report.
We've created internal ticket(s) MAGETWO-69919 to track progress on the issue.

[Tristan-N]

@hostep I implemented your fix and it seems to work well on the productpage in my local dev environment, but when we test it (unit testing using Travis), Travis fails to accept this solution. Any idea why this error shows up and how to fix this error?

There were 3 failures:
1) Magento\Catalog\Test\Unit\Block\Product\View\AttributesTest::testGetAdditionalData with data set "Product Has No Attribute Value" (array(Magento\Eav\Model\Entity\Attribute), false, array(), array(array('Test Attribute', 'N/A', 'attribute')))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'attribute' => Array (...)
 )
/home/travis/build/Vendor/theme/vendor/magento/module-catalog/Test/Unit/Block/Product/View/AttributesTest.php:122
2) Magento\Catalog\Test\Unit\Block\Product\View\AttributesTest::testGetAdditionalData with data set "Product With Null Attribute Value" (array(Magento\Eav\Model\Entity\Attribute), true, array(), array(array('Test Attribute', 'No', 'attribute')))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'attribute' => Array (...)
 )
/home/travis/build/Vendor/theme/vendor/magento/module-catalog/Test/Unit/Block/Product/View/AttributesTest.php:122
3) Magento\Catalog\Test\Unit\Block\Product\View\AttributesTest::testGetAdditionalData with data set "Product With Phrase Attribute Value" (array(Magento\Eav\Model\Entity\Attribute), true, array(), array(array('Test Attribute', 'test', 'attribute')))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'attribute' => Array (...)
 )
/home/travis/build/Vendor/theme/vendor/magento/module-catalog/Test/Unit/Block/Product/View/AttributesTest.php:122
                                                                           
FAILURES!                                                                  
Tests: 12340, Assertions: 37040, Failures: 3, Incomplete: 41, Skipped: 148.

[orlangur]

@Tristan-N please continue work on top of https://github.com/magento/magento2/pull/10619/files branch. The only problem left there was integrity test for translations didn't like __('') and __('Yes'), I didn't have a chance to look into it deeper yet.

Another case which would be nice to be covered is to not perform string casting when $value is an array.

[okorshenko]

The issue has been fixed and delivered to 2.2-develop branch. Will be available with upcoming patch release

[gtlt]

I had a template fix against 'N/A' in 2.2.x but now in 2.2.4 it is 'No' 'Non' etc in all languages and __('No') doesn't translate ... so I have to check against all translation of 'No'. Any clue ?