New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sales module MassAction refactoring #9197

Closed
wants to merge 3 commits into
base: develop
from

Conversation

@ldusan84
Collaborator

ldusan84 commented Apr 10, 2017

I think that the MassAction feature in the Sales module is not correctly architectured as it contains hidden dependencies.

Description

If you try to create your own mass action in the sales grid you need to extend the base MassAction class. If you do that you will end up with a fatal error when you use it in admin panel:

<?php
namespace ExampleVendor\ExampleModule\Controller\Adminhtml;

class Example extends \Magento\Sales\Controller\Adminhtml\Order\AbstractMassAction
{

}

"Fatal error: Call to a member function create() on a non-object"

The problem is this:

abstract class AbstractMassAction extends \Magento\Backend\App\Action
{
    /**
    ***
     */
    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);
        }
    }
}

So the collectionFactory should be injected into constructor but you realize that only after you examine the entire class. We shouldn't do that as we have dependency injection in order to make dependencies visible.

You can find out more about this on my blog post http://dusanlukic.com/magento-2-hidden-dependency-refactoring

I moved the dependency into abstract method making it mandatory for child classes to provide the collection.

This feature needs more refactoring, this would be the first step.

No tests needed as public api remains unchanged.

/**
* @param Context $context
* @param Filter $filter
* @param CollectionFactory $collectionFactory
* @param OrderManagementInterface $orderManagement

This comment has been minimized.

@avoelkl

avoelkl Apr 17, 2017

Contributor

It seems the only subclass from \AbstractMassAction using the OrderManagmentInterface is \MassHold where it is injected via constructor. I don't see a need to declare it as param here.

This comment has been minimized.

@ldusan84

ldusan84 Apr 18, 2017

Collaborator

@avoelkl exactly, thanks, will remove those two.

@avoelkl avoelkl self-assigned this Apr 17, 2017

@avoelkl avoelkl added the improvement label Apr 17, 2017

@avoelkl avoelkl added this to the April 2017 milestone Apr 17, 2017

@avoelkl avoelkl moved this from TODO to Review In Progress in Community Pull Requests Apr 17, 2017

@avoelkl

This comment has been minimized.

Contributor

avoelkl commented Apr 17, 2017

Hi Dusan!
Thanks for the PR! As mentioned earlier I really like the blogpost you've created earlier. Seeing a PR resulting from it is even bette :)
Actually I would prefer moving DI into the constructor but given the current state the abstract method+child classes are good to go i think.
I will clarify this with the Magento team to get their opinion on this, though.

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Apr 18, 2017

Hi @avoelkl thank you for the review.

Yes, I agree, this is only one step and some refactoring after this should follow.

Looking forward to hearing Magento team's opinion.

@avoelkl

This comment has been minimized.

Contributor

avoelkl commented Apr 24, 2017

Hi @ldusan84,
Some update on this: There are some errors in the tests for EE as there are dependencies on the refactored classes. Do you have an EE installation ready to verify this?
Otherwise we can have the Magento team looking at this and resolving it.

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Apr 24, 2017

Hi @avoelkl

Thanks for looking into this. I'll check what's happening on EE and let you know, no worries.

@magento-team magento-team moved this from Review In Progress to TODO in Community Pull Requests Apr 26, 2017

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented Apr 30, 2017

Hi @avoelkl

I have fixed the issues in EE, can you please tell me what would be the best way to submit those changes?

@avoelkl

This comment has been minimized.

Contributor

avoelkl commented May 2, 2017

Hi @ldusan84,
You can add those commits to this PR and then we can run the acceptance tests on the latest updates here.

@ldusan84

This comment has been minimized.

Collaborator

ldusan84 commented May 2, 2017

Hi @avoelkl

The thing is that those files that should be changed are EE files that do not exist in the community edition. I don't think that I can commit those here?

@avoelkl

This comment has been minimized.

Contributor

avoelkl commented May 3, 2017

Ah sorry, you're right. My fault.
Let me check :)

@avoelkl

This comment has been minimized.

Contributor

avoelkl commented May 3, 2017

We'll give you access to the EE repo, I'll contact you with further details :)

@okorshenko okorshenko modified the milestones: April 2017, May 2017 May 9, 2017

@magento-team magento-team moved this from TODO to Merging In Progress in Community Pull Requests May 26, 2017

@magento-team magento-team moved this from Merging In Progress to Response Needed in Community Pull Requests May 29, 2017

@okorshenko okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

@okorshenko okorshenko self-assigned this Jun 21, 2017

@magento-team magento-team moved this from Response Needed to Review In Progress in Community Pull Requests Jun 21, 2017

@okorshenko

This comment has been minimized.

Contributor

okorshenko commented Jun 21, 2017

Hello @ldusan84
Unfortunately we can not accept this pull request since it introduces BiC. We agree that having a hidden dependency in abstract controller is an issue. We will deprecate this controller in 2.2. There is no big value from this controller.

Thank you for your everyone collaboration!

@okorshenko okorshenko closed this Jun 21, 2017

@magento-team magento-team moved this from Review In Progress to Done in Community Pull Requests Jun 21, 2017

magento-team pushed a commit that referenced this pull request Jun 22, 2017

Oleksii Korshenko
MAGETWO-67516: Sales module MassAction refactoring #9197
 - deprecated Sales AbstractController
 - added comments in Abstract model to deprecated methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment