[Forward Port PR-14344] Fix generating product URL rewrites for anchor categories

[hostep]

(cherry picked from commit 63111ed)

Description (*)

This is a forward port of #14344 to 2.4-develop
This replaces #20826 which is stuck because the author is no longer responding for a few months.
Previous attempts at fixing this issue were done in #7667 & #22212

FOR THE MAINTAINERS: if this gets approved, can you please please please backport this to Magento 2.3, because the change is already in Magento 2.2, and by this PR also in 2.4, so it should also be part of 2.3, so we have consistent behavior over all these major Magento versions.

Related Pull Requests

• Fixes issues with url rewrites created on shop with multiple storeviews #7667
• Fix generating product URL rewrites for anchor categories #14344
• 11615: Fix UrlRewriteGenerator for anchor categories #20826
• Add store id to retrieve localized content from category repository #22212

Fixed Issues (if relevant)

1. Wrong parent category url_key in URL #4112: Wrong parent category url_key in URL
2. URL Rewrites vs multiple storeviews - a never ending battle #11615: URL Rewrites vs multiple storeviews - a never ending battle
3. URL Rewrites vs multiple storeviews - too many rewrites are being generated #11616: URL Rewrites vs multiple storeviews - too many rewrites are being generated
4. Magento 2.3 Wrong product url for anchor categories for multiple storeviews #25124: Magento 2.3 Wrong product url for anchor categories for multiple storeviews
5. Product category url rewrite missing storeview specific url_key #26393: Product category url rewrite missing storeview specific url_key
6. Probably some more ...

Manual testing scenarios (*)

1. See steps to reproduce in URL Rewrites vs multiple storeviews - a never ending battle #11615 and URL Rewrites vs multiple storeviews - too many rewrites are being generated #11616

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[rogyar]

Hi @hostep. It would be awesome to have this fix finally merged. But considering the number of talks/attempts, it will be fair to cover this fix using a functional test. Could I kindly ask you to do that?

Thank you!

[hostep]

I agree @rogyar!

Due to me not having much time free and having very little experience with writing tests, somebody else may pick this up if they want.
Maybe I'll have some more time later this week, we'll see if it happens or not from my part.

[hostep]

Thanks for the test @engcom-Echo!

But I'm confused, since it doesn't seem to be testing the generated url rewrites of products belonging to categories of at least 2 levels deep.
Am I missing something obvious here?

Also: why is the parent_id, path & level of those categories with ids 3 & 4 different on a different storeview, that seems like a mistake?

[engcom-Echo]

Hi @hostep. Thanks for your comment.

The test checks this code if we use the parameters of $storeId.

$this->categoryRepository->get($anchorCategoryId, $storeId);

These fixtures are a little scary but in my opinion, we should check the code for different use $storeId - that's exactly what I saw an important change.

[hostep]

Ok thanks for the feedback!

In my opinion, we should also test if the generated url_rewrites are correct, as that's the purpose of this PR, see #11615 for what the correct values should be in this case.

Could we add a test for that as well? Thanks!

[engcom-Echo]

I will take care of functional test coverage.

[hostep]

Thanks for the MFTF test @engcom-Echo!

I think I understand how it works. But just to be clear: this test is failing on the code as it was before this fix, right?

And shouldn't we also test the url_key_custom_store? Or is that not necessary? Something like:

<amOnPage url="{{category.url_key_custom_store}}/{{subCategory.url_key_custom_store}}/{{product.urlKey}}2.html" stepKey="goToProductPage"/>
<see selector="{{StorefrontProductInfoMainSection.productName}}" userInput="{{product.name}}" stepKey="seeProductNameInStoreFront"/>

(I'm not sure how the url is build when you should be on a different store in MFTF though)

Thanks!

[engcom-Echo]

@magento run all tests

[rogyar]

Hi @engcom-Echo. Could I ask you to rename the action group in order to follow the general naming rules, please? So we have

AssertStorefrontProductRewriteUrlSubCategoryActionGroup

in the result.

Please, check the technical guidelines (part 11.3.5).

Thank you.

[engcom-Echo]

Thank you

[rogyar]

Please, use the area prefix (Admin|Storefront) for this action group. If we have all action groups prefixed correctly, it's much easier to find and reuse an action group in the future.

Thank you!

[magento-engcom-team]

Hi @rogyar, thank you for the review.
ENGCOM-6997 has been created to process this Pull Request

[lbajsarowicz]

For what input should the result change?
I see expectation attribute, but there's not context attribute.

[lbajsarowicz]

@mszydlo Today I wanted to forward this change, then realised that @hostep is doing it already for 21 days. Awesome work! I can't wait to get this merged.

[engcom-Alfa]

✔️ QA Passed

Before:

After:

[slavvka]

@magento run Functional Tests EE

[m2-assistant]

Hi @hostep, 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.

[hostep]

Thanks @slavvka, do you know if this is going to be backported to Magento 2.3? Since #14344 was merged in 2.2, we should probably also have this fix on 2.3, no?

[hostep]

@slavvka, @chernenm, can you guys let me know if this is being backported to the 2.3.x release?

We have this awkward situation where this bug is fixed in Magento 2.2.10, 2.2.11 and 2.4.0 (I guess?), but the fix is not in 2.3.x ...

[peterjaap]

@hostep seems like at least a part of this PR is present in 2.3.6; https://github.com/magento/magento2/blob/2.3.6/app/code/Magento/CatalogUrlRewrite/Model/Product/AnchorUrlRewriteGenerator.php#L70

Edit; guess that's the only actual application code part that is involved. The rest of the files in this PR are test-related.

[hostep]

Ah nice find @peterjaap !

It was added in MC-32810: [Magento Cloud] - Incorrect URL Rewrites for non-default websites after upgrade to 2.3.4 (what a silly description, this has been a bug for many years)

However, it seems to consist of two changes and only one of them is done in 2.4-develop not the other.

Fix one: 2.3.6 vs 2.4-develop => missing in 2.4
Fix two: 2.3.6 vs 2.4-develop => both in 2.3 and 2.4

So this is again a weird thing and maybe somebody forgot to do something here.

@slavvka, @chernenm, @sdzhepa: can we figure this out please? Was that first change in MC-32810 forgotten to be included in 2.4-develop, or was this left out intentionally for some reason?