- added release number to all existing @deprecated tag in php classes, methods and properties (app/code/Magento, lib/internal/Magento, setup/src/Magento/Setup)
- added tag @since to all existing php classes, methods and properties (app/code/Magento, lib/internal/Magento, setup/src/Magento/Setup)
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.
```php
$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 issues
==
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.
soumah