Fix generating product URL rewrites for anchor categories

[mszydlo]

2.3-develop PR #20826

Description

During product URL rewrite generation anchor categories were loaded with no store ID specified resulting with default url key being read from the DB.

Fixed Issues

1. URL Rewrites vs multiple storeviews - a never ending battle #11615: URL Rewrites vs multiple storeviews - a never ending battle

Manual testing scenarios

Test scenario is well described in the linked issue

The same fix for 2.3-develop: #20826

[magento-cicd2]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[mzeis]

Hi @mszydlo, thank you for creating the new PR! Please can you have a look at the CLA comment?

[mzeis]

Note: CLA check is green now.

[mzeis]

Hi @mszydlo, here are my results from testing with the steps described in #11615:

If categories have been created before applying the patch and the product is created after applying the patch, everything works fine.

If categories and the product have been created before applying the patch, then:

• Running bin/magento indexer:reindex doesn't correct the entries

• Unassigning the product from the category in the "All Store Views" scope deletes the entries for store_id = 1 (= Dutch), but not for store_id = 2 (= French):

  request_path	target_path	store_id
  level-1-nl.html	catalog/category/view/id/3	1
  level-1-nl/level-2-nl.html	catalog/category/view/id/4	1
  level-1-fr.html	catalog/category/view/id/3	2
  level-1-fr/level-2-fr.html	catalog/category/view/id/4	2
  product-1.html	catalog/product/view/id/1	1
  product-1.html	catalog/product/view/id/1	2
  level-1-fr/level-2-fr/product-1.html	catalog/product/view/id/1/category/4	2
  level-1/product-1.html	catalog/product/view/id/1/category/3	2

  If I then re-assign the product to category "Level 2" and save the product, rewrites are created correctly (and a 301 redirect for level-1/product.html is created, last line):

  request_path	target_path	store_id
  level-1-nl.html	catalog/category/view/id/3	1
  level-1-nl/level-2-nl.html	catalog/category/view/id/4	1
  level-1-fr.html	catalog/category/view/id/3	2
  level-1-fr/level-2-fr.html	catalog/category/view/id/4	2
  product-1.html	catalog/product/view/id/1	1
  level-1-nl/level-2-nl/product-1.html	catalog/product/view/id/1/category/4	2
  level-1-nl/product-1.html	catalog/product/view/id/1/category/3	2
  product-1.html	catalog/product/view/id/1	2
  level-1-fr/level-2-fr/product-1.html	catalog/product/view/id/1/category/4	2
  level-1-fr/product-1.html	catalog/product/view/id/1/category/3	2
  level-1/product-1.html	level-1-fr/product-1.html	2

  So re-assigning the product to the category also wouldn't clean up the existing entries without bloating the table when you have many products/categories.

Can you confirm this?

I think that we need to solve these problems. Otherwise existing stores continue they travel down the rewrite URL entries hell. A call to bin/magento indexer:reindex should correct the entries created before applying the patch to ensure a smooth upgrade.

[hostep]

Hi @mszydlo & @mzeis: thank for working on this issue, really appreciate it!

@mzeis: I think you are confusing Magento 1 and Magento 2. In Magento 1 the url rewrites are generated by the indexer, but in Magento 2 they are not (there just isn't an easy way to regenerate them without doing funky stuff)

I can see your point though, so maybe Magento should add a command in bin/magento to regenerate them as to avoid bloated tables, there is a commercial extension available which does this, but maybe this needs to be part of Magento itself?

Btw: the new problem you are reporting also sounds a lot like #11616, but I'm not sure if it is exactly the same?

[mzeis]

@mzeis: I think you are confusing Magento 1 and Magento 2. In Magento 1 the url rewrites are generated by the indexer, but in Magento 2 they are not (there just isn't an easy way to regenerate them without doing funky stuff)

Wow yes, you are absolutely right. Sorry about that, not sure where my mind was.

Btw: the new problem you are reporting also sounds a lot like #11616, but I'm not sure if it is exactly the same?

Sounds similar but we'll have to investigate whether the cause is the same.

I'll try find out what the take of the core team is regarding URL rewriting and its problems.

[hostep]

I'll try find out what the take of the core team is regarding URL rewriting and its problems.

That would be very much appreciated, the url rewrite problems have existed since Magento 2.0.0 and have been driving me crazy ever since and I haven't seen any interest from the core Magento team in resolving them, which is insane. It's in my top 3 list of issues which should be tackled as soon as possible, but I don't know how to communicate these priorities to the core team...

[mzeis]

@hostep Here is what I found out for now:

• the core team is still investigating possibilities to solve the issue without introducting something like a new URL rewrite indexer
• a solution for this was considered but didn't end up in the core because it had a strong impact on performance

I don't know specifics but I'll keep you updated if I hear more.

[hostep]

Thanks for the feedback @mzeis!
Anyway, sorry for going off topic a bit there.

I think the PR can be processed further. It should at least fix a small portion of the problem.

[mzeis]

@hostep Thanks for your input! We're discussing how to proceed.

[mzeis]

Hi @mszydlo and @hostep, a quick update on this PR: the product team has to measure the impact of this change before we can continue processing this PR.

Considerations are:

• the fix has an impact on other URL-rewrite generator
• (unexpected) changes can have a big influence on SEO
• even if this is an unwanted behaviour, it might be used by some merchants as a feature. Because of this, the change might have to be an manageable operation rather than a regular upgrade with a fix.

I hope you understand that while the original bug is important to fix, we have to be cautious with the next steps. If somebody uses this PRs changes for their stores I would value feedback on their impact in the meanwhile.

[peterjaap]

@mzeis we're using this fix for now on one of our stores. Will report funky issues, if any, here.

[hostep]

Hi @mzeis: would you already have an update from the core team around this issue?

I agree about the cautiousness, it might be a good idea to have a new bin/magento command which can automatically fix these things, as to help them fix all at once, because fixing the code won't fix all already existing incorrect url rewrites. And that command can maybe also generate 301 redirects for url's which got fixed.

@peterjaap: have you noticed any funkyness yet by applying this fix?

[peterjaap]

@hostep not yet no.

[mzeis]

Hi @hostep, I'm on vacation currently but didn't hear about updates yet. I will try to get one for you when I am back.

[mzeis]

Hi @hostep, I contacted the team and they will look into what's the current state.

[sivaschenko]

Hi @mzeis , do you have any updates on this pull request?

[mzeis]

Hi @sivaschenko! The last time I asked CET, internal work (by the core team) was being done on this but it was said to be a complicate issue. Maybe JIRA could tell more about the current state?

[sivaschenko]

@mzeis do you have any ticket number or contacts to follow up?

[soleksii]

✔️ QA Passed

[sivaschenko]

This pull request will be processed after #20826 is merged to 2.3-develop

[VladimirZaets]

Hi @mszydlo. The branch 2.2-develop will be closed for contribution from July 15, 2019. Please finish all your Pull Requests till this date, otherwise, they will be closed.
Thanks for collaboration.

[hostep]

@VladimirZaets: you've got to be kidding right? This PR is open since the beginning of 2018 (before 2.3-develop was even a thing), you guys have been stalling and stalling and stalling this issue for whatever reason, and now you come to mention something like this, like wth? 😛

[orlangur]

@hostep so true. It would make much more sense to go over 2.3 PRs first in order to try push them forward instead of adding automated message without checking.

[m2-assistant]

Hi @mszydlo, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.