Urlrewrite module refactoring?

[tzyganu]

This is more of a question and a personal oppinion than an issue, but it can be an issue if you want it to be 😄
Right now the url rewrites are kind of limited to products, categories and cms pages and it makes really hard to extend the functionality to custom entities.
Let's take this example to make it clearer Magento\UrlRewrite\Controller\Adminhtml\Url\Rewrite\Edit::execute()

Notice the ugly switch-case statements

public function execute()
{
    $this->_view->loadLayout();
    $this->_setActiveMenu('Magento_UrlRewrite::urlrewrite');
    $mode = $this->_getMode();
    switch ($mode) {
        case self::PRODUCT_MODE:
            $editBlock = $this->_view->getLayout()->createBlock(
                'Magento\UrlRewrite\Block\Catalog\Product\Edit',
                '',
                [
                    'data' => [
                        'category' => $this->_getCategory(),
                        'product' => $this->_getProduct(),
                        'is_category_mode' => $this->getRequest()->has('category'),
                        'url_rewrite' => $this->_getUrlRewrite(),
                    ]
                ]
            );
            break;
        case self::CATEGORY_MODE:
            $editBlock = $this->_view->getLayout()->createBlock(
                'Magento\UrlRewrite\Block\Catalog\Category\Edit',
                '',
                [
                    'data' => ['category' => $this->_getCategory(), 'url_rewrite' => $this->_getUrlRewrite()]
                ]
            );
            break;
        case self::CMS_PAGE_MODE:
            $editBlock = $this->_view->getLayout()->createBlock(
                'Magento\UrlRewrite\Block\Cms\Page\Edit',
                '',
                [
                    'data' => ['cms_page' => $this->_getCmsPage(), 'url_rewrite' => $this->_getUrlRewrite()]
                ]
            );
            break;
        case self::ID_MODE:
        default:
            $editBlock = $this->_view->getLayout()->createBlock(
                'Magento\UrlRewrite\Block\Edit',
                '',
                ['data' => ['url_rewrite' => $this->_getUrlRewrite()]]
            );
            break;
    }
    $this->_view->getPage()->getConfig()->getTitle()->prepend($editBlock->getHeaderText());
    $this->_addContent($editBlock);
    $this->_view->renderLayout();
}

Sure I can do an around interceptor on it to plugin my own entity, but that's dangerous and I would rather avoid it.
But even if I do that then there is the _getMode() method in the same class that's protected, so... "no interceptors for you".
Notice the if-elseif chain even uglier than the switch-case statements.

protected function _getMode()
{
    if ($this->_getProduct()->getId() || $this->getRequest()->has('product')) {
        $mode = self::PRODUCT_MODE;
    } elseif ($this->_getCategory()->getId() || $this->getRequest()->has('category')) {
        $mode = self::CATEGORY_MODE;
    } else
        if ($this->_getCmsPage()->getId() || $this->getRequest()->has('cms_page')) {
        $mode = self::CMS_PAGE_MODE;
    } elseif ($this->getRequest()->has('id')) {
        $mode = self::ID_MODE;
    } else {
        $mode = $this->_objectManager->get('Magento\UrlRewrite\Block\Selector')->getDefaultMode();
    }
    return $mode;
}

This means I have to add a preference for this class so I can use mine. What If there are 2 or more custom entities that want url rewrite?

I think a cleaner way of doing it would be through di.xml. Have a general class Edit that can receive as parameters the modes and the blocks needed for each mode (and other data that might be needed). This way the catalog related url rewrites actions and blocks can be moved to CatalogUrlRewrite module, cms related actions can be moved to CmsUrlRewrite and I can create my own extension called SuperAwesome and have a second one called SuperAwesomeUrlRewrite to link my entity to url rewrites.

Care to comment on this? I would really like to hear your thoughts about the url rewrites.

[orlangur]

You're right, Magento\UrlRewrite\Controller\Adminhtml\Url\Rewrite\Edit::execute is legacy code which worked the same way when there were no CatalogUrlRewrite and CmsUrlRewrite modules, so, it's not extensible at all.

From modularity perspective:

• Catalog must not contain UrlRewrite usages
• UrlRewrite must not contain Catalog, CatalogUrlRewrite, Cms, CmsUrlRewrite usages
• CatalogUrlRewrite, CmsUrlRewrite (maybe UrlRewrite itself too?) must be removable, i.e. it's possible to remove them completely with no impact on system

Approach to controller action refactoring you described looks good to me 👍 :

[elenleonova]

Hi Marius, very good catch. Url rewrites modularity definitely makes sense, and we have added that to our internal backlog MAGETWO-38050. However, we don't plan to work on that in the nearest time, so if you would like to contribute with such change, we would highly appreciate that.

[tzyganu]

@eleonova I will try to do something, but I can't promise anything. it's kind of a big task for my current knowledge of Magento2.

[tzyganu]

I've created a PR for this: #1338.
Let me know if I need to do something else.

[vzabaznov]

Hi @tzyganu , issue was fixed, please check it, and if you think that not, feel free to write here or to open new one

[tzyganu]

@vzabaznov WooHoo. Thanks for this. I will take a look.