Add abstract method execute() to \Magento\Framework\App\Action\Action

[Vinai]

Purpose

Make implementing Actions more developer friendly via self-documenting code.

Background

The existing method \Magento\Framework\App\Action\Action::dispatch() calls an execute() method without actually implementing it.

$result = $this->execute();

Any subclass actions simply need to implement that method and it will be called automatically when the superclass method is dispatch() is called.

Without the changes in this PR, the execute() method in effect was part of an implicit interface exposed by the Action class.

Because of this the Action class could never be instantiated and used (that is, dispatched), without first subclassing it.
This in turn makes the Action class an implicitly abstract class.

All this PR does it that it makes the class interface explicit by adding the execute method as an abstract method to \Magento\Framework\App\Action\Action.

All the other changes in this PR are just follow-up changes to accommodate for this one update.

Follow up changes

Because concrete classes can't have abstract methods, the Action class itself also had to be made abstract.

A number of subclasses of Action that did not implement execute(), because they themselves also needed to be extended, had to be declared as abstract classes now, too, because they inherit the abstract method from the Action superclass.

There where some tests that instantiated these implicitly abstract classes.
This PR adds stub implementations for each of these.

• Magento\Checkout\Test\Unit\Controller\OnepageTest in app/code/Magento/Checkout/Test/Unit/Controller/
• Magento\Contact\Test\Unit\Controller\IndexTest in app/code/Magento/Contact/Test/Unit/Controller/
• Magento\Tax\Test\Unit\App\Action\ContextPluginTest in app/code/Magento/Tax/Test/Unit/App/Action/
• Magento\Catalog\Helper\Product\ViewTest in dev/tests/integration/testsuite/Magento/Catalog/Helper/Product/
• Magento\Cms\Helper\PageTest in dev/tests/integration/testsuite/Magento/Cms/Helper/Stub/
• Magento\Sales\Controller\Adminhtml\Order\CreateTest in dev/tests/integration/testsuite/Magento/Sales/Controller/Adminhtml/Order/

Finally, the Magento\Ui\Controller\UiActionInterface class, which was implemented by an abstract subclass of Action, namely \Magento\Ui\Controller\Adminhtml\AbstractAction, contained a
public function execute(); itself.
This explicit interface contract was in conflict with the implicit interface of the Action class, which did defined the same method, but implicitly.

To resolve this conflict this PR changes the method exposed by the UiActionInterface to public function executeAjaxRequest();. This seemed the most fitting when looking at the code implementing the interface method. Internally it only calls the now explicit execute() method inherited from the Action interface.

Also, it seems as if the UiActionInterface::execute() method was only called by a single test. However, I realize the method might be used a lot more in the non-public EE code, which will also need to be adjusted to be compatible with this PR.

Reasons for this PR

As stated above, the purpose of this PR is to make the developer experience better. This is done by making an implicit interface explicit. This has a whole number of benefits, besides being good OOP practice:

• It is clear which classes should be instantiated and which should not
• It is clear which method needs to be implemented when subclassing Action
• The expected return type of execute() is documented in the phpdoc block
• When subclassing Action the IDE can be used to quickly a method stub

Allover the changes lead to more self-documenting code that tells developers to they should use it.

Potential reasons against this PR

The explicit abstract execute() method added in this PR is declared with protected visibility.
This basically removes it as a valid extension point for plugins. However, since the method was not declared as public previously (in fact, is wasn't declared at all), it should not have been used as an extension point for plugins anyway.

So the main reason against this PR I can think of is that the EE code will need to be adjusted (plus, I might be missing many obvious reasons).
Of course I'd be happy to make the required updates, but I realize that is problematic due to the commercial licenced, non-open code.
I've got a bunch of hacky shell scripts to find and update any controllers needed to be declared abstract, but I'd rather not share them as they are just one-time hacks.
All the other changes - updating the ui action interface and the test stubs - where done manually and didn't take long.

Additional notes

This PR would resolve issue #771.

[orlangur]

Why not just use getMockForAbstractClass or real child classes?

[Vinai]

I didn't want to use a real subclass because that would potentially change the test behavior. Wanted to keep it as close to the current state as possible.
Also didn't use getMockForAbstractClass since I wanted to try to stick with real code as much as possible within the integration tests. A simple stub implementation seemed like the least intrusive way to go.
I don't really care though. If you prefer a mock of the abstract instead, no prob.

[gordonknoppe]

👍

[vrann]

Thanks @Vinai changes looks good, it will be accepted soon. Internal tracking number MAGETWO-39879

[Vinai]

Thanks @vrann, I'm very happy to hear that!