Moving category in hierarchy causes url_path to be incorrect when using different url keys in multiple storeviews

[hostep]

Hi!

This is yet another issue in the seemingly endless catalog of Magento2 url bugs with categories and products when using multiple storeviews.

Preconditions

1. Tested with Magento 2.2.4 & Magento 2.3-develop branch (commit: 2910d8b), and most likely also all Magento 2.1.x and 2.0.x releases are affected (untested)
2. PHP 7.1.18
3. Updates on Dec 23, 2020: Issue still actual for 2.4.x Moving category in hierarchy causes url_path to be incorrect when using different url keys in multiple storeviews #16202 (comment) Moving category in hierarchy causes url_path to be incorrect when using different url keys in multiple storeviews #16202 (comment)

Steps to reproduce

1. Setup a clean Magento installation, I'm using commit 2910d8b here
2. Make sure you have 2 storeviews: English (code: en) & French (code: fr)
3. Set Stores > Configuration > General > Web > Url Options > Add Store Code to Urls to Yes
4. Flush caches
5. Add new Categories, in this exact structure:

  Default Category
├── All 1
│   └── All 2
├── All 3
└── All 4

6. Start translating these category names & url keys, as following (uncheck 'Create permanent redirect for old url' every single time), I've also added the category id from my case, as it might be useful to understand the expected & actual results:

Store view	Category name	Category url key	Category ID
All Store Views	All 1	all-1	3
English	EN 1	en-1	3
French	FR 1	fr-1	3
All Store Views	All 2	all-2	4
English	EN 2	en-2	4
French	FR 2	fr-2	4
All Store Views	All 3	all-3	5
English	EN 3	en-3	5
French	FR 3	fr-3	5
All Store Views	All 4	all-4	6
English	EN 4	en-4	6
French	FR 4	fr-4	6

7. So far, so good, all values in the catalog_category_entity_varchar for the attributes url_key and url_path are correct. And all entries in the url_rewrite table are good.
   Feel free to review them at this point.
8. Switch back to 'All Store Views' in the Categories listing if you aren't already on that store view
9. Drag category 'All 2' out of 'All 1', and make its new parent 'Default Category'
10. Drag category 'All 4' inside of 'All 3', so that becomes its new parent.
11. You now have this category structure:

  Default Category
├── All 1
├── All 3
│   └── All 4
├── All 2

12. At this point the values in the catalog_category_entity_varchar for the attribute url_path are already incorrect on store view level, but the problem doesn't manifest itself yet on the frontend, since the url rewrites are still ok somehow. So let's start to break something on the frontend now.
13. Add a new subcategory 'All 5' under the category 'All 2' (no need to create translations for this one), ID: 7, see below
14. Add a new subcategory 'All 6' under the category 'All 4' (no need to create translations for this one), ID: 8, see below
15. You now have this category structure:

  Default Category
├── All 1
├── All 3
│   └── All 4
│   │   └── All 6
├── All 2
│   └── All 5

Expected result

Contents of the catalog_category_entity_varchar table for the url_path attribute (I took the liberty to "translate" the attribute_id and store_id to make the table easier to understand):

attribute	store_code	entity_id	value
url_path	all	3	all-1
url_path	all	4	all-2
url_path	all	5	all-3
url_path	all	6	all-3/all-4
url_path	all	7	all-2/all-5
url_path	all	8	all-3/all-4/all-6
url_path	en	3	en-1
url_path	en	4	en-2
url_path	en	5	en-3
url_path	en	6	en-3/en-4
url_path	en	7	en-2/all-5
url_path	en	8	en-3/en-4/all-6
url_path	fr	3	fr-1
url_path	fr	4	fr-2
url_path	fr	5	fr-3
url_path	fr	6	fr-3/fr-4
url_path	fr	7	fr-2/all-5
url_path	fr	8	fr-3/fr-4/all-6

When you look at the frontend of the shop and click on the following categories in the menu and look at the url, you'll see:

Category name	Category url
EN 1	/en/en-1.html
EN 2	/en/en-2.html
EN 3	/en/en-3.html
EN 4	/en/en-3/en-4.html
EN 5	/en/en-2/all-5.html
EN 6	/en/en-3/en-4/all-6.html

(exact same for French storeview)

Actual result

Contents of the catalog_category_entity_varchar table for the url_path attribute:

attribute	store_code	entity_id	value
url_path	all	3	all-1
url_path	all	4	all-2
url_path	all	5	all-3
url_path	all	6	all-3/all-4
url_path	all	7	all-2/all-5
url_path	all	8	all-3/all-4/all-6
url_path	en	3	en-1
url_path	en	4	en-1/en-2
url_path	en	5	en-3
url_path	en	6	en-4
url_path	en	7	en-1/en-2/all-5
url_path	en	8	en-4/all-6
url_path	fr	3	fr-1
url_path	fr	4	fr-1/fr-2
url_path	fr	5	fr-3
url_path	fr	6	fr-4
url_path	fr	7	fr-1/fr-2/all-5
url_path	fr	8	fr-4/all-6

When you look at the frontend of the shop and click on the following categories in the menu and look at the url, you'll see:

Category name	Category url
EN 1	/en/en-1.html
EN 2	/en/en-2.html
EN 3	/en/en-3.html
EN 4	/en/all-3/en-4.html
EN 5	/en/en-1/en-2/all-5.html
EN 6	/en/en-4/all-6.html

(exact same for French storeview)

Further discussion

I think this is because in the methods afterChangeParent and updateUrlPathForChildren of the class Magento\CatalogUrlRewrite\Model\Category\Plugin\Category\Move should loop over all store id's and recalculate the url_path for every store view separately.

Now, I don't think that fixing the code alone will be enough. You have to keep in mind that people have been running into this bug (and a lot of other url bugs) in the past 2 to 3 years. So it won't be enough to just fix the code.

I believe Magento should get some new command line commands for bin/magento which looks at all category's url_path attributes, and fix those if incorrect, and then also (maybe in a separate command) re-generate url rewrites (with an optional flag for creating 301 redirects, for shops already live vs shops in development).
You could argue to write upgrade scripts which do this automatically when updating to a new version of Magento, but I don't think that would be a good idea, I think store owners need to execute these things manually, it might have SEO consequences if it happens automatically and store owners might not expect that to happen.

@akaplya: since you are responsible for the UrlRewrites components according to the wiki, I'm including you in here.
It might also be nice to get some kind of update from you regarding how it's going with fixing all those other store view related bugs with url rewrites for products and categories which still haven't been properly fixed (at least not in 2.2.4). If you don't know what I'm talking about, let me know, I can send you a whole list of issues.

Thanks!

[hostep]

