Module Catalog: fix issue with getting infinite loop while using product "Save and Duplicate" feature in Admin panel #18567
Conversation
…uct "Save and Duplicate" feature in Admin panel
|
Hi @oleksii-lisovyi. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
|
@oleksii-lisovyi, just out of curiosity: did you test it with a product which has a different url_key then the default value on a specific storeview level? |
|
Just a quick heads up, this commit landed in the But the do while removal in this PR is certainly a good thing! |
| ? $matches[1] . '-' . ($matches[2] + 1) | ||
| : $urlKey . '-1'; | ||
| $duplicate->setUrlKey($urlKey); | ||
| $duplicate->save(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
} 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:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@hostep why? This code simply works in 2.3 and I see no issue in it for 2.2
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@hostep if things are added more than once when calling save, they are not implemented well.
|
@oleksii-lisovyi could you please clarify if #18566 causes #18532 as mentioned in its description or fixes it? |
|
@orlangur, looks like the fix I provided fixes the issue with infinite loop. There is an issue with product images duplicating, that is caused by having this loop: each iteration generates all the images from the original product, but they aren't cleared once duplicated product saving failed. |
4 participants
This PR fixes infinite loop issue while using "Save and Duplicate" feature on product edit page in Admin panel.
Description
Removing catching any of \Magento\Framework\Exception\AlreadyExistsException & \Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException exceptions prevents getting potential infinite loop, that causes the issues described in the related issue (see below).
Getting infinite loop is caused by issue with incorrect product
URL Pathre-generation in Catalog URL Rewrite module (here is related PR: #18566).Fixed Issues (if relevant)
#18532: Module Catalog: product "Save and Duplicate" causes getting infinite loop.
Manual testing scenarios
Perform steps to reproduce from the PR #18566.
Contribution checklist