Magento 2 is quite a large project as it covers a very broad problem domain. The whole Magento 1 -> Magento 2 switch is a very challenging process so we have to give recognition to Magento 2 development team and their efforts to put everything together.
As you probably know, Magento has different modules, some more complex than others. In previous months I worked on a module that was based on Magento 2’s sales module. I noticed something that I didn’t quite like and went on to explore it further.
I would like to explain this situation and propose some ideas on Magento 2 code architecture.
Dependency injection in Magento 2
Just a quick reminder, Magento 2 uses constructor injection where you type hint constructor arguments in order to get necessary objects. You can type hint either a class or an interface and define your preference in an xml file.
Mass action
So as you probably know, in Magento admin area you have many different grids with data:
You can select multiple items and then perform a “mass action” on them that applies on all of the selected items:
If you are building an extension there might be a need for you to create your own mass action, in the sales grid.
Creating your own mass action
For that to happen you need to create your own controller that has to extend from this class:
Magento/Sales/Controller/Adminhtml/Order/AbstractMassAction.php
<?php
/**
* Copyright © 2016 Magento. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Sales\Controller\Adminhtml\Order;
use Magento\Framework\Model\ResourceModel\Db\Collection\AbstractCollection;
use Magento\Framework\Controller\ResultFactory;
use Magento\Framework\App\ResponseInterface;
use Magento\Framework\Controller\ResultInterface;
use Magento\Backend\App\Action\Context;
use Magento\Ui\Component\MassAction\Filter;
/**
* Class AbstractMassStatus
*/
abstract class AbstractMassAction extends \Magento\Backend\App\Action
{
/**
* @var string
*/
protected $redirectUrl = '*/*/';
/**
* @var \Magento\Ui\Component\MassAction\Filter
*/
protected $filter;
/**
* @var object
*/
protected $collectionFactory;
/**
* @param Context $context
* @param Filter $filter
*/
public function __construct(Context $context, Filter $filter)
{
parent::__construct($context);
$this->filter = $filter;
}
/**
* Execute action
*
* @return \Magento\Backend\Model\View\Result\Redirect
* @throws \Magento\Framework\Exception\LocalizedException|\Exception
*/
public function execute()
{
try {
$collection = $this->filter->getCollection($this->collectionFactory->create());
return $this->massAction($collection);
} catch (\Exception $e) {
$this->messageManager->addError($e->getMessage());
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
return $resultRedirect->setPath($this->redirectUrl);
}
}
/**
* Return component referer url
* TODO: Technical dept referer url should be implement as a part of Action configuration in in appropriate way
*
* @return null|string
*/
protected function getComponentRefererUrl()
{
return $this->filter->getComponentRefererUrl()?: 'sales/*/';
}
/**
* Set status to collection items
*
* @param AbstractCollection $collection
* @return ResponseInterface|ResultInterface
*/
abstract protected function massAction(AbstractCollection $collection);
}
Let’s take a closer look at this line:
$collection = $this->filter->getCollection($this->collectionFactory->create());
The property $this->collectionFactory is not created in the constructor and we need it for this code to work.
Hidden dependency
So if you just extend this class you will end up with a fatal error. It is not obvious that you need collection factory.
If a dependency cannot be seen from the class’s interface, it is a hidden dependency (source).
Since dependencies are passed through constructor in Magento 2, we can consider the constructor as an “interface” in our case for this definition.
We shouldn’t need to examine the entire class in order to find out what it’s subclass might need. That’s one of the reasons why we have dependency injection – to make dependencies visible and explicit.
Possible solutions
I think there are two ways in which we can resolve this issue:
Moving dependency into the constructor
The obvious solution is to move the dependency into constructor and make it visible. The problem with this approach however is that there is no interface or parent class for factories in Magento 2 so we can’t type hint.
Factories are automatically generated by Magento core and I am not sure why don’t they implement an interface. We would then be formally aware of their public contract. The fact that they are generated makes the interface even more necessary.
/var/generation/Magento/Sales/Model/ResourceModel/Order/Shipment/CollectionFactory.php
<?php
namespace Magento\Sales\Model\ResourceModel\Order\Shipment;
/**
* Factory class for @see
* \Magento\Sales\Model\ResourceModel\Order\Shipment\Collection
*/
class CollectionFactory
{
/**
* Object Manager instance
*
* @var \Magento\Framework\ObjectManagerInterface
*/
protected $_objectManager = null;
/**
* Instance name to create
*
* @var string
*/
protected $_instanceName = null;
/**
* Factory constructor
*
* @param \Magento\Framework\ObjectManagerInterface $objectManager
* @param string $instanceName
*/
public function __construct(\Magento\Framework\ObjectManagerInterface $objectManager, $instanceName = '\\Magento\\Sales\\Model\\ResourceModel\\Order\\Shipment\\Collection')
{
$this->_objectManager = $objectManager;
$this->_instanceName = $instanceName;
}
/**
* Create class instance with specified parameters
*
* @param array $data
* @return \Magento\Sales\Model\ResourceModel\Order\Shipment\Collection
*/
public function create(array $data = array())
{
return $this->_objectManager->create($this->_instanceName, $data);
}
}
As you can see, at the moment the create method is there purely by convention. It would be nice if these generated classes would implement an interface like this:
<?php
namespace Magento\Sales\Model\ResourceModel\Order\Shipment;
class CollectionFactory implements FactoryInterface
{
/**/
}
<?php
interface FactoryInterface
{
public function create(array $data = array());
}
That way we could at least type hint in the constructor and then perhaps set the concrete class in di.xml.
Abstract method and retrieving collection from child classes
We could also delegate the whole process of getting the collection to the child class by creating an abstract method.
Child classes would be responsible for returning their collections and the hidden dependency is now removed.
Magento/Sales/Controller/Adminhtml/Order/AbstractMassAction.php
<?php
/**/
abstract class AbstractMassAction extends \Magento\Backend\App\Action
{
/**
* Execute action
*
* @return \Magento\Backend\Model\View\Result\Redirect
* @throws \Magento\Framework\Exception\LocalizedException|\Exception
*/
public function execute()
{
try {
return $this->massAction($this->getCollection());
} catch (\Exception $e) {
$this->messageManager->addError($e->getMessage());
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultFactory->create(ResultFactory::TYPE_REDIRECT);
return $resultRedirect->setPath($this->redirectUrl);
}
}
/**/
/**
* return \Magento\Framework\Data\Collection\AbstractDb
*/
abstract protected function getCollection();
}
The getCollection method now has to be implemented in child classes:
/app/code/Magento/Sales/Controller/Adminhtml/Order/MassHold.php
<?php
/**/
use Magento\Ui\Component\MassAction\Filter;
/**/
class MassHold extends \Magento\Sales\Controller\Adminhtml\Order\AbstractMassAction
{
/**
* @var OrderManagementInterface
*/
protected $orderManagement;
/**
* @var Filter
*/
protected $filter;
/**
* @param Context $context
* @param Filter $filter
* @param CollectionFactory $collectionFactory
* @param OrderManagementInterface $orderManagement
* @param Filter $filter
*/
public function __construct(
Context $context,
Filter $filter,
CollectionFactory $collectionFactory,
OrderManagementInterface $orderManagement,
) {
parent::__construct($context);
$this->collectionFactory = $collectionFactory;
$this->orderManagement = $orderManagement;
$this->filter = $filter;
}
/**
* return \Magento\Framework\Data\Collection\AbstractDb
*/
protected function getCollection()
{
return $this->filter->getCollection($this->collectionFactory->create());
}
}
I prefer this solution as it gives us a cleaner separation of concerns between classes.
Conclusion
I raised this issue some time ago on Magento 2 Github https://github.com/magento/magento2/issues/4380 but it looks like it wasn’t considered a priority and I understand that.
What I think we as a community could use is refactoring roadmap where we would improve one module each month for example, discuss code architecture and so on. I know many people that would be interested in that.
Big thanks to Nikola Posa for giving his opinion on this issue
Would you like to recommend this article to your friends or colleagues?
Feel free to share.
