Skip to content

Module Catalog: fix issue with getting infinite loop while using product "Save and Duplicate" feature in Admin panel #18567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
oleksii-lisovyi wants to merge 1 commit into from
Closed

Module Catalog: fix issue with getting infinite loop while using product "Save and Duplicate" feature in Admin panel #18567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions app/code/Magento/Catalog/Model/Product/Copier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,14 @@ public function copy(Product $product)
$duplicate->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID);

$this->copyConstructor->build($product, $duplicate);
$isDuplicateSaved = false;
do {
$urlKey = $duplicate->getUrlKey();
$urlKey = preg_match('/(.*)-(\d+)$/', $urlKey, $matches)
? $matches[1] . '-' . ($matches[2] + 1)
: $urlKey . '-1';
$duplicate->setUrlKey($urlKey);
try {
$duplicate->save();
$isDuplicateSaved = true;
} catch (\Magento\Framework\Exception\AlreadyExistsException $e) {
} catch (\Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException $e) {
}
} while (!$isDuplicateSaved);

$urlKey = $duplicate->getUrlKey();
$urlKey = preg_match('/(.*)-(\d+)$/', $urlKey, $matches)
? $matches[1] . '-' . ($matches[2] + 1)
: $urlKey . '-1';
$duplicate->setUrlKey($urlKey);
$duplicate->save();
Copy link
Contributor

@orlangur orlangur Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such change is incorrect as new code tries to save duplicate only once - while the whole point of loop was in guaranteed saving of a duplicated product.

@hostep suggestion makes perfect sense to me, please try to do a backport of this logic and check if it solves your problem.

Copy link
Author

@oleksii-lisovyi oleksii-lisovyi Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orlangur, the only thing, that the loop guarantees, that there will be infinite trials of saving the duplicated product. It doesn't guarantees that the duplicated product actually is saved at the second or third trial and so on.

It means, that having any piece of code, that causes getting any of catched exceptions breaks your website.
It could be not the Magento module, but having such loop makes an opportunity of having infinite loop and broken production, because someone didn't write the module in the right way.

Copy link
Contributor

@hostep hostep Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @oleksii-lisovyi here, this do while loop can cause major problems, this should be solved in a better way then doing a potential infinite loop.
Finding a unique url key should not be done by trying to save a product over and over again, this should be done separately somehow.

Copy link
Contributor

@orlangur orlangur Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleksii-lisovyi @hostep

} catch (\Magento\Framework\Exception\AlreadyExistsException $e) {
} catch (\Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException $e) {

Such exceptions are specific enough, if some third-party code throws them permanently - it really needs to be fixed.

The problem mentioned is solved in 2.3 and loop is still there:

magento2/app/code/Magento/Catalog/Model/Product/Copier.php

Line 92 in 50592b4

} while (!$isDuplicateSaved);

Finding a unique url key should not be done by trying to save a product over and over again, this should be done separately somehow.

Please tell more about this "somehow". Main purpose of this code is to avoid race condition (when some URL Key we would use was added by somebody else). Otherwise we would just try the next free URL Key and it would work.

And again, 2.2 should not live on its own, any backportable fix from 2.3 must be backported.

Copy link
Contributor

@hostep hostep Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main purpose of this code is to avoid race condition

Aha, I wasn't aware of the purpose of this code, but this makes sense indeed!
Some inline comments might prove useful here.

Please tell more about this "somehow"

If it's to avoid a race condition, then this will be pretty hard to do indeed.
Can't we just show an error when a duplicate url key was detected and tell to user to retry or something like that?

Copy link
Contributor

@orlangur orlangur Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hostep why? This code simply works in 2.3 and I see no issue in it for 2.2

Copy link
Contributor

@hostep hostep Oct 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a save method can trigger things you don't want to trigger more than once.
In the case of #9466, calling save more than once copies the image files on the filesystem associated with the product more than once, even though by duplicating a product, you expect the image files to only be copied a single time.

Just some wild idea: can we not first add a completely random unique bunch of characters (a hash if you will) as a url key to the duplicated product, then save the product, and then generate a more sensible url key using that do while loop and only save that on the product so only a single attribute gets changed?

Copy link
Contributor

@orlangur orlangur Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hostep if things are added more than once when calling save, they are not implemented well.


$this->getOptionRepository()->duplicate($product, $duplicate);
$product->getResource()->duplicate(
$product->getData($metadata->getLinkField()),
Expand Down