Magento2: Eliminate the need for inheritance for action controllers

[antonkril]

By design, all Magento action controllers should implement \Magento\Framework\App\ActionInterface. But some crucial request processing behavior (event dispatching, authorisation, etc) resides in classes like \Magento\Framework\App\Action\Action and \Magento\Backend\App\AbstractAction. So if an action controller implements the ActionInterface but does not inherit from one of these "layer supertype" classes, it will lose that crucial behavior.

So, Magento module developers do not have a way to create an action controller without using inheritance. To avoid inheritance (see why), the request processing behavior should be extracted from "layer supertypes". Proposed solution is to move the behavior to action controller plugins.

Action controller should contain only its custom behavior.

AC:

• module developer does not have to extend from any class to create a fully functional action controller, implementing \Magento\Framework\App\ActionInterface is enough
• controllers "supertypes" (\Magento\Backend\App\AbstractAction, \Magento\Framework\App\Action\Action, \Magento\Framework\App\Action\AbstractAction) are deprecated
• magento supports both controller implementations (inheritance based and non-inheritance based)
• [optional] controllers are migrated

[mcspronko]

@antonkril Can you please assign me? :)

[maksek]

@mcspronko, thank you for taking the feature, let me know if you need some help with it.

[antonkril]

If @mcspronko takes it, I am sure it will be done :)

[okorshenko]

Hi @mcspronko do you have any progress on this or should we un-assign this ticket from you?

[mcspronko]

Hi @okorshenko I am working on it right now. Please leave it with me. Appreciate it.

[7ochem]

Can you guys remove the [up for grabs] label?

[okorshenko]

Hi @7ochem
Why we should remove this label?

[7ochem]

Hi @okorshenko, because it's not up for grabs anymore as it is being picked up by @mcspronko as I read correctly? Or doesn't it work like that?

[okorshenko]

@7ochem the issue type is still Up For Grabs. If you are interested in this implementation, you can join @mcspronko and work together.

Please let us know if you would like to

[mcspronko]

@7ochem I would be more than happy to share controllers refactoring for all modules. There are lots of work to share :)

[okorshenko]

Hi @mcspronko do you have any progress on this? Thank you

[7ochem]

@mcspronko I did not have time to respond. I'm in the middle of a "private project" (👶) so don't have much time to invest.

I feel @okorshenko is pushing this though because Magento wants to have it in the 2.2 release, am I guessing this right?

If you @mcspronko like to share your progress with me, I could spend any hour I can free up to assist on this.

[okorshenko]

Hi @mcspronko and @7ochem, no pressure from our side. I just want to make sure that we have some progress on that or if you have any questions we will happy to assist you.
If you are not going to work on this, we will unassign this ticket and someone else will pick it up

[7ochem]

@okorshenko I'd like to help out, but I don't know how far @mcspronko has booked progress on this. Would be a pity if I would have to pick this up from the start...

[mcspronko]

Hi guys, i am having limited access to internet, can't push now. I have few
commits for review including additional interfaces and plugins. This will be
with you soon. I am happy to share controllers refactoring once i will
pass review.

[mcspronko]

Hi guys,

Finally I've pushed changes to the mcspronko/magento2 repository. Here is a commit #1 and commit #2 for your review, I am happy for your feedback and discussion.

Proposed changes

Plugin for Magento\Framework\AppActionInterface

The Magento\Framework\App\Action\Plugin\Action class provides all logic which is previously used in the _Magento\Framework\App\Action\Action::dispatch()_ method. Currently I am using around plugin to cover before and after logic. I believe we may consider before and after plugins, however it may lead to code duplication. From other side it might be cleaner from stack trace point of view during debugging action controllers.

This plugins allows to directly use the _Magento\Framework\AppActionInterface_ interface without a need to extend from the _Magento\Framework\App\Action\AbstractAction_ class and the _Magento\Framework\App\Action\Action_ class. The _Magento\Framework\App\Action\Action::_forward()_ method however isn't covered for the moment.

New Magento\Backend\AppActionInterface interface

This interface extends the _Magento\Framework\AppActionInterface_ interface and should be used for all backend action controllers. In this case new plugin can be introduced to cover backend-related operations.

Plugin for Magento\Backend\AppActionInterface interface

This plugin covers logic from _Magento\Backend\App\AbstractAction::dispatch()_ method. The Magento\Backend\AppActionInterface::beforeExecute() method (before plugin) provides logic related to form key/secret key validation, permission checks, locale settings and redirecting a user in case of failed permissions.

