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 :)

[ghost]

@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.

[hostep]

I just accidentally stumbled upon a commit in 2.3-develop which might fix this issue (not tested yet): 1f5d415

This also seems to be available already in 2.2-develop, but not included in any official release yet (at the of time of writing that is).

[psavary]

Hi @hostep. Thank you for taking the time to work this issue!
I've tested 1f5d415
This doesn't work as intended, only new, non existing catalog categories for store views will be generated, the existing ones aren't updated.
This might be an issue (from what I gather) with this line The data has not changed for the url_key and therefore will not be regenerated.

[nwtben]

Is this still an issue in magento 2.4.x ?

[hostep]

@nwtben: I wonder about that as well since I saw a few changes flying by in some pull request regarding the url rewrite generation with storeview specific data, so maybe it got fixed, but maybe also not ...

Feel free to test and report back! 🙂

[hostep]

I can very easily break the frontend again. Here are some extra steps to perform after the steps from my opening post.

16. Edit the 'All 2' category on the English & French storeview, change the url-key temporarily to be able to uncheck the 'Create Permanent Redirect for old URL' checkbox, then check 'Use Default Value' so it uses all-2 again as url_key, and save the category.
17. Reindex & flush caches
18. Go to homepage on frontend in English & French and click the 'All 5' category, now check the url

Expected

Category name	Url in English	Url in French
All 2	/en/all-2.html	/fr/all-2.html
All 5	/en/all-2/all-5.html	/fr/all-2/all-5.html

Actual:

Category name	Url in English	Url in French
All 2	/en/all-2.html	/fr/all-2.html
All 5	/en/en-2/all-5.html	/fr/fr-2/all-5.html

$ bin/magento catalog:category:integrity:urlpath
+-------------+-------+----------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Category ID | Name  | Store ID | Problem                                                                                                                                                          |
+-------------+-------+----------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+
...
| 7           | All 5 | 1        | Category has an incorrect url_path value "en-2/all-5". It should be "all-2/all-5". Unfortunately this is not easily fixable using the backend of Magento.        |
| 7           | All 5 | 2        | Category has an incorrect url_path value "fr-2/all-5". It should be "all-2/all-5". Unfortunately this is not easily fixable using the backend of Magento.        |

...
+-------------+-------+----------+------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Discussion

For the developers who want to work on this issue, note that the bug starts happening between step 9 and 12, once you start dragging categories around, the url_path is not re-calculated correctly on storeview scope 0.

In my opinion this should become a higher priority, shopowners using multiple storeviews and translated url_key's for categories still can't drag categories around in the hierarchy without breaking their url structure. Which is really bad for SEO. We have to tell our clients to delete a category, re-create it, re-add all children categories and re-assign all products, if they want to position it in another position, which is ridiculous. (/cc @sidolov)

@engcom-Delta: can you re-open please? Also I have no powers to re-open my own tickets, so please don't suggest that I can, because we have very limited rights unfortunatly 🙁

[sidolov]

@hostep thank you for the clarification! I have raised the priority of the picket to P1 since the flow to manage the categories you mentioned is awful.

[magento-engcom-team]

✅ Confirmed by @sdzhepa
Thank you for verifying the issue. Based on the provided information internal tickets MC-40780 were created

Issue Available: @sdzhepa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

[hostep]

I accidentally bumped into a commit which might fix this: MC-40371: Moving category in hierarchy causes url_path to be incorrect

I'll try to test later when I find more energy.

[viktorpetryk]

@magento I am working on this

[dmitriyprime]

✔️ QA passed

