Skip to content

Commit

Permalink
Add abstract method execute() to \Magento\Framework\App\Action\Action
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Vinai committed Jun 26, 2015
1 parent 8158c8c commit f8c8f55
Show file tree
Hide file tree
Showing 116 changed files with 199 additions and 119 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/
namespace Magento\AdminNotification\Controller\Adminhtml;

class Notification extends \Magento\Backend\App\AbstractAction
abstract class Notification extends \Magento\Backend\App\AbstractAction
{
/**
* @return bool
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Backend/App/Action.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@
/**
* @SuppressWarnings(PHPMD.NumberOfChildren)
*/
class Action extends \Magento\Backend\App\AbstractAction
abstract class Action extends \Magento\Backend\App\AbstractAction
{
}
2 changes: 1 addition & 1 deletion app/code/Magento/Backend/Controller/Adminhtml/Auth.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/**
* Auth backend controller
*/
class Auth extends AbstractAction
abstract class Auth extends AbstractAction
{
/**
* Check if user has permissions to access this controller
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Backend/Controller/Adminhtml/Cache.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Magento\Backend\App\Action;
use Magento\Framework\Exception\LocalizedException;

class Cache extends Action
abstract class Cache extends Action
{
/**
* @var \Magento\Framework\App\Cache\TypeListInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/
namespace Magento\Backend\Controller\Adminhtml;

class Dashboard extends \Magento\Backend\App\Action
abstract class Dashboard extends \Magento\Backend\App\Action
{
/**
* @return bool
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Backend/Controller/Adminhtml/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/**
* Index backend controller
*/
class Index extends AbstractAction
abstract class Index extends AbstractAction
{
/**
* Check if user has permissions to access this controller
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Backend/Controller/Adminhtml/System.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class System extends AbstractAction
abstract class System extends AbstractAction
{
/**
* @return bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Account extends Action
abstract class Account extends Action
{
/**
* @return bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

use Magento\Backend\App\Action;

class Design extends Action
abstract class Design extends Action
{
/**
* Core registry
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Store extends Action
abstract class Store extends Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Backup/Controller/Adminhtml/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Index extends \Magento\Backend\App\Action
abstract class Index extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Catalog/Controller/Adminhtml/Category.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
/**
* Catalog category controller
*/
class Category extends \Magento\Backend\App\Action
abstract class Category extends \Magento\Backend\App\Action
{
/**
* Initialize requested category and put it into registry.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Widget extends \Magento\Backend\App\Action
abstract class Widget extends \Magento\Backend\App\Action
{
/**
* @var \Magento\Framework\View\LayoutFactory
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Catalog/Controller/Adminhtml/Product.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* Catalog product controller
* @SuppressWarnings(PHPMD.NumberOfChildren)
*/
class Product extends \Magento\Backend\App\Action
abstract class Product extends \Magento\Backend\App\Action
{
/**
* @var Product\Builder
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use Magento\Catalog\Controller\Adminhtml\Product;
use Magento\Framework\Controller\Result;

class AbstractProductGrid extends Product
abstract class AbstractProductGrid extends Product
{
/**
* @var \Magento\Framework\View\Result\LayoutFactory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
/**
* Adminhtml catalog product action attribute update controller
*/
class Attribute extends Action
abstract class Attribute extends Action
{
/**
* @var \Magento\Catalog\Helper\Product\Edit\Action\Attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
use Magento\Framework\Controller\Result;
use Magento\Framework\View\Result\PageFactory;

class Attribute extends \Magento\Backend\App\Action
abstract class Attribute extends \Magento\Backend\App\Action
{
/**
* @var \Magento\Framework\Cache\FrontendInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Set extends \Magento\Backend\App\Action
abstract class Set extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Catalog/Controller/Product.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use Magento\Catalog\Controller\Product\View\ViewInterface;
use Magento\Catalog\Model\Product as ModelProduct;

class Product extends \Magento\Framework\App\Action\Action implements ViewInterface
abstract class Product extends \Magento\Framework\App\Action\Action implements ViewInterface
{
/**
* Initialize requested product object
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Catalog/Controller/Product/Compare.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* @SuppressWarnings(PHPMD.LongVariable)
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Compare extends \Magento\Framework\App\Action\Action
abstract class Compare extends \Magento\Framework\App\Action\Action
{
/**
* Customer id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use Magento\Framework\Registry;
use Magento\Framework\Stdlib\DateTime\Filter\Date;

class Catalog extends Action
abstract class Catalog extends Action
{
/**
* Dirty rules notice message
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

use Magento\Backend\App\Action;

class Widget extends Action
abstract class Widget extends Action
{
/**
* @return bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/**
* Class Widget
*/
class Widget extends Action
abstract class Widget extends Action
{
/**
* @return bool
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Checkout/Controller/Cart.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
/**
* Shopping cart controller
*/
class Cart extends \Magento\Framework\App\Action\Action implements ViewInterface
abstract class Cart extends \Magento\Framework\App\Action\Action implements ViewInterface
{
/**
* @var \Magento\Framework\App\Config\ScopeConfigInterface
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Checkout/Controller/Onepage.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
/**
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Onepage extends Action
abstract class Onepage extends Action
{
/**
* @var array
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ protected function setUp()
$context->expects($this->once())
->method('getEventManager')
->willReturn($this->eventManager);

$this->controller = $objectManager->getObject(
'Magento\Checkout\Controller\Onepage',
'Magento\Checkout\Test\Unit\Controller\Stub\OnepageStub',
[
'context' => $context
]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php


namespace Magento\Checkout\Test\Unit\Controller\Stub;

class OnepageStub extends \Magento\Checkout\Controller\Onepage
{
protected function execute()
{
// Empty method stub for test
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
namespace Magento\CheckoutAgreements\Controller\Adminhtml;

class Agreement extends \Magento\Backend\App\Action
abstract class Agreement extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Cms/Controller/Adminhtml/Block.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Block extends \Magento\Backend\App\Action
abstract class Block extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Images extends \Magento\Backend\App\Action
abstract class Images extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Contact/Controller/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
/**
* Contact index controller
*/
class Index extends \Magento\Framework\App\Action\Action
abstract class Index extends \Magento\Framework\App\Action\Action
{
/**
* Recipient email config path
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function setUp()
)
);

$this->_controller = new \Magento\Contact\Controller\Index(
$this->_controller = new \Magento\Contact\Test\Unit\Controller\Stub\IndexStub(
$context,
$this->getMock('\Magento\Framework\Mail\Template\TransportBuilder', [], [], '', false),
$this->getMockForAbstractClass('\Magento\Framework\Translate\Inline\StateInterface', [], '', false),
Expand Down
12 changes: 12 additions & 0 deletions app/code/Magento/Contact/Test/Unit/Controller/Stub/IndexStub.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php


namespace Magento\Contact\Test\Unit\Controller\Stub;

class IndexStub extends \Magento\Contact\Controller\Index
{
protected function execute()
{
// Empty method stub for test
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/
namespace Magento\CurrencySymbol\Controller\Adminhtml\System;

class Currency extends \Magento\Backend\App\Action
abstract class Currency extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/
namespace Magento\CurrencySymbol\Controller\Adminhtml\System;

class Currencysymbol extends \Magento\Backend\App\Action
abstract class Currencysymbol extends \Magento\Backend\App\Action
{
/**
* Check the permission to run it
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Customer/Controller/Account.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* @SuppressWarnings(PHPMD.NumberOfChildren)
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Account extends \Magento\Framework\App\Action\Action
abstract class Account extends \Magento\Framework\App\Action\Action
{
/**
* List of actions that are allowed for not authorized users
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Customer/Controller/Address.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Address extends \Magento\Framework\App\Action\Action
abstract class Address extends \Magento\Framework\App\Action\Action
{
/**
* @var \Magento\Customer\Model\Session
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Cart extends \Magento\Backend\App\Action
abstract class Cart extends \Magento\Backend\App\Action
{
/**
* Customer we're working with
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Customer/Controller/Adminhtml/Group.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
/**
* Customer groups controller
*/
class Group extends \Magento\Backend\App\Action
abstract class Group extends \Magento\Backend\App\Action
{
/**
* Core registry
Expand Down
2 changes: 1 addition & 1 deletion app/code/Magento/Customer/Controller/Adminhtml/Index.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
* @SuppressWarnings(PHPMD.TooManyFields)
* @SuppressWarnings(PHPMD.NumberOfChildren)
*/
class Index extends \Magento\Backend\App\Action
abstract class Index extends \Magento\Backend\App\Action
{
/**
* @var \Magento\Framework\Validator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*
* @author Magento Core Team <core@magentocommerce.com>
*/
class Validatevat extends \Magento\Backend\App\Action
abstract class Validatevat extends \Magento\Backend\App\Action
{
/**
* Perform customer VAT ID validation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/**
* Catalog composite product configuration controller
*/
class Wishlist extends \Magento\Backend\App\Action
abstract class Wishlist extends \Magento\Backend\App\Action
{
/**
* Wishlist we're working with.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class Editor extends \Magento\Backend\App\Action
abstract class Editor extends \Magento\Backend\App\Action
{
/**
* @var \Magento\Theme\Model\Config
Expand Down
Loading

0 comments on commit f8c8f55

Please sign in to comment.