[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

p-bystritsky · 2017-11-06T12:28:19Z

This is a backport for #10619

Description

Attributes with no value set should not display on storefront.

Fixed Issues (if relevant)

Unused product attributes display with value N/A or NO on storefront #9961: Unused product attributes display with value N/A or NO on storefront.
Yes/No attribute value is not shown on a product details page #6634: Yes/No attribute value is not shown on a product details page

Manual testing scenarios

Create a Custom Attribute, set it's "Catalog Input Type for Store Owner" to "Text Area" , "Visible on Catalog Pages on Storefront" to "Yes".
Assign Custom Attribute to Default Attribute Set.
Create a Custom Product with Default Attribute Set, make it visible on Storefront, do not set any value to Custom Attribute.
Open Custom Product on Storefront - the Custom Attribute label and it's value should not be visible.
Edit the Custom Product, set any value to it's Custom Attribute.
Open Custom Product on Storefront - the Custom Attribute label and it's value should be visible.

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…on storefront.

orlangur · 2017-11-06T14:26:18Z

Hi @p-bystritsky, thanks for your contribution, we can start from processing this PR targeting 2.2-develop. The only problem in old PR was

2) Magento\Test\Integrity\Phrase\ArgumentsTest::testArguments
2 missed phrases were discovered: 
Missed Phrase: File: /home/travis/build/magento/magento2/app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributeTest.php 
Line: 68
Missed Phrase: File: /home/travis/build/magento/magento2/app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributeTest.php 
Line: 133
Failed asserting that an array is empty.

which was not running on Travis at that time.

orlangur · 2017-11-06T14:27:51Z

app/code/Magento/Catalog/Test/Unit/Block/Product/View/AttributesTest.php

