community-features icon indicating copy to clipboard operation
community-features copied to clipboard

Eliminate the need for inheritance for action controllers

Open magento-engcom-team opened this issue 7 years ago • 5 comments

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

Original Report: https://github.com/magento/magento2/issues/9582 by @antonkril

magento-engcom-team avatar Dec 11 '17 16:12 magento-engcom-team

Basically this feature request was already implemented

  • ✔ implemented in https://github.com/magento/magento2/pull/26778 (thanks @vinai and @lbajsarowicz),
  • ✔ documentation updated in https://github.com/magento/devdocs/pull/6901 (thanks @lbajsarowicz)
  • ✔ new controllers should not use inheritance - enforced by static test in https://github.com/magento/magento2/pull/29339 (thanks @swnsma).

The only thing that still not done

  • ❌controllers still weren't migrated to not use inheritance

Good job! 👍

ihor-sviziev avatar Nov 05 '20 16:11 ihor-sviziev

Migrating controllers would be an unnecessary backward compatibility break. Why not keep them the way they are, and only enforce new ones to use composition?

Vinai avatar Nov 06 '20 08:11 Vinai

@Vinai I would agree with you that migration of all controllers might be not necessary, but I'm strongly believe we should have good examples as much as possible in order to show to extension developers how to use controllers without inheritance and that it's best practice.

Also for some controllers it might be really needed, for instance for controller that handles ESI requests from varnish.

ihor-sviziev avatar Nov 06 '20 09:11 ihor-sviziev

Was using "controller_action_predispatch" to forward requests, by doing something like this

$request->initForward()
            ->setRouteName('routename')
            ->setModuleName('modulename')
            ->setControllerName('index')
            ->setActionName('index')
            ->setDispatched(false);

This does not work any more since the event dispatching was moved to a plugin.

Before, the original matched actions execute() was never run, because it checked if request was dispatched before running it. Now execute() is always run.

Just wanted to report, in case it wasn't intentional to break it. And if some one else have the same problem, solved it by making a Router to handle forwarding instead.

mannebusk avatar Mar 06 '21 21:03 mannebusk

Have you test your case again in latest code branch (2.4-develop github). New approach no-longer used plugin for event dispatch controller_action_predispatch

mrtuvn avatar Mar 07 '21 01:03 mrtuvn