Replace hard-coded list of category attributes #28216

fredden · 2020-05-13T20:36:14Z

Description

Magento\Catalog\Model\Category\DataProvider::getFieldsMap() had a hard-coded list of category attributes. This pull request replaces this with a list generated from cache. The cache is built from reading view/adminhtml/ui_component/category_form.xml files, so this dynamically generated list includes all category attributes, not only those in the hard-coded list.

Fixed Issues

Fixes Adding custom attribute to category doesn't show store specific value #13440 - Adding custom attribute to category doesn't show store specific value

Related Pull Requests

https://github.com/magento/partners-magento2ee/pull/267

Manual testing scenarios

See issue #13440 for reproduction steps, expected result, and actual result.

Questions or comments

I have not yet updated any tests, however I have tested this locally.
magento/module-catalog-staging will need to also be updated for Commerce as a similar hard-coded list exists there and that class extends this. - See https://github.com/magento/partners-magento2ee/pull/267

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 are green)

m2-assistant · 2020-05-13T20:36:16Z

Hi @fredden. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered only manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s). <test-build(s)> is a comma-separated list of build names. Allowed build names are Database Compare, Functional Tests CE, Functional Tests EE, Functional Tests B2B, Integration Tests, Magento Health Index, Sample Data Tests CE, Sample Data Tests EE, Sample Data Tests B2B, Static Tests, Unit Tests, WebAPI Tests.

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

FredericMartinez · 2020-06-05T16:04:49Z

It works in 2.3.5-p1
Thanks @fredden

marvinhinz · 2020-07-10T13:51:46Z

@magento run all tests

thampe · 2020-07-10T14:05:58Z

This looks good, I also tested it in 2.3.5-p1.

marvinhinz · 2020-07-10T14:44:51Z

@magento-engcom-team any idea why there are no build reports?

marvinhinz · 2020-07-13T14:55:40Z

@magento run all tests

…category_form-hard-coded-list

…list

lenaorobei · 2020-08-17T17:19:56Z

@magento run all tests

lenaorobei · 2020-08-17T17:21:14Z

app/code/Magento/Catalog/Model/Category/DataProvider.php

@@ -233,6 +241,7 @@ public function __construct(
        $this->storeManager = $storeManager;
        $this->request = $request;
        $this->categoryFactory = $categoryFactory;
+        $this->uiConfigFactory = $uiConfigFactory;


To keep backward compatibility let's add nullable argument.

As far as I know, Magento's stance on extension is to prefer composition over inheritance. If a third party has extended this model via a preference (which is documented as "a method of last resort"), then keeping their extension up to date with changes here is a necessary cost to using such a technique. I do not see any benefit in using a nullable argument here.

My objections aside, I do not know how to create this object if not via dependency injection. (I have no experience with the Object Manager as "Magento prohibits the direct use of the ObjectManager" and any existing usages "are not tacit endorsements of using the ObjectManager directly." I understand this exists as an internal implementation detail of Magento's dependency injection system.)
How should I create this virtualType if not supplied via dependency injection?

magento2/app/code/Magento/Ui/etc/di.xml

Lines 339 to 343 in 8b7d949

<virtualType name="uiComponentConfigFactory" type="Magento\Framework\Config\DataInterfaceFactory">

<arguments>

<argument xsi:type="string" name="instanceName">Magento\Ui\Config\Data</argument>

</arguments>

</virtualType>

You are absolutely right about composition over inheritance principle. However, constructor modification is considered as a braking change. As a workaround, we recommend adding new optional parameter to the constructor at the end of the arguments list.

Please refer to the Backward compatible development guide and section about Adding a constructor parameter.

lenaorobei · 2020-08-17T17:24:05Z

app/code/Magento/Catalog/Model/Category/DataProvider.php

+            }
+
+            // Remove empty groups
+            if (empty($fieldsMap[$group])) {


I believe we can remove this along with assignment in the line 674.

Technically we can remove this and the assignment on 674. I'd prefer to keep these however, as I feel like this method is more explicit in intent: showing clearly that empty groups are omitted on purpose.

The PHP manual also suggests that implicit array creation is discouraged. (Emphasis is mine.)

https://www.php.net/manual/en/language.types.array.php#language.types.array.syntax.modifying

If $arr doesn't exist yet, it will be created, so this is also an alternative way to create an array. This practice is however discouraged because if $arr already contains some value (e.g. string from request variable) then this value will stay in the place and [] may actually stand for string access operator. It is always better to initialize a variable by a direct assignment.

In this case I'd prefer creating array variable to collect needed data and if it is not empty, write this data to $fieldsMap[$group].

lenaorobei

Please see review comments.

Looks like the fix should be applied to commerce repo. You can easily open commerce PR and link this this one through PR description.

engcom-Foxtrot · 2020-09-22T09:35:39Z

@magento run all tests

…e specific value - SVC test fix.

engcom-Foxtrot · 2020-09-22T14:25:52Z

@magento run all tests

engcom-Alfa · 2020-10-01T09:10:45Z

✔️ QA Passed

Manual testing scenario:

Create new module with Setup InstallData.php
Install new category attribute following Magento's current category attribute array.
magento-bug2.txt
Add view/adminhtml/ui_component/category_form.xml for above attribute
magento-bug.txt
php bin/magento setup:upgrade
Set different values per scope level using store switcher

Before:
✖️ Can't see [Store View] label under attribute label.
✖️ Can't see different values per scope level (global, website, store)

After:
✔️ [Store View] label under attribute label.
✔️ Able to see different values per scope level (global, website, store)

On Storefront:

On Admin panel:

…list

engcom-Foxtrot · 2020-10-01T13:37:14Z

@magento run all tests

engcom-Foxtrot · 2020-10-02T08:46:22Z

@magento run all tests

m2-assistant · 2020-10-07T20:18:38Z

Hi @fredden, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Replace hard-coded list of category attributes

e9511e8

ghost added the Progress: pending review label May 13, 2020

magento-engcom-team added Component: Catalog Release Line: 2.4 Partner: Fisheye partners-contribution Pull Request is created by Magento Partner labels May 13, 2020

fredden added 7 commits July 13, 2020 17:51

Merge remote-tracking branch 'upstream/2.4-develop' into uiComponent/…

af9f897

…category_form-hard-coded-list

Fix unit test

3bd5fa0

Add special handling of <container/> config nodes

471dc6b

Merge remote-tracking branch 'upstream/2.4-develop' into uiComponent/…

fd5b12e

…category_form-hard-coded-list

Skip disabled components

d3e011c

Avoid use of array_merge() to appease code linter

8d899e1

Declare missing property

d62b0b5

Merge branch '2.4-develop' into uiComponent/category_form-hard-coded-…

223a7f5

…list

lenaorobei reviewed Aug 17, 2020

View reviewed changes

ghost assigned lenaorobei Aug 17, 2020

lenaorobei reviewed Aug 17, 2020

View reviewed changes

lenaorobei requested changes Aug 17, 2020

View reviewed changes

ghost removed the Progress: pending review label Aug 17, 2020

magento#13440 - Adding custom attribute to category doesn't show stor…

cebb5be

…e specific value - SVC test fix.

engcom-Foxtrot removed the Progress: needs update label Sep 23, 2020

engcom-Foxtrot requested a review from lenaorobei September 23, 2020 11:13

ghost added the Progress: pending review label Sep 23, 2020

lenaorobei added Risk: medium Auto-Tests: Covered All changes in Pull Request is covered by auto-tests labels Sep 25, 2020

lenaorobei approved these changes Sep 25, 2020

View reviewed changes

ghost added Progress: waiting related PRs Progress: ready for testing and removed Progress: pending review Progress: waiting related PRs labels Sep 25, 2020

engcom-Alfa self-assigned this Sep 30, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Sep 30, 2020

engcom-Alfa added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Oct 1, 2020

ghost added Progress: extended testing and removed Progress: testing in progress labels Oct 1, 2020

Merge branch '2.4-develop' into uiComponent/category_form-hard-coded-…

c3ce246

…list

ghost added Progress: accept and removed Progress: extended testing labels Oct 6, 2020

magento-engcom-team mentioned this pull request Oct 7, 2020

Adding custom attribute to category doesn't show store specific value #13440

Closed

magento-engcom-team pushed a commit that referenced this pull request Oct 7, 2020

ENGCOM-8311: Replace hard-coded list of category attributes #28216

27dc18c

magento-engcom-team merged commit be1f880 into magento:2.4-develop Oct 7, 2020

fredden deleted the uiComponent/category_form-hard-coded-list branch October 7, 2020 21:19

	<virtualType name="uiComponentConfigFactory" type="Magento\Framework\Config\DataInterfaceFactory">
	<arguments>
	<argument xsi:type="string" name="instanceName">Magento\Ui\Config\Data</argument>
	</arguments>
	</virtualType>

Replace hard-coded list of category attributes #28216

Replace hard-coded list of category attributes #28216

Uh oh!

Conversation

fredden commented May 13, 2020 • edited by lenaorobei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues

Related Pull Requests

Manual testing scenarios

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented May 13, 2020

Uh oh!

FredericMartinez commented Jun 5, 2020

Uh oh!

marvinhinz commented Jul 10, 2020

Uh oh!

thampe commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marvinhinz commented Jul 10, 2020

Uh oh!

marvinhinz commented Jul 13, 2020

Uh oh!

lenaorobei commented Aug 17, 2020

Uh oh!

lenaorobei Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

fredden Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

lenaorobei Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

lenaorobei Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

fredden Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

lenaorobei Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

lenaorobei left a comment

Choose a reason for hiding this comment

Uh oh!

engcom-Foxtrot commented Sep 22, 2020

Uh oh!

engcom-Foxtrot commented Sep 22, 2020

Uh oh!

engcom-Alfa commented Oct 1, 2020

Uh oh!

engcom-Foxtrot commented Oct 1, 2020

Uh oh!

engcom-Foxtrot commented Oct 2, 2020

Uh oh!

m2-assistant bot commented Oct 7, 2020

Uh oh!

Uh oh!

fredden commented May 13, 2020 •

edited by lenaorobei

Loading

thampe commented Jul 10, 2020 •

edited

Loading