MC-38897: URL Rewrite is lost when adding a existing product to another store view #32092
Conversation
|
Hi @engcom-Golf. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
m2-community-project
bot
added
the
Priority: P2
|
@magento run all tests |
| foreach ($product->getStoreIds() as $id) { | ||
| if (!$this->isGlobalScope($id) && | ||
| !$this->storeViewService->doesEntityHaveOverriddenUrlKeyForStore( | ||
| if ((!empty($changedStoreIds) && in_array((int)$id, $changedStoreIds)) |
There was a problem hiding this comment.
I believe it could be simplified to
| if ((!empty($changedStoreIds) && in_array((int)$id, $changedStoreIds)) | |
| if (in_array((int)$id, $changedStoreIds) |
| @@ -276,4 +283,83 @@ private function isCategoryRewritesEnabled() | |||
| { | |||
| return (bool)$this->config->getValue('catalog/seo/generate_category_product_rewrites'); | |||
| } | |||
|
|
|||
| /** | |||
| * Return product changed categories store root category ids | |||
There was a problem hiding this comment.
This description is pretty confusing. Could please change to something like...
| * Return product changed categories store root category ids | |
| * Retrieve affected store ids for added/removed categories |
| $actualCategoryIds = $product->getCategoryIds(); | ||
| $result = []; | ||
| if ($oldCategoryIds === null) { | ||
| return $result; | ||
| } |
There was a problem hiding this comment.
As $product->getCategoryIds() may cost us extra request to Db it's better to end up earlier.
Also, why we skip url rewrite generation in case product didn't have categories but currently maybe does?
| $actualCategoryIds = $product->getCategoryIds(); | |
| $result = []; | |
| if ($oldCategoryIds === null) { | |
| return $result; | |
| } | |
| if ($oldCategoryIds === null) { | |
| return $result; | |
| } | |
| $actualCategoryIds = $product->getCategoryIds(); | |
| $result = []; | |
| $newCategories = array_filter( | ||
| $product->getCategoryCollection()->getItems(), | ||
| function (CategoryInterface $category) use ($product) { | ||
| return !in_array($category->getId(), $product->getOrigData('category_ids')); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Is it possible to use here collection filter instead of post-processing?
| $newCategories = array_filter( | |
| $product->getCategoryCollection()->getItems(), | |
| function (CategoryInterface $category) use ($product) { | |
| return !in_array($category->getId(), $product->getOrigData('category_ids')); | |
| } | |
| ); | |
| $newCategories = $product->getCategoryCollection()->addIdFilter($product->getOrigData('category_ids'))->getItems(); |
There was a problem hiding this comment.
it is not, as in that case, we can get either the wrong result if the collection was loaded before or break the original collection for further processing. Reload items here does not make sense.
| } | ||
|
|
||
| /** | ||
| * Return added product categories store root category ids |
There was a problem hiding this comment.
Please change comment to more clear one. See example above
| } | ||
|
|
||
| /** | ||
| * Return store root category id parsed from path |
There was a problem hiding this comment.
Looks like this comment doesn't reflect actual method behavior
| } | ||
|
|
||
| /** | ||
| * Return removed product categories store root category ids |
There was a problem hiding this comment.
Please change comment to more clear one. See example above
| $storeIds = array_reduce( | ||
| $categories, | ||
| function (array $result, Category $category) { | ||
| $result[] = $category->getStoreIds(); | ||
|
|
||
| return $result; | ||
| }, | ||
| [] | ||
| ); | ||
| return array_merge([], ...$storeIds); |
There was a problem hiding this comment.
Probably could be simplified to
| $storeIds = array_reduce( | |
| $categories, | |
| function (array $result, Category $category) { | |
| $result[] = $category->getStoreIds(); | |
| return $result; | |
| }, | |
| [] | |
| ); | |
| return array_merge([], ...$storeIds); | |
| $result = [[]]; | |
| foreach ($categories as $category) { | |
| $result[] = $category->getStoreIds(); | |
| } | |
| $result = array_merge(...$result); | |
| return array_unique($result); |
| * @magentoDataFixture Magento/CatalogUrlRewrite/_files/product_with_category.php | ||
| * @SuppressWarnings(PHPMD.ExcessiveMethodLength) | ||
| */ | ||
| public function testChangeCategoryGlobalScopeWithStoreOverriddenUrlKey() |
There was a problem hiding this comment.
Is it possible to simplify this test using dataProvider?
There was a problem hiding this comment.
I'm not sure if it is a good idea as here we just have one more save and assertation, but if I explode this test to different variations it will work twice longer and wouldn't be simpler than this variant.
m2-community-project
bot
added
Progress: needs update
Progress: review
and removed
Progress: pending review
Progress: needs update
labels
engcom-Golf
force-pushed
the
MC-38897
branch
2 times, most recently
from
74b55b5 to
b1f5552
Compare
|
@magento run all tests |
engcom-Golf
force-pushed
the
MC-38897
branch
from
b1f5552 to
93e464e
Compare
engcom-Golf
force-pushed
the
MC-38897
branch
from
93e464e to
78da8a1
Compare
|
@magento run all tests |
|
@magento run all tests |
|
Closing this for now as it seems abandoned for a long time. Please feel free to reopen if needed. |
Description (*)
The problem was in the process of URL rewrites generation if product is saved in global scope
This PR fixes issue by forcing generation URL rewrites for stores with scope overridden URL key in case if category for store was changed and category url_key is used for URL rewrite for products
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Expected result
Url rewrite is generated for categor/product if category is assigned to product and removed if category is removed from product.
Questions or comments
Contribution checklist (*)