MC-38931: Product URL Rewrites are not removed when product removed from website #31106

engcom-Golf · 2020-12-01T07:50:40Z

Description (*)

Fixes URL rewrite delete/generation on website remove/assign

Related Pull Requests

Fixed Issues (if relevant)

Fixes Product URL Rewrites are not removed when product removed from website #24184

Manual testing scenarios (*)

test URL rewrite generation/delete from admin
test URL rewrite generation/delete by REST API requests (productRepository POST/PUT/DELETE)
test URL rewrite generation/delete by mass update (corn should be enabled)
test URL rewrite generation/delete through import

Questions or comments

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 · 2020-12-01T07:50:43Z

Hi @engcom-Golf. 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

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 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

engcom-Golf · 2020-12-01T09:16:36Z

@magento run all tests

engcom-Kilo · 2020-12-02T08:35:30Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

+                    array_diff($product->getWebsiteIds(), $oldWebsiteIds)
+                );
+            }
+            $this->appendRewrites->execute([$product], $storesToAdd);


$this->appendRewrites->execute() may throw UrlAlreadyExistsException. Please make sure it will be processed or process it here.

Don't need to handle this exception. This is expected behavior in case when admin tries to save a product with url-key that already assigned to another item either product or category. See \Magento\CatalogUrlRewrite\Model\ProductUrlRewriteTest::testUrlRewriteOnProductSaveWithExistingUrlKey

In this case please add exception to annotaions.

engcom-Kilo · 2020-12-02T08:37:01Z

app/code/Magento/CatalogUrlRewrite/Model/Product/AppendUrlRewritesToProducts.php