After fix:
**mysql> select * from ece_catalog_category_entity_varchar;
+----------+--------------+----------+-------------------+--------+
| value_id | attribute_id | store_id | value | row_id |
+----------+--------------+----------+-------------------+--------+
| 1 | 45 | 0 | Root Catalog | 1 |
| 2 | 52 | 0 | PRODUCTS | 2 |
| 3 | 45 | 0 | Default Category | 2 |
| 4 | 52 | 0 | PRODUCTS | 3 |
| 5 | 45 | 0 | All 1 | 3 |
| 6 | 119 | 0 | all-1 | 3 |
| 7 | 120 | 0 | all-1 | 3 |
| 8 | 52 | 0 | PRODUCTS | 4 |
| 9 | 45 | 0 | All 2 | 4 |
| 10 | 119 | 0 | all-2 | 4 |
| 11 | 120 | 0 | all-2 | 4 |
| 12 | 52 | 0 | PRODUCTS | 5 |
| 13 | 45 | 0 | All 3 | 5 |
| 14 | 119 | 0 | all-3 | 5 |
| 15 | 120 | 0 | all-3 | 5 |
| 16 | 52 | 0 | PRODUCTS | 6 |
| 17 | 45 | 0 | All 4 | 6 |
| 18 | 119 | 0 | all-4 | 6 |
| 19 | 120 | 0 | all-3/all-4 | 6 |
| 20 | 120 | 2 | en-1 | 3 |
| 21 | 120 | 2 | en-2 | 4 |
| 22 | 45 | 2 | EN 1 | 3 |
| 23 | 119 | 2 | en-1 | 3 |
| 24 | 120 | 3 | fr-1 | 3 |
| 25 | 120 | 3 | fr-2 | 4 |
| 26 | 45 | 3 | FR 1 | 3 |
| 27 | 119 | 3 | fr-1 | 3 |
| 29 | 45 | 2 | EN 2 | 4 |
| 30 | 119 | 2 | en-2 | 4 |
| 32 | 45 | 3 | FR 2 | 4 |
| 33 | 119 | 3 | fr-2 | 4 |
| 34 | 120 | 2 | en-3 | 5 |
| 35 | 45 | 2 | EN 3 | 5 |
| 36 | 119 | 2 | en-3 | 5 |
| 37 | 120 | 3 | fr-3 | 5 |
| 38 | 45 | 3 | FR 3 | 5 |
| 39 | 119 | 3 | fr-3 | 5 |
| 40 | 120 | 2 | en-3/en-4 | 6 |
| 41 | 45 | 2 | EN 4 | 6 |
| 42 | 119 | 2 | en-4 | 6 |
| 43 | 120 | 3 | fr-3/fr-4 | 6 |
| 44 | 45 | 3 | FR 4 | 6 |
| 45 | 119 | 3 | fr-4 | 6 |
| 47 | 120 | 1 | all-2 | 4 |
| 51 | 120 | 1 | all-3/all-4 | 6 |
| 54 | 52 | 0 | PRODUCTS | 7 |
| 55 | 45 | 0 | All 5 | 7 |
| 56 | 119 | 0 | all-5 | 7 |
| 57 | 120 | 0 | all-2/all-5 | 7 |
| 58 | 120 | 1 | all-2/all-5 | 7 |
| 59 | 120 | 2 | en-2/all-5 | 7 |
| 60 | 120 | 3 | fr-2/all-5 | 7 |
| 61 | 52 | 0 | PRODUCTS | 8 |
| 62 | 45 | 0 | All 6 | 8 |
| 63 | 119 | 0 | all-6 | 8 |
| 64 | 120 | 0 | all-3/all-4/all-6 | 8 |
| 65 | 120 | 1 | all-3/all-4/all-6 | 8 |
| 66 | 120 | 2 | en-3/en-4/all-6 | 8 |
| 67 | 120 | 3 | fr-3/fr-4/all-6 | 8 |
+----------+--------------+----------+-------------------+--------+
59 rows in set (0.01 sec)

[hostep]

@dmitriyprime: which fix? MC-40371 or #32598 (MC-40780) ? If the latter, please comment on that pull request instead of here, thanks! 🙂
Also please format your output by putting it in a code block (starts with 3 backticks and a newline and ends with a newline and 3 more backticks)

[magento-engcom-team]

Hi @hostep, @victorpetryk.

Thank you for your report and collaboration!

The related internal Jira ticket MC-40780 was closed as Fixed.

The fix will be available with the upcoming 2.4.3 release.

[hostep]

For the people interested, MC-40371 fixed part of the problem, but it started breaking again after step 16. #32598 then solved the remainder, and now the bug no longer occurs as far as I can see. All category url_path's (and url rewrites having to do with these) are being generated correctly if you start dragging categories around in the hierarchy.

If people are interested, here is a zip file with two patches that can be applied with https://github.com/cweagans/composer-patches or https://github.com/vaimo/composer-patches/ (use patch level 1 please): issue-16202-patches.zip

These patches are tested on Magento 2.4.2 but could in theory also be applied to Magento 2.4.0 or 2.4.1. I haven't looked into backporting these all the way to 2.3.6 though (and maybe never will ...)

Big thanks to @victorpetryk and @nikita-shcherbatykh and possibly some others involved here for finally fixing this huge bug that has existed since Magento 2 was released! 🥳