+
+    protected function setUp()
+    {
+        // @codingStandardsIgnoreStart


Can we get rid of such annotations?

Removed them.

…on storefront.

p-bystritsky · 2017-11-08T11:39:16Z

@orlangur can you please provide more information about requested changes?

orlangur · 2017-11-08T14:55:51Z

@p-bystritsky didn't check after last changes yet, Travis builds are green so I suppose it will be good to go.

vkublytskyi

The integrity tests are fixed now.

But I would like to ask you clean up the code and remove obsolete logic to improve code readability and make behavior more explicit. See my inline comments.

vkublytskyi · 2017-11-23T13:26:44Z

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

@@ -85,13 +85,15 @@ public function getAdditionalData(array $excludeAttr = [])

                if (!$product->hasData($attribute->getAttributeCode())) {
                    $value = __('N/A');
+                } elseif ($value instanceof Phrase) {
+                    $value = (string)$value;
                } elseif ((string)$value == '') {
                    $value = __('No');


This is weird logic. Why empty string should be transformed to No. Looks like this some obsolete workaround that should be removed. If this cause any issue then it definitely should be fixed in another way.
Please remove this elseif logic as it has no sense.

Most likely this was used to display boolean attributes with Yes/No values.

@orlangur Yes/No values at this point already handled in \Magento\Eav\Model\Entity\Attribute\Frontend\AbstractFrontend::getValue. And this is dangerous to substitute empty value with "No" as empty value may appear for different reasons (e.g. empty text field, dropdown without selected item, etc.). Magento should rid of such code, decrease coupling between different classes and do not base implementation on current execution flow but build reusable components with high cohesion so changes in one system part will not cause bugs in another.

vkublytskyi · 2017-11-23T13:31:17Z

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

@@ -85,13 +85,15 @@ public function getAdditionalData(array $excludeAttr = [])

                if (!$product->hasData($attribute->getAttributeCode())) {
                    $value = __('N/A');


I think we also should remove this code as there are a lot of complaints about such behavior. This code was "dead" for a long time so everyone expects that not used attribute is ignored on storefront.
Please remove this if

Can we change system behavior in a way desired by Community without PO approval?

Actually, behavior was changed when instanceof Phrase was added to if below. I can see that __('N/A') code was dead for a long time (including M1) so expectations in most Magento versions not used attributes are not displayed on a storefornt product page. Support also has multiple tickets to return previous behavior (not show N/A) back.

Thanks, I wan't aware about M1, and yes, such useful behavior was introduced accidentally in scope of another bugfix.

orlangur · 2017-11-29T17:47:02Z

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

+
+                if ($value instanceof Phrase) {
+                    $value = (string)$value;
+                } if ($attribute->getFrontendInput() == 'price' && is_string($value)) {


This if formatting looks pretty strange, should it be on next line or elseif? For the sake of cleanup === use would be nice also.

Sure. should be elseif. Thanks

…th value N/A or NO on storefront. #12057

Ctucker9233 · 2018-01-18T21:14:15Z

@magento-engcom-team Are these changes safe for 2.1.x? Is there a backport in progress? If not, I'll take it.

p-bystritsky added 2 commits November 6, 2017 14:17

magento#9961: Unused product attributes display with value N/A or NO …

c2a2450

…on storefront.

magento#9961: Unused product attributes display with value N/A or NO …

a5c064a

…on storefront.

orlangur self-assigned this Nov 6, 2017

orlangur requested changes Nov 6, 2017

View reviewed changes

orlangur added the Progress: needs update label Nov 6, 2017

magento#9961: Unused product attributes display with value N/A or NO …

3473c1c

…on storefront.

magento-engcom-team added 2.2.x bugfix Component: Catalog Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017

okorshenko removed the Can not Reproduce label Nov 7, 2017

magento-engcom-team added the Can not Reproduce label Nov 8, 2017

okorshenko removed the Can not Reproduce label Nov 8, 2017

magento-engcom-team added the Can not Reproduce label Nov 8, 2017

okorshenko removed the Can not Reproduce label Nov 8, 2017

vkublytskyi self-assigned this Nov 23, 2017

vkublytskyi added this to the November 2017 milestone Nov 23, 2017

vkublytskyi added the Release Line: 2.2 label Nov 23, 2017

vkublytskyi suggested changes Nov 23, 2017

View reviewed changes

magento-team added Progress: accept and removed Progress: needs update labels Nov 29, 2017

orlangur requested changes Nov 29, 2017

View reviewed changes

vkublytskyi force-pushed the ISSUE-9961 branch from 6533642 to 021070f Compare November 30, 2017 11:39

Resolve conflicts with mainline and clean up code

b3cbda2

vkublytskyi force-pushed the ISSUE-9961 branch from 021070f to b3cbda2 Compare November 30, 2017 11:43

vkublytskyi approved these changes Nov 30, 2017

View reviewed changes

okorshenko merged commit b3cbda2 into magento:2.2-develop Dec 1, 2017

okorshenko pushed a commit that referenced this pull request Dec 1, 2017

MAGETWO-84411: [Backport] #9961: Unused product attributes display wi…

e4a5038

…th value N/A or NO on storefront. #12057

magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Dec 1, 2017

magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Jan 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

p-bystritsky commented Nov 6, 2017

orlangur commented Nov 6, 2017

orlangur Nov 6, 2017

p-bystritsky Nov 6, 2017

p-bystritsky commented Nov 8, 2017

orlangur commented Nov 8, 2017

vkublytskyi left a comment

vkublytskyi Nov 23, 2017

orlangur Nov 23, 2017

vkublytskyi Nov 24, 2017

vkublytskyi Nov 23, 2017

orlangur Nov 23, 2017

vkublytskyi Nov 24, 2017

orlangur Nov 24, 2017

orlangur Nov 29, 2017

vkublytskyi Nov 30, 2017

Ctucker9233 commented Jan 18, 2018

		@@ -85,13 +85,15 @@ public function getAdditionalData(array $excludeAttr = [])

		if (!$product->hasData($attribute->getAttributeCode())) {
		$value = __('N/A');

[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

[Backport] magento/magento2#9961: Unused product attributes display with value N/A or NO on storefront. #12057

Conversation

p-bystritsky commented Nov 6, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

orlangur commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-bystritsky commented Nov 8, 2017

orlangur commented Nov 8, 2017

vkublytskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ctucker9233 commented Jan 18, 2018