+            }
+        }
+        if (!empty($urls)) {
+            $this->urlPersist->replace(array_merge(...$urls));


$this->urlPersist->replace() may throw UrlAlreadyExistsException. Please add it to annotations.

In this case, if an exception is thrown in a sub-routine, then @throws must not be used in the parent method.
https://devdocs.magento.com/guides/v2.4/coding-standards/docblock-standard-general.html

engcom-Kilo · 2020-12-02T08:37:33Z

app/code/Magento/CatalogUrlRewrite/Model/Product/AppendUrlRewritesToProducts.php

+    private $dataLocator;
+
+    /**
+     * AppendUrlRewritesToProducts constructor.


Please remove.

engcom-Kilo · 2020-12-02T08:38:51Z

app/code/Magento/CatalogUrlRewrite/Model/Product/AppendUrlRewritesToProducts.php

+use Magento\UrlRewrite\Model\UrlPersistInterface;
+
+/**
+ * Update url rewrites to products class


It's a bit confusing description and class name. Could you please add more clear description and/or class name?

engcom-Kilo · 2020-12-02T09:14:07Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

            }
        }
+
+        return array_merge([], ...$storeIdsArray);


Please add result to local cache. This method already used twice during observer execution.
Also it would be better to use $product instead on $websiteIds as parameter to avoid code duplication.
Lines
$oldWebsiteIds = $product->getOrigData('website_ids') ?? []
$websiteIds = array_diff($oldWebsiteIds, $product->getWebsiteIds())
could be moved in this method. Rename method to more appropriate one after this changes.

there is no code duplication, as we use the different order of sub-arrays to compare and getting result of website ids. A cache can be added but in the current implementation, it won't give any result of performance improvement. Don't see a possibility to add and delete the same website at the same time.

engcom-Kilo · 2020-12-02T14:14:22Z

app/code/Magento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/ProductToWebsiteChange.php

+/**
+ * Update URL rewrites after website change
+ */
+class ProductToWebsiteChange


Plugin name is confusing. Please change it to url rewrite related.

engcom-Kilo · 2020-12-02T14:23:12Z

dev/tests/integration/testsuite/Magento/UrlRewrite/Model/StoreSwitcher/RewriteUrlTest.php

     * @magentoDataFixture Magento/Catalog/_files/category_product.php
+     * @magentoDataFixture Magento/UrlRewrite/_files/url_rewrite.php


Looks like this file could be reverted.

In this case, changing the order of fixture loading have sense, as product save generates rewrites for all assigned websites. Then can not be checked logic of redirect from unexisting rewrite to home page.

engcom-Kilo · 2020-12-02T14:25:07Z

...tsuite/Magento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/ProductToWebsiteChangeTest.php

+     */
+    protected function setUp(): void
+    {
+        parent::setUp();


No need to call parent construct

engcom-Kilo · 2020-12-02T14:26:52Z

...tsuite/Magento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/ProductToWebsiteChangeTest.php

+    private function loadWebsiteByCode($websiteCode)
+    {
+        $websiteRepository = Bootstrap::getObjectManager()->get(WebsiteRepository::class);
+        try {


No need to wrap it in try-catch.

engcom-Kilo · 2020-12-02T14:27:06Z

...tsuite/Magento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/ProductToWebsiteChangeTest.php

+     */
+    private function getProductModel(string $sku, int $storeId = null): ProductInterface
+    {
+        try {


No need to wrap it in try-catch

engcom-Golf · 2020-12-03T13:15:03Z

@magento run all tests

engcom-Kilo · 2020-12-03T13:26:18Z

...gento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/UpdateProductWebsiteUrlRewritesTest.php

+     * @param $websiteCode
+     * @return Website
+     */
+    private function loadWebsiteByCode($websiteCode)


No need to extract couple code lines into separate method.

engcom-Kilo · 2020-12-03T13:26:22Z

...gento/CatalogUrlRewrite/Plugin/Catalog/Model/Product/UpdateProductWebsiteUrlRewritesTest.php

+     * @param int|null $storeId
+     * @return ProductInterface
+     */
+    private function getProductModel(string $sku, int $storeId = null): ProductInterface


No need to extract couple code lines into separate method.

engcom-Kilo · 2020-12-03T13:29:26Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

+                    array_diff($product->getWebsiteIds(), $oldWebsiteIds)
+                );
+            }
+            $this->appendRewrites->execute([$product], $storesToAdd);


In this case please add exception to annotaions.

engcom-Golf · 2020-12-03T14:27:36Z

@magento run all tests

zakdma · 2020-12-03T14:47:06Z

@magento import code to https://github.com/magento-tsg/magento2ce

engcom-Golf · 2020-12-03T14:48:30Z

@magento run all tests

magento-engcom-team · 2020-12-03T14:50:15Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-31106.

engcom-Golf · 2020-12-07T08:13:18Z

@magento run all tests

zakdma · 2020-12-07T10:35:45Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-12-07T10:38:55Z

@zakdma, an error occurred during the Pull Request import.

zakdma · 2020-12-07T10:41:37Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-12-07T10:44:08Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-31106.

engcom-Golf · 2020-12-07T11:34:05Z

@magento run all tests

engcom-Golf · 2020-12-07T16:09:00Z

@magento run all tests

zakdma · 2020-12-08T09:46:55Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

     * @return void
-     * @throws \Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException
+     * @throws UrlAlreadyExistsException


You should not add Exception to annotation if your method does not throw exception directly

required by https://devdocs.magento.com/guides/v2.3/coding-standards/technical-guidelines.html#5-exceptions in 5.3

@engcom-Golf please refer to docblock magento standards
https://devdocs.magento.com/guides/v2.3/coding-standards/docblock-standard-general.html#functions-methods
A declaration of possible exceptions using the @throws tag, if the actual body of function triggers an exception.

@engcom-Golf please refer to docblock magento standards
https://devdocs.magento.com/guides/v2.3/coding-standards/docblock-standard-general.html#functions-methods
A declaration of possible exceptions using the @throws tag, IF THE ACTUAL BODY OF FUNCTION TRIGGERS AN EXCEPTION.

Some kind of collision, but I'd like to not hide the possible exception throwing. This was required by @engcom-Kilo review as well.
#31106 (comment)

zakdma · 2020-12-08T09:48:47Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

+    {
+        $oldWebsiteIds = $product->getOrigData('website_ids');
+
+        return array_values($oldWebsiteIds) != array_values($product->getWebsiteIds());


This array comparison is unreliable. Better use array_diff instead

array_diff returns empty result in case if new websites added

No. Check this
http://sandbox.onlinephpfunctions.com/code/5f1d5a8f31414027b9f5962569e9a6d7d54f9b3b

and what about this case
http://sandbox.onlinephpfunctions.com/code/c2007754d416a55aea54a6630a990b984cb08c6d

added check both variants with array diff

zakdma · 2020-12-08T09:59:36Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

+     * @param array $websiteIds
+     * @return array
+     */
+    private function getStoresListByWebsiteIds(array $websiteIds): array


Could you move this code in separate class since you use it also in UpdateProductWebsiteUrlRewrites
Store module should be right place for it

zakdma · 2020-12-08T10:01:34Z

app/code/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserver.php

+     */
+    private function isRewriteUseCategoryPath(): bool
+    {
+        return $this->scopeConfig->isSetFlag('catalog/seo/product_use_categories');


Looks like you check flag for default scope but not for particular scope

wrong configuration was taken, changed to Generate "category/product" URL Rewrites global config instead

zakdma · 2020-12-08T11:06:35Z

app/code/Magento/CatalogUrlRewrite/Model/Product/GetProductUrlRewriteDataByStore.php

+    {
+        $productId = $product->getId();
+        if (isset($this->urlRewriteData[$productId][$storeId])) {
+            return $this->urlRewriteData[$productId][$storeId];


Its a little bit dangerous to have cache here. You should at least have method to reset cache and do it after rewrites changed

added clearing of local cache by product after generation

Please name it clearProductUrlRewriteDataCache to clarify that we clear exactly cache, not url rewrite data itself

zakdma · 2020-12-08T11:40:42Z

.../testsuite/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserverTest.php

+                'is_auto_generated' => 1,
+                'redirect_type' => 0,
+                'store_id' => $testStore4->getId(),
+            ]


zakdma · 2020-12-08T11:41:01Z

.../testsuite/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserverTest.php

+                'is_auto_generated' => 1,
+                'redirect_type' => 0,
+                'store_id' => $testStore4->getId(),
+            ]


zakdma · 2020-12-08T11:41:15Z

.../testsuite/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserverTest.php

+                'is_auto_generated' => 1,
+                'redirect_type' => 0,
+                'store_id' => $testStore1->getId(),
+            ]


zakdma · 2020-12-08T11:41:35Z

.../testsuite/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserverTest.php

+                'is_auto_generated' => 1,
+                'redirect_type' => 0,
+                'store_id' => $testStore3->getId(),
+            ]


zakdma · 2020-12-08T11:41:52Z

.../testsuite/Magento/CatalogUrlRewrite/Observer/ProductProcessUrlRewriteSavingObserverTest.php

+                'redirect_type' => 0,
+                'store_id' => $testStore4->getId(),
+            ]
+        ];


engcom-Golf · 2020-12-08T14:41:55Z

@magento run all tests

zakdma · 2020-12-08T15:02:14Z

app/code/Magento/CatalogUrlRewrite/Model/Product/GetProductUrlRewriteDataByStore.php

+     *
+     * @param ProductInterface $product
+     */
+    public function clearProductRewriteData(ProductInterface $product)


Please name it clearProductUrlRewriteDataCache to clarify that we clear exactly cache, not url rewrite data itself

zakdma · 2020-12-08T15:02:37Z

app/code/Magento/CatalogUrlRewrite/Model/Product/GetProductUrlRewriteDataByStore.php

+    {
+        $productId = $product->getId();
+        if (isset($this->urlRewriteData[$productId][$storeId])) {
+            return $this->urlRewriteData[$productId][$storeId];


Please name it clearProductUrlRewriteDataCache to clarify that we clear exactly cache, not url rewrite data itself

…rom website

engcom-Golf · 2020-12-08T16:11:39Z

@magento run all tests

zakdma · 2020-12-10T09:31:46Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-12-10T09:34:55Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-31106.

…rom website

zakdma · 2020-12-10T16:48:56Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-12-10T16:51:00Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-31106.

m2-assistant · 2020-12-12T08:00:00Z

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

magento-engcom-team added Component: Catalog Component: CatalogUrlRewrite Release Line: 2.4 labels Dec 1, 2020

engcom-Golf force-pushed the MC-38931 branch from 1d35d25 to 2151e6c Compare December 1, 2020 09:16

engcom-Golf requested review from engcom-Kilo, zakdma and viktorpetryk December 2, 2020 08:14

engcom-Kilo suggested changes Dec 2, 2020

View reviewed changes

m2-community-project bot assigned engcom-Kilo Dec 2, 2020

m2-community-project bot added the Progress: needs update label Dec 2, 2020

sidolov added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Dec 2, 2020

m2-community-project bot added Progress: review and removed Progress: needs update labels Dec 2, 2020

engcom-Golf requested a review from engcom-Kilo December 3, 2020 13:15

engcom-Kilo suggested changes Dec 3, 2020

View reviewed changes

engcom-Golf force-pushed the MC-38931 branch from c1129ad to f277113 Compare December 3, 2020 14:27

engcom-Kilo approved these changes Dec 3, 2020

View reviewed changes

engcom-Golf requested a review from engcom-Kilo December 4, 2020 11:26

engcom-Golf force-pushed the MC-38931 branch from b3124d6 to 5c118d3 Compare December 7, 2020 08:12

engcom-Golf force-pushed the MC-38931 branch from 5c118d3 to 6460928 Compare December 7, 2020 11:28

engcom-Golf force-pushed the MC-38931 branch from 6460928 to 67defbf Compare December 7, 2020 16:04

zakdma suggested changes Dec 8, 2020

View reviewed changes

m2-community-project bot assigned zakdma Dec 8, 2020

zakdma reviewed Dec 8, 2020

View reviewed changes

engcom-Golf force-pushed the MC-38931 branch from 95eb63a to f8d7e0d Compare December 8, 2020 15:23

MC-38931: Product URL Rewrites are not removed when product removed f…

2195e72

…rom website

engcom-Golf force-pushed the MC-38931 branch from f8d7e0d to 2195e72 Compare December 8, 2020 16:11

zakdma approved these changes Dec 8, 2020

View reviewed changes

engcom-Golf force-pushed the MC-38931 branch from 2195e72 to a08e303 Compare December 10, 2020 15:04

MC-38931: Product URL Rewrites are not removed when product removed f…

d30fd1f

…rom website

engcom-Golf force-pushed the MC-38931 branch from e31e93e to d30fd1f Compare December 10, 2020 15:21

magento-engcom-team merged commit a41bd02 into magento:2.4-develop Dec 12, 2020

chelevich mentioned this pull request Apr 24, 2021

Cannot create product "URL key for specified store already exists." - pure magento, no migration, no scripts, no categories #32832

Closed

5 tasks

		* @magentoDataFixture Magento/Catalog/_files/category_product.php
		* @magentoDataFixture Magento/UrlRewrite/_files/url_rewrite.php

MC-38931: Product URL Rewrites are not removed when product removed from website #31106

MC-38931: Product URL Rewrites are not removed when product removed from website #31106

Uh oh!

Conversation

engcom-Golf commented Dec 1, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Dec 1, 2020

Uh oh!

engcom-Golf commented Dec 1, 2020

Uh oh!

engcom-Kilo Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

engcom-Golf commented Dec 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

engcom-Golf commented Dec 3, 2020

Uh oh!

zakdma commented Dec 3, 2020

Uh oh!

engcom-Golf commented Dec 3, 2020

Uh oh!

engcom-Kilo Dec 2, 2020 •

edited

Loading

zakdma Dec 8, 2020 •

edited

Loading