Decouple the Url rewite modules #1296

[tzyganu]

I've removed the references to CatalogUrlRewrite and CmsUrlRewrite modules from the main UrlRewrite module.
I moved every block, controller and template from the main module to the appropriate module.
I introduced a new class Magento\UrlRewrite\Model\Mode that is basically a collection of url rewrite modes. Any additional mode can be added to these modes via di.xml and each mode must implement a certain interface.
I'm almost sure that some things could have been done differently, but at least it's a place to start. Any of the CatalogUrlRewrite and CmsUrlRewrite can now be disabled without impacting the UrlRewrite module's prper functioning.
If you have other suggestions after the code review, please send them my way.

[kokoc]

@tzyganu Thank you for the contribution!!! We will review the code and get back to you once we complete the analysis.

[kokoc]

Internal reference: MAGETWO-38050

[kokoc]

@tzyganu The code and functionality looks great! However, there are few things which block pull request from being merged to the mainline:

• Failed tests - the code was moved, but tests trying to create non-existing classes. @Covers annotations links with non-existing classes, etc
• Model/Mode/CmsPage::ENTITY_TYPE and others - url rewrites functionality already has such constants(\Magento\CmsUrlRewrite\Model\CmsPageUrlRewriteGenerator::ENTITY_TYPE)
• The part of mode interface belongs to view layer(getEditBlockClass, getSortOrder) and mostly could be replaced with the native layout mechanism. See https://github.com/magento/magento2/blob/develop/app/code/Magento/Checkout/view/frontend/layout/checkout_cart_item_renderers.xml#L12
• Comes from previos point - each rewrite module (cms, catalog, etc) doesn't need separate route to be rendered.

Could you please fix these points?

[tzyganu]

@kokoc
Let's take your points step by step:

