Added a dedicated modifier pool for categories

[bgorski]

Description (*)

This PR greatly decreases chances for conflicts if your application has more than one custom or 3rd party module extending category edit form using modifiers.

The problem:
As we know, ui component forms can be extended either in xml files or using modifiers written in PHP and injected via modifier pools to their corresponding form.
This is easy for products, the etc/adminhtml/di.xml file in the Magento_Catalog module defines a Magento\Catalog\Ui\DataProvider\Product\Form\Modifier\Pool virtual type with some modifiers already there and injects it as the pool argument for Magento\Catalog\Ui\DataProvider\Product\Form\ProductDataProvider. If you want to add your own argument, you simply target that virtual class in your own di.xml and add a new item to the modifiers array.
However, in case of categories it's not that straightforward. Although the overall logic is the same, categories don't have any virtual type for the pool defined. So they get null as the pool argument and end up with the generic Magento\Ui\DataProvider\Modifier\Pool as the pool, which is added in the parent constructor of the data provider. Needless to say, you can't add your own modifiers that are related to category form to that generic pool.
This means that if you want to add modifiers to category form, you need to create your own virtualType for the pool, add your modifier to it and inject it as the pool argument for the data provider for the category form. Which works if you happen to have only one module doing that, but if you have more than one, a conflict is guaranteed (they last one to load wins and overwrites the pool argument with its own pool).
A solution to that is adding a new virtualType for that pool and injecting it as the pool argument directly in the Magento_Core. Although that pool doesn't contain any actual modifiers by default, it serves as a point to which custom/3rd party modules can hook to and add their own modifiers without conflicts.
This is also a backwards-compatible solution, because if you happen to already have some module adding its own pool to the category form, that pool will overwrite the empty one added in this PR (assuming you have a correct sequence in your module.xml, with Magento_Catalog added there).

Manual testing scenarios (*)

1. None really. Go to a category edit page in the admin panel (which is the only page loading the affected functionality) for any category and ensure it still works if you want to test something.

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)

Resolved issues:

1. resolves [Issue] Added a dedicated modifier pool for categories #30134: Added a dedicated modifier pool for categories

[m2-assistant]

Hi @bgorski. 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 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)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests

You can find more information about the builds here

ℹ️ 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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[bgorski]

@magento run all tests

[bgorski]

@magento run Functional Tests B2B

[rogyar]

Hi @bgorski. Thank you for your improvement. From my point of view, that's a good idea since I met a similar problem with the virtual type overriding for the pull. Could we create a simple integration test that ensures that the pool of category data provider is not null after the change introduced in this PR?

[sidolov]

@magento create issue

[bgorski]

Hi @bgorski. Thank you for your improvement. From my point of view, that's a good idea since I met a similar problem with the virtual type overriding for the pull. Could we create a simple integration test that ensures that the pool of category data provider is not null after the change introduced in this PR?

The pool wouldn't be null before this PR is introduced, because the parent constructor assigns a generic Magento\Ui\DataProvider\Modifier\Pool. So we could only create a test testing the object creation (on the object manager level) instead of testing an already created object. However, now I'm thinking that we could as well change the constructor head line:
PoolInterface $pool = null,
to just
PoolInterface $pool,
So this would be indirectly tested by already existing integration tests. Meaning that if it was passed null, it would simply throw Type Error occurred when creating object: Magento\\Catalog\\Model\\Category\\DataProvider\\Interceptor, Argument 13 passed to Magento\\Catalog\\Model\\Category\\DataProvider::__construct() must implement interface Magento\\Ui\\DataProvider\\Modifier\\PoolInterface, null given
@rogyar what do you think?

[bgorski]

@magento run all tests

[bgorski]

I went ahead and did exactly what I mentioned in my previous comment. The integration test that will test the correct instantiation of the data provider is dev/tests/integration/testsuite/Magento/Catalog/Model/Category/DataProviderTest.php

[bgorski]

@magento run all tests

[bgorski]

@magento run all tests

[bgorski]

Actually I reverted the change above. It would be a major change as the data provider is marked with @api. I don't think it's worth it at this point, but I think it would be best to introduce that as a second PR instead of creating a new integration test for the reason I mentioned in one of my comments above - the parent constructor assigns the generic pool, so the change from that null to a pool object could only be checked at the object manager level (before instantiating the data provider). At least without providing a custom di.xml that adds something to the pool of the new virtualType to make the pool testable this way. I haven't done anything like that in any integration tests, though. Is that possible and worth it?

[bgorski]

@magento run all tests

[bgorski]

@magento run Functional Tests B2B, Functional Tests CE

[rogyar]

Hi @bgorski. Thank you for providing the extended information on that matter. Let's wait with the complex test and check for approval from architects for this change.

Thank you.

[rogyar]

Approving this PR with "Not test required option". I see no easy way to cover this case with an automated test without developing a separate modifier

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-8956 has been created to process this Pull Request

[engcom-Dash]

Hello @bgorski

Thanks for the collaboration & contribution!

✔️ QA Passed
Preconditions:

Install fresh Magento 2.4-develop
Steps to reproduce

• As per the description

Category edit page is working as expected with the changes from PR.

Builds are failed. Hence, moving this PR to Extended Testing.

Thanks

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run Unit Tests, Magento Health Index, Functional Tests B2B

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run Functional Tests EE, Functional Tests B2B

[engcom-Dash]

The Functional B2B test failures are different in recent 2 successive builds. They neither part of PR nor failing because of the PR changes, hence moving it to Merge in Progress.

Run 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/6e2c6d60cd098595f84235f49bb79ba1/Functional/allure-report-b2b/index.html#categories/bcf93358aa1b517577cad18e5921ef41/1d255ca3873ee1db/

Run 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/a770080f722523271b871a6c7d7f9a3c/Functional/allure-report-b2b/index.html#categories/6cd9617c5e67638d802b7c193afd3e9b/9ab34cb795cc76de/

[engcom-Dash]

Hello @bgorski,

Thanks for your contributions!

Moving this PR to on hold since it’s a feature request and we need PO approval for it. Once we get approval, we will proceed accordingly.

Thanks.

[engcom-Dash]

Hello @bgorski,

We have received the go-ahead on this PR from the PO. We are moving it to extended testing for now to analyze the build results with the latest code.

Thanks.

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run Functional Tests CE, Functional Tests B2B

[engcom-Dash]

The Functional B2B and CE test failures seem flaky and some of these failures are known issues. They are neither part of PR nor failing because of PR changes. Hence moving it to Merge in Progress.

Functional Tests B2B
Run 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/0212e875e1873c13065fa73a0d248b8a/Functional/allure-report-b2b/index.html#categories/bdbf199525818fae7a8651db9eafe741

Run 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/6f8308dd72248c56d307606d7399eb3f/Functional/allure-report-b2b/index.html#categories/6ca9d315ebd9ab54dbe1fb9148ebdc1f/cb5e9612ef6d1ddf/

Functional Tests CE
Run 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/c688cf3bf9118608762e0a4c83b623a7/Functional/allure-report-ce/index.html#categories/7fdf0257964d90d5e088c5e5e0fce79b/d30937b3ac92d3fc/

Run 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/30119/c179751456bcc47c25026d0d4bab2963/Functional/allure-report-ce/index.html#categories/75b40a63f79830217aae704c6d6af940/7c66665d979637/

Known Issue:
StoreFrontAddOutOfStockProductToShoppingCartTestCest: ACQE-6818

[engcom-Dash]

Moving this PR to Extended Testing to resolve the conflicts

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run Functional Tests B2B, Unit Tests

[engcom-Dash]

@magento run Functional Tests B2B, Unit Tests

[engcom-Dash]

@magento run all tests