The Magento\Backend\App\Action\Plugin\Action requires refactoring as number of dependencies is way bigger than expected. I would like to collect some feedback to move forward.

Permissions Validation

The Magento\Backend\App\Action\Permission\Validator is responsible for validating resource for a given action (see _Magento\Backend\App\AbstractAction::_isAllowed()_ method). The proposal here is to pass list of resources via _di.xml_ to the _Magento\Backend\App\Action\Permission\Validator_ class.
Example:

<type name="Magento\Backend\App\Action\Permission\Validator">
    <arguments>
        <argument name="resources" xsi:type="array">
            <item name="admin_index_index" xsi:type="array">
                <item name="controller" xsi:type="string">Magento\Backend\Controller\Adminhtml\Index\Index</item>
                <item name="resource" xsi:type="string">true</item>
            </item>
            <item name="customer_index_index" xsi:type="array">
                <item name="controller" xsi:type="string">Magento\Customer\Controller\Adminhtml\Index\Index</item>
                <item name="resource" xsi:type="string">Magento_Customer::customer</item>
            </item>
        </argument>
    </arguments>
</type>

Every time a permission should be validated for a given controller we have to pass action controller class name and resource. List of resources may be converted to a class with defined interface for more developer friendly approach.

UPD: After playing with di.xml file I found more friendly configuration we may use to pass resources and controller class names.

    <type name="Magento\Backend\App\Action\Permission\Validator">
      <arguments>
         <argument name="resources" xsi:type="array">
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\Index" xsi:type="string">Magento_Backend::dashboard</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\AjaxBlock" xsi:type="string">Magento_Backend::dashboard</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\CustomersMost" xsi:type="string">Magento_Backend::dashboard</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\CustomersNewest" xsi:type="string">Magento_Backend::dashboard</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\ProductsViewed" xsi:type="string">Magento_Backend::dashboard</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\RefreshStatistics" xsi:type="string">Magento_Reports::statistics</item>
            <item name="Magento\Backend\Controller\Adminhtml\Dashboard\Tunnel" xsi:type="string">Magento_Backend::dashboard</item>
         </argument>
      </arguments>
    </type>

Public Methods to ignore secret key validation

Please refer to a _Magento\Backend\App\AbstractAction::$_publicActions_ variable.
There is an open question on how to provide knowledge to _Magento\Backend\App\Action\SecretKeyValidator_ class which is responsible for secret key validation. List of public actions for validation should be passed. Maybe again, with action controller class names to be validated in the _SecretKeyValidator_ class.

UPD: I found only 4 occurrences where $_publicMethods variable is set. Maybe this is a workaround (read temporary solution) for forms without form key properly set.

Please let me know your thoughts.

cc: @antonkril @7ochem @okorshenko

Thanks,
Max

[mcspronko]

I am starting refactoring Magento 2 controllers with new interfaces. Considering re-visiting backend action interface if it's really needed.

[antonkril]

Sorry for the late review. Here are my comments:

1. Plugins should be separated. (authentication, authorization, secret key, etc.). We need the ability to granularly add/remove behavior to actions. So I don't think we need the Magento\Framework\App\Action\Plugin\Action.
2. Why is Backend Action interface needed?
3. Permission check looks good, but "Authorization" looks like a better name
4. Public actions can be implemented by just disabling the secret key plugin from actions.

[mcspronko]

Thanks @antonkril for the feedback.

1. Can you please clarify, is it enough to have ability to add/remove behavior using plugins or you want to have a separate mechanism for adding/removing behavior for actions?
2. I was under the impression that we can have a separate set of plugins executed for backend and frontend if we would be using different interfaces. For admin controllers Backend Action Interface might be used and for this interface we have plugins assigned. Or it is better to have as you said in #1 flexible way of adding/removing behavior based on area executed.
3. Renaming to Authorization and moving to Magento\Framework\App\Action directory.
4. Thanks for the tip.

Anton, do we want to preserve all events from the \Magento\Framework\App\Action\Action::dispatch() method?

[antonkril]

1. A separate mechanism is preferred, but not required in the scope of this improvement.
2. Application Areas allow us to have different sets of plugins for admin and storefront with the same interface
3. Yes, we have to preserve all the events

[magento-engcom-team]

Hi @mcspronko do you have any progress on this?

[mcspronko]

Hi @magento-engcom-team

I am working on the action items shared by @antonkril and this will be committed very soon.

[magento-engcom-team]

Hi @antonkril. Thank you for the report. We are moving your feature request to the special project. You can track this issue here: https://github.com/magento-engcom/community-features/issues/9