• Failed tests: I'm afraid to modify them. To put it professionally, I SUCK at unit testing. I will give it a try, but I'm afraid of the outcome
• url constants: True, but there is not 'custom' constant. I wanted to have them there for consistency. But I get your point. Is it ok if I use the existing constants for cms and catalog rewrites and leave my 'custom' constant for Mode/Custom in place?
• move methods to view layer. I understand what you mean, but I have doubts on how to approach this. So I should have a mapping for the edit block names and sort order in the layout file for the url rewrite add/edit page?
• Custom routes: I disagree (but maybe I don't have the facts straight). I have to call a controller action specific to each url rewrite module (CmsUrlRewrite to retrive the pages grid, CatalogUrlRewrite to retrieve the products grid and categories grid). What route should I use. since the controller actions are placed in their own module?

Please reply to my comment above and I will get started on the fixes.

[kokoc]

@tzyganu

• Our team will fix smaller things like styling or test coverage. So, no worries, the final code will contain your code + our fixes
• I think this point is bigger than just a constants. I support your idea to unify all entities and introduce single point of extensibility. However, "mode" interface mostly describes rendering (view) logic, but placed inside models. Let's discuss methods one by one:
  • getLabel - 👍
  • getEntityType - 👍
  • getEditBlockClass - duplicate layout logic in model namespace
  • getSortOrder - duplicate layout logic in model
  • getCategory|getProduct|getCms - should be part of appropriate blocks
  • match - needed to create appropriate block (entity type is stored in \Magento\UrlRewrite\Model\UrlRewrite). Could be replaced by layout + validation based on configuration.

I think the easiest possible ways to fix second point without mixing the system layers are: keep only first two methods in interface; add constant to some exisitng class and remove interface;

• Yes, you need to have a single block that is able to take a child based on entity type. The list of blocks should be stored in children nodes in layout. Usually we call such things renderers. Each specific sub-block will be able to show add/edit form for particular entity. Parent block could be rendered on single controller.
• The list of categories/products/cms pages definitely should not be urlRewrite module responsibility, but add/edit page URL should belong to UrlRewrite module.

[tzyganu]

@kokoc
Ok. It starts to become clear to me. I get the renderers part. I think I can do it.
I can also move the getters for the current entity (category, product, page) to the renderer blocks

But here is what I don't understand:
Where should I place the controller actions needed for retrieving/sorting/filtering the grid of products and cms pages and category tree.

Also I have some concern regarding the getSortOrder. I have a hunch it may not belong in the view layer. I mean it would work, but I think it might be an issue with the catalog rewrites. Since for a product you can generate a rewrite based on product id and on product id and category id together, the product matcher must run before the category matcher, because when generating an url based on product and category ids the keyword 'category' is present in the url. And this will be matched by the category mode. I hope I explained it clear.
Anyway, the risk is small because noone edits the admin layouts 😄
I'll try this week to implement the changes you suggested and send it back for code review.
Thank you for your assistance.

[tzyganu]

@kokoc
I tried to do the changes you suggested but hit a brick wall. I knew I was going to regret starting this task 😄
So I moved the edit blocks to the view layer as you suggested, I got rid of the sortOrder method and moved the getCategory|getProduct|getCms methods to the blocks instead of the mode model but here is the problem.
In my previous commit I changed the behavior of the Save action for the url rewrite. Instead of handling the product|category|page url rewrite directly in the action I dispatched an event so I can move the logic to an observer in the appropriate model (url_rewrite_save_handle).
The problem is that in those observers I need to access the entity involved in the url rewrite.
Now that I moved the getCategory|getProduct|getCms methods to the blocks I cannot access the entity unless I inject a block instance in the observer or if I duplicate code.
Both of the methods seam wrong.
Is it OK if I keep the getCategory|getProduct|getCms methods in the Mode classes?

Note: if at one point you want to stop wasting time with me, just because I can't manage to make it work just tell me. I won't mind.

[kokoc]

@tzyganu sorry for silence. I'll analyze the latest code and get back to you with some suggestion about "getters". We greatly appreciate your contribution!

[kokoc]

@tzyganu

Regarding getters, the part of complex logic here is required to detect the entity type for some HTTP request. However, we know the entity type from very begging. It’s stored in UrlRewrite model or transferred directly in GET request(currently it uses three different params: category, product, cms). So, I suggest to use explicit entity_type GET param for these purposes. That will significantly simplify getters logic and make them useless (ProductRepositoryInterface::getById($request->getParam('product'))).

Regarding observer, I propose you to replace it with polymorphism. Seems like the logic behind observer responsible for two main things: target_path creation and GET request validation. So, let’s create separate interface for this and call it directly from controller:

<?php
namespace Magento\UrlRewrite\Controller\Adminhtml\Url\Rewrite;

class Save extends \Magento\UrlRewrite\Controller\Adminhtml\Url\Rewrite
{
    public function execute()
    {
        // ...
        $type = $this->getRequest()->getParam('entity_type') ?: \Magento\UrlRewrite\Model\Mode\Custom::ENTITY_TYPE;


        $model->setEntityType($type)
            ->setRequestPath($requestPath)
            ->setTargetPath($this->getRequest()->getParam('target_path', $model->getTargetPath()))
            ->setRedirectType($this->getRequest()->getParam('redirect_type'))
            ->setStoreId($this->getRequest()->getParam('store_id', 0))
            ->setDescription($this->getRequest()->getParam('description'));

        $handler = $handlerFactory->getByType($type);
        $handler->prepareUrlRewrite($model, $request);
        $model->save();

        $this->messageManager->addSuccess(__('The URL Rewrite has been saved.'));
        // ....

    }
}

class ProductHandler implements UrlSaveHandlerInterface{
    function prepareUrlRewrite($urlRewrite, $request) {
        $product = ProductRepositoryInterface::getById($request->getParam('product')); //got validation inside

        $category = null;
        if ($request->getParam('category')) {
            $category = CategoryRepositoryInterface::getById($request->getParam('category')); //got validation inside
            $urlRewrite->setMetadata(serialize(['category_id' => $category->getId()]));
        }

        $targetPath = $this->productUrlPathGenerator->getCanonicalUrlPath($product, $category);

        if ($urlRewrite->getRedirectType() && !$urlRewrite->getIsAutogenerated()) {
            $data = [
                UrlRewriteService::ENTITY_ID => $urlRewrite->getEntityId(),
                UrlRewriteService::TARGET_PATH => $targetPath,
                UrlRewriteService::ENTITY_TYPE => $urlRewrite->getEntityType(),
                UrlRewriteService::STORE_ID => $urlRewrite->getStoreId(),
            ];
            $rewrite = $this->urlFinder->findOneByData($data);
            if (!$rewrite) {
                throw new LocalizedException(__('Chosen product is not associated with the chosen store or category.'));
            }
            $targetPath = $rewrite->getRequestPath();
        }
        $urlRewrite
            ->setTargetPath($targetPath)
            ->setEntityId($product->getId());

    }
}

Does something like this make sense?

[tzyganu]

@kokoc Sorry for the late response. I was away for a while. Thanks for the feedback. I will try to follow your guide, but for the moment I put this on hold because of the lack of time. I will get back to this as soon as I have some spare time.

[magento-cicd2]

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments.

[piotrekkaminski]

@kokoc hey did we ever made progress on this one?

[tzyganu]

@piotrekkaminski @kokoc I didn't. I kind of neglected this. I was hoping the core team will pick it up 😄

[tzyganu]

this issue seams to be solved by the core team. I will close this PR now.

[orlangur]

@tzyganu, how do you see it? As to me it seems like UrlRewrite still depends on Catalog which is incorrect: https://github.com/magento/magento2/blob/develop/app/code/Magento/UrlRewrite/Block/Catalog/Category/Edit.php#L29

[tzyganu]

@orlangur Damn. you are right. I didn't look very careful at it. Maybe you can open an other issue. If I reopen this one most probably it will be ignored for a while.

[orlangur]

@tzyganu thanks, just want to be sure I'm not missing something 👍

This is one of mine "personal TD" which I would like to work on :) So, I would like to prepare a PR (maybe, stealing a part of your implementation) if you don't mind.

The only tricky part here I see is BC preserving. Probably will have to leave all old classes but make them ready for removal (thus literally module dependencies will remain until classes may be simply removed in some 3.0 release).