Skip to content

Conversation

@m-michalis
Copy link

@m-michalis m-michalis commented Nov 9, 2025

Description

I'm splitting up the work in 3 PRs after @addison74's comment in

Parts:

  • Keep/Skip product images (this pr): Ask whether to keep or skip product images on "Duplicate" + configuration for automating this behavior
  • Delete product/images: Whether to physically delete images on either when removing images from a product and/or deleting a product + configuration for default behavior.
  • Use new copied images: Copy and actually use copied images on new duplicated product (bug for many years) + propagate store values correctly regarding image, small_image, thumbnail attributes

This PR: Keep/Skip product images

  • Configuration option System > Configuration > Catalog > Product Image > Skip Images on Duplicate (always ask, skip, keep)
  • Prompt user on "Duplicate" click button.
  • Added
     // ..
     * @method bool getSkipImagesOnDuplicate()
     * @method $this setSkipImagesOnDuplicate(bool $value)
     // ..
     class Mage_Catalog_Model_Product // ..
  • Translation strings
  • Tests

Questions or comments

Need help in adding tests

…copy the images or not.

added a configuration option “Skip Images on Duplicate” and a small prompt on Duplicate when the
source product has images.
@github-actions github-actions bot added Component: Catalog Relates to Mage_Catalog Template : admin Relates to admin template Component: Adminhtml Relates to Mage_Adminhtml translations Relates to app/locale labels Nov 9, 2025
# Conflicts:
#	app/code/core/Mage/Catalog/Model/Product.php
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 9, 2025

Comment on lines +656 to +662
/**
* @return int
*/
public function skipProductImageOnDuplicate()
{
return Mage::getStoreConfigAsInt(self::XML_NODE_SKIP_IMAGE_ON_DUPLICATE_ACTION);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @return int
*/
public function skipProductImageOnDuplicate()
{
return Mage::getStoreConfigAsInt(self::XML_NODE_SKIP_IMAGE_ON_DUPLICATE_ACTION);
}
public function skipProductImageOnDuplicate(): int
{
return Mage::getStoreConfigAsInt(self::XML_NODE_SKIP_IMAGE_ON_DUPLICATE_ACTION);
}

* @return string
*/
public function getDuplicateUrl()
public function getDuplicateUrl($skipImages = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function getDuplicateUrl($skipImages = false)
public function getDuplicateUrl(bool $skipImages = false)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a new method getDuplicateSkipUrl()?

Comment on lines +31 to +36
/**
* Used when duplicating product
*
* @var string
*/
protected $_skipImagesOnDuplicate = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Used when duplicating product
*
* @var string
*/
protected $_skipImagesOnDuplicate = false;
/**
* Used when duplicating product
*/
protected bool $skipImagesOnDuplicate = false;

Comment on lines +747 to +752
/**
* @param bool $newProductSkipImages
* @return $this
*/
public function setSkipImagesOnDuplicate(bool $newProductSkipImages){
$this->_skipImagesOnDuplicate = $newProductSkipImages;
Copy link
Contributor

@sreichel sreichel Nov 9, 2025

Choose a reason for hiding this comment

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

Suggested change
/**
* @param bool $newProductSkipImages
* @return $this
*/
public function setSkipImagesOnDuplicate(bool $newProductSkipImages){
$this->_skipImagesOnDuplicate = $newProductSkipImages;
/**
* @return $this
*/
public function setSkipImagesOnDuplicate(bool $newProductSkipImages)
{
$this->skipImagesOnDuplicate = $newProductSkipImages;

Comment on lines +756 to +761
/**
* @return bool|string
*/
public function getSkipImagesOnDuplicate(){
return $this->_skipImagesOnDuplicate;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @return bool|string
*/
public function getSkipImagesOnDuplicate(){
return $this->_skipImagesOnDuplicate;
}
public function getSkipImagesOnDuplicate(): bool
{
return $this->skipImagesOnDuplicate;
}

Comment on lines +762 to +763


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +1370 to +1374
if($newProduct->getSkipImagesOnDuplicate() == null && $this->_getImageHelper()->skipProductImageOnDuplicate() === -1){
$newProduct->setSkipImagesOnDuplicate(false);
}else{
$newProduct->setSkipImagesOnDuplicate((bool) $this->_getImageHelper()->skipProductImageOnDuplicate());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if($newProduct->getSkipImagesOnDuplicate() == null && $this->_getImageHelper()->skipProductImageOnDuplicate() === -1){
$newProduct->setSkipImagesOnDuplicate(false);
}else{
$newProduct->setSkipImagesOnDuplicate((bool) $this->_getImageHelper()->skipProductImageOnDuplicate());
}
if (is_null($newProduct->getSkipImagesOnDuplicate()) && $this->_getImageHelper()->skipProductImageOnDuplicate() === -1) { # todo: use constant
$newProduct->setSkipImagesOnDuplicate(false);
} else {
$newProduct->setSkipImagesOnDuplicate($this->_getImageHelper()->skipProductImageOnDuplicate());
}

@sreichel
Copy link
Contributor

sreichel commented Nov 9, 2025

Much thanks.

Please run phpcs-fixer and phpstan. Some errors should be fixed via suggestions.

I'd add constants for options, of -1, 0 and 1.

Please revert changes for trailing spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Template : admin Relates to admin template translations Relates to app/locale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants