Deprecate result factories in favor of one generic

[ihor-sviziev]

Description (*)

Issue

In magento we have a mess with result objects in adminhtml controllers. When you want to add new result type, for instance add json result - you have three options

1. Inject new type with as constructor argument
   

   magento2/app/code/Magento/Cms/Controller/Adminhtml/Wysiwyg/Images/Upload.php

   Line 39 in 20069c4

   $this->resultJsonFactory = $resultJsonFactory;

   
   

   magento2/app/code/Magento/Cms/Controller/Adminhtml/Wysiwyg/Images/Upload.php

   Line 65 in 20069c4

   $resultJson = $this->resultJsonFactory->create();

2. Inject it via ObjectManger (really-really bad, but we could do that):
   

   magento2/app/code/Magento/Catalog/Controller/Adminhtml/Category/Add.php

   Line 62 in 20069c4

   $resultPageFactory = $this->_objectManager->get(\Magento\Framework\View\Result\PageFactory::class);

   
   

   magento2/app/code/Magento/Catalog/Controller/Adminhtml/Category/Add.php

   Line 64 in 20069c4

   $resultPage = $resultPageFactory->create();

3. Use already existing resultFactory object:
   

   magento2/lib/internal/Magento/Framework/App/Action/AbstractAction.php

   Line 44 in 20069c4

   $this->resultFactory = $context->getResultFactory();

   
   

   magento2/app/code/Magento/AdminNotification/Controller/Adminhtml/System/Message/ListAction.php

   Line 72 in 20069c4

   $resultJson = $this->resultFactory->create(ResultFactory::TYPE_JSON);

Examples

In many places we're using 1st option that for different types causes not needed code duplicates, examples:

magento2/app/code/Magento/Sales/Controller/Adminhtml/Order.php

Lines 111 to 114 in 20069c4

	\Magento\Framework\View\Result\PageFactory $resultPageFactory,
	\Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory,
	\Magento\Framework\View\Result\LayoutFactory $resultLayoutFactory,
	\Magento\Framework\Controller\Result\RawFactory $resultRawFactory,

magento2/app/code/Magento/Checkout/Controller/Onepage.php

Lines 114 to 117 in 20069c4

	\Magento\Framework\View\Result\PageFactory $resultPageFactory,
	\Magento\Framework\View\Result\LayoutFactory $resultLayoutFactory,
	\Magento\Framework\Controller\Result\RawFactory $resultRawFactory,
	\Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory

magento2/app/code/Magento/Customer/Controller/Adminhtml/Index.php

Lines 222 to 225 in 20069c4

	\Magento\Framework\View\Result\LayoutFactory $resultLayoutFactory,
	\Magento\Framework\View\Result\PageFactory $resultPageFactory,
	\Magento\Backend\Model\View\Result\ForwardFactory $resultForwardFactory,
	\Magento\Framework\Controller\Result\JsonFactory $resultJsonFactory

Solution

My PR marks all specific result factories as @deprecated in favor of one generic factory that will have the same result, so we'll have only one option how to create result objects. It should decrease amount of mess in controllers.

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

N/A

Questions or comments

I checked - option 3 works correctly in admin because it replaces result object types:

magento2/app/code/Magento/Backend/etc/adminhtml/di.xml

Lines 97 to 114 in 20069c4

	</type>
	<type name="Magento\Framework\Controller\ResultFactory">
	<arguments>
	<argument name="typeMap" xsi:type="array">
	<item name="redirect" xsi:type="array">
	<item name="type" xsi:type="const">Magento\Framework\Controller\ResultFactory::TYPE_REDIRECT</item>
	<item name="class" xsi:type="string">Magento\Backend\Model\View\Result\Redirect</item>
	</item>
	<item name="page" xsi:type="array">
	<item name="type" xsi:type="const">Magento\Framework\Controller\ResultFactory::TYPE_PAGE</item>
	<item name="class" xsi:type="string">Magento\Backend\Model\View\Result\Page</item>
	</item>
	<item name="forward" xsi:type="array">
	<item name="type" xsi:type="const">Magento\Framework\Controller\ResultFactory::TYPE_FORWARD</item>
	<item name="class" xsi:type="string">Magento\Backend\Model\View\Result\Forward</item>
	</item>
	</argument>
	</arguments>

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]

Hi @ihor-sviziev. 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.3-develop instance - deploy vanilla Magento instance

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

[magento-engcom-team]

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

[ihor-sviziev]

For now moved to this PR "on hold" till it be discussed with Magento Architects

[orlangur]

@ihor-sviziev are we even able to add new deprecations within 2.3.x release line?

[orlangur]

https://testing-service.magento-community.engineering/reports/magento/magento2/pull/24711/7e54f79e68bbc21bc69ccadb41cd037a/SemanticVersionChecker/report-magento2.html LOL :) So naive test implementation. I suppose it is even based on regular expressions.

[buskamuza]

Summary of Arch discussion around this topic:

1. the problem with multiple result factories used in one action controller is caused by incorrect object decomposition. With correct implementation (desired state), only one result type is necessary in one action controller
2. Specialized factories provide better interface because they declare returned type. While generic ones don't.

Based on this, we all agreed that it's better to use specialized factories than generic one. And from the three described variants, 1st one (factory injected in constructor) should be a guideline.
2nd option is discouraged and should not be used. Maybe somebody implemented it for backward compatibility (though it's unnecessary in action controllers).
3rd option also don't look like a good approach. Probably it was just an "automatic" aggregation of parameters for action controllers with many of them.

[ihor-sviziev]

Summary of Arch discussion around this topic:

1. the problem with multiple result factories used in one action controller is caused by incorrect object decomposition. With correct implementation (desired state), only one result type is necessary in one action controller
2. Specialized factories provide better interface because they declare returned type. While generic ones don't.

Based on this, we all agreed that it's better to use specialized factories than generic one. And from the three described variants, 1st one (factory injected in constructor) should be a guideline.
2nd option is discouraged and should not be used. Maybe somebody implemented it for backward compatibility (though it's unnecessary in action controllers).
3rd option also don't look like a good approach. Probably it was just an "automatic" aggregation of parameters for action controllers with many of them.

So in this case should we deprecate generic ResultFactory?

[orlangur]

Great @buskamuza!

With correct implementation (desired state), only one result type is necessary in one action controller

Missed this when we discussed in Slack, really nice catch 👍

[ihor-sviziev]

@buskamuza,

1. With correct implementation (desired state), only one result type is necessary in one action controller

Actually not, in many places we'll have two types of result - Page & Forward.

Example
On frontend we have catalog/product/view action. In case if product isn't enabled on website or such product ID doesn't exists in DB - we need to return 404 page. How to do that - return two types of results - when we have product - return Page result, if we don't have - return forward to "no route" action.

We could apply this example to any entity on frontend, result will be the same.

I see your points, but still don't think that introducing multiple dependencies instead of one - good idea.

Am I missed something?

2. Specialized factories provide better interface because they declare returned type. While generic ones don't.

Sure, this is good point. Maybe we need to introduce new result factory that will have methods "createPageResult", "createRedirectResult"...? Actually currently we have 2 types of results - frontend & backend, they have the same interface

[m2-assistant]

Hi @ihor-sviziev, 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.

[buskamuza]

Actually not, in many places we'll have two types of result - Page & Forward.

Yes, we have such places.
I looked at a few action controllers and there are cases where:

• a single Forward factory is used
• a single another (Redirect/Page/etc) factory is used
• combination of two different types are used. It's not only Page and Forward. There are cases of using Redirect and Forward. So having a single factory with Page and Forward wouldn't be very useful.

I think this topic is partially subjective. As for me, having two factories injected is a lesser issue than having a single factory with a non-strict interface. So I'd prefer having multiple factories. Especially in action controllers, which are (or should be) small and having one more dependency should not be a big issue. In the same time it would clearly show the dependencies used.
If we would need Page and Forward factories together in all (or almost all) cases, it would definitely make sense for me to combine them in one interface. But because this doesn't look to be the case (though I didn't collect full statistics), I'd rather avoid combining them.

[ihor-sviziev]

@buskamuza ok, so another question. If using "pageFactory" is not really good option - maybe we need to mark this function as deprecated with description why and alternatives?

Why I started thinking about it - because another people creates PRs that uses this approach, example:
https://github.com/magento/magento2/pull/26923/files#diff-0c891006ed5654828682a33f2add24abR63