This is just a proof of concept (it's really dirty code I think), but applying this patch resolves the issues with the catalog_category_entity_varchar table, but it leaves one incorrect url rewrite case (which might not be directly related to this issue):

Expected:

Category name	Category url
EN 1	/en/en-1.html
EN 2	/en/en-2.html
EN 3	/en/en-3.html
EN 4	/en/en-3/en-4.html
EN 5	/en/en-2/all-5.html
EN 6	/en/en-3/en-4/all-6.html

Actual:

Category name	Category url
EN 1	/en/en-1.html
EN 2	/en/en-2.html
EN 3	/en/en-3.html
EN 4	/en/all-3/en-4.html
EN 5	/en/en-2/all-5.html
EN 6	/en/en-3/en-4/all-6.html

Patch:

diff --git a/app/code/Magento/CatalogUrlRewrite/Model/Category/Plugin/Category/Move.php b/app/code/Magento/CatalogUrlRewrite/Model/Category/Plugin/Category/Move.php
index 17d12ba563e..e93628aafa3 100644
--- a/app/code/Magento/CatalogUrlRewrite/Model/Category/Plugin/Category/Move.php
+++ b/app/code/Magento/CatalogUrlRewrite/Model/Category/Plugin/Category/Move.php
@@ -8,6 +8,7 @@ namespace Magento\CatalogUrlRewrite\Model\Category\Plugin\Category;
 use Magento\Catalog\Model\Category;
 use Magento\CatalogUrlRewrite\Model\CategoryUrlPathGenerator;
 use Magento\CatalogUrlRewrite\Model\Category\ChildrenCategoriesProvider;
+use Magento\CatalogUrlRewrite\Service\V1\StoreViewService;
 
 class Move
 {
@@ -21,16 +22,23 @@ class Move
      */
     private $childrenCategoriesProvider;
 
+    /**
+     * @var StoreViewService
+     */
+    private $storeViewService;
+
     /**
      * @param CategoryUrlPathGenerator $categoryUrlPathGenerator
      * @param ChildrenCategoriesProvider $childrenCategoriesProvider
      */
     public function __construct(
         CategoryUrlPathGenerator $categoryUrlPathGenerator,
-        ChildrenCategoriesProvider $childrenCategoriesProvider
+        ChildrenCategoriesProvider $childrenCategoriesProvider,
+        StoreViewService $storeViewService
     ) {
         $this->categoryUrlPathGenerator = $categoryUrlPathGenerator;
         $this->childrenCategoriesProvider = $childrenCategoriesProvider;
+        $this->storeViewService = $storeViewService;
     }
 
     /**
@@ -53,6 +61,24 @@ class Move
     ) {
         $category->setUrlPath($this->categoryUrlPathGenerator->getUrlPath($category));
         $category->getResource()->saveAttribute($category, 'url_path');
+
+        $clonedCategory = clone $category;
+
+        foreach ($clonedCategory->getStoreIds() as $storeId) {
+            if ($this->storeViewService->doesEntityHaveOverriddenUrlPathForStore(
+                $storeId,
+                $clonedCategory->getId(),
+                Category::ENTITY
+            )) {
+                $clonedCategory->setStoreId($storeId);
+                $clonedCategory->setUrlKey($clonedCategory->getResource()->getAttributeRawValue($clonedCategory->getId(), 'url_key', $storeId));
+
+                $clonedCategory->unsUrlPath();
+                $clonedCategory->setUrlPath($this->categoryUrlPathGenerator->getUrlPath($clonedCategory));
+                $clonedCategory->getResource()->saveAttribute($clonedCategory, 'url_path');
+            }
+        }
+
         $this->updateUrlPathForChildren($category);
 
         return $result;
@@ -68,6 +94,21 @@ class Move
             $childCategory->unsUrlPath();
             $childCategory->setUrlPath($this->categoryUrlPathGenerator->getUrlPath($childCategory));
             $childCategory->getResource()->saveAttribute($childCategory, 'url_path');
+
+            foreach ($childCategory->getStoreIds() as $storeId) {
+                if ($this->storeViewService->doesEntityHaveOverriddenUrlPathForStore(
+                    $storeId,
+                    $childCategory->getId(),
+                    Category::ENTITY
+                )) {
+                    $childCategory->setStoreId($storeId);
+                    $childCategory->setUrlKey($childCategory->getResource()->getAttributeRawValue($childCategory->getId(), 'url_key', $storeId));
+
+                    $childCategory->unsUrlPath();
+                    $childCategory->setUrlPath($this->categoryUrlPathGenerator->getUrlPath($childCategory));
+                    $childCategory->getResource()->saveAttribute($childCategory, 'url_path');
+                }
+            }
         }
     }
 }

[Tjitse-E]

We've had the same problem on one of our multistores. We solved it with iazel/regenurl, the module contains two console commands that help you fix it. More info about the fix here in the readme.

[hostep]

@Tjitse-E: quickly looking through the code, I can't see it fixing values in the catalog_category_entity_varchar table (url_path attribute), it looks like it only generates correct url rewrites, but doesn't fix incorrect category data in the database. The end result is probably correct, but I'd like the data in the database to be correct as well :)

@hostep, thank you for your report.
We've acknowledged the issue and added to our backlog.

[Tjitse-E]

@hostep you're right, the script doesn't fix the bad entities in the catalog_category_entity_varchar, but hopefully your fix can help with this.