Skip to content

Conversation

@m-michalis
Copy link
Contributor

@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
@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.

@sreichel
Copy link
Contributor

@m-michalis do you have time to re-review? Next release is planed in a few days. Would be nice to have it in.

@sreichel sreichel requested a review from addison74 December 3, 2025 17:40
@m-michalis
Copy link
Contributor Author

@m-michalis do you have time to re-review? Next release is planed in a few days. Would be nice to have it in.

@sreichel i was overloaded for the past month. i'll try committing some hours in the next weeks.

@addison74
Copy link
Contributor

addison74 commented Dec 13, 2025

I'll take the time to check a few PRs, this being one of them.

@m-michalis: PHP CS Fixer errors are easy to fix, the GitHub interface shows what they are, unless you use the command line in a DDEV test environment.

Also please let me know your opinion about this implementation https://github.com/MahoCommerce/maho/pull/345/files. Do we need a simple or configurable one?

@m-michalis
Copy link
Contributor Author

@addison74

@m-michalis: PHP CS Fixer errors are easy to fix, the GitHub interface shows what they are, unless you use the command line in a DDEV test environment.

I use DDEV. any tips?

Also please let me know your opinion about this implementation https://github.com/MahoCommerce/maho/pull/345/files. Do we need a simple or configurable one?

Opinion on what? From my experience it depends on the store. I've only seen 2 cases required, "always ask" and "skip images". I don't believe a store would use "always copy images" on product clone though.

@sreichel

#5083 (comment)
Please revert changes for trailing spaces.
which trailing spaces?

@sreichel
Copy link
Contributor

sreichel commented Dec 13, 2025

  • PHP CS Fixer and all other tools have ...
    • a DDEV command (ddev php-cs-fixer, ddev rector, ...)
    • a script in composer.json (compser run php-cs-fixer:fix, ...)
  • Having "copy always" makes no sense to me.
  • Ignore my comment about the spaces. Its okay.
  • Mage_Adminhtml_Model_System_Config_Source_Catalog_ImageDuplicate
    • has no copyright block
    • should have declare_strict set as new class

@addison74
Copy link
Contributor

addison74 commented Dec 14, 2025

1

My comments

  1. Skip Images on Duplicate can create confusion because it is not about the skip action, it is more than that. Here are a few ideas: "Product Images Handling on Duplicate", "Handle Product Images When Duplicating".
  2. The comment must be improved as follows The 'Ask' option only appears in the Admin interface. Programmatic duplication always preserves images.

2

My comments

1. Proposed dropdown order for product image duplication:

Always Ask → First

  • Puts control in the admin’s hands.
  • Ensures a conscious decision is made for each product duplication.
  • Prioritizing user interaction encourages careful handling of media.

Duplicate Product Without Images → Second

  • Modern and safe default for optimizing storage.
  • Encourages efficient media usage by not duplicating unnecessary files.
  • Makes it easy for admins to create duplicates without bloating the media folder.

Copy Images to the New Product → Last

  • Keeps Magento’s original behavior available as a fallback.
  • Not promoted as the default choice to discourage automatic file duplication.
  • Only use when true duplication of images is necessary for business reasons.

2. Better option names:

  • Always ask: Ask before duplicate, Prompt before duplicate, Confirm before duplicate
  • Copy images to the new product: Duplicate product with images (actually it is not just copy, it is a copy and rename action)
  • Duplicate product without images: OK

@addison74
Copy link
Contributor

addison74 commented Dec 14, 2025

Regarding the window content.

Screenshot 2025-12-14 215127

This is my proposal for this window:

Title: Duplicate Product
Question: This product has images. Do you want to create a duplicate product that includes them?
Buttons: Duplicate with images (left) and Duplicate without images (right)

Here is the visual result

Screenshot 2025-12-14 221120

@addison74
Copy link
Contributor

I appreciated the prompt is correctly displayed only when the product has images; and it is skipped when there are no images, which improves the UX significantly. Great work, just check my previous comments and improve.

@addison74 addison74 requested a review from sreichel December 14, 2025 20:25
@sreichel sreichel added this to the 20.17.0 milestone Dec 17, 2025
@sreichel sreichel modified the milestones: 20.17.0, 20.18.0 Dec 17, 2025
m-michalis and others added 3 commits January 9, 2026 19:42
Co-authored-by: Sven Reichel <[email protected]>
Co-authored-by: Sven Reichel <[email protected]>
- more descriptive labeling/text
- fixed ProductController.php:827
@m-michalis
Copy link
Contributor Author

@sreichel @addison74 i've done some changes, see last commit.

how is it formatting with spaces? which tool does this? i have wrong configuration? i mean with:

 * @method int                                                getEntityTypeId()
 * @method bool                                               getForceReindexRequired()
 * @method array                                              getGroupedLinkData()

vs

 * @method int getEntityTypeId()
 * @method bool getForceReindexRequired()
 * @method array getGroupedLinkData()

can't resolve this. Could you shed some light on this?

@addison74
Copy link
Contributor

@m-michalis - the conflict is solved. The first formatting is the correct one. Please check the file again if I missed something from the docblock.

Align variable declaration formatting for attributeId.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2026

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2026

how is it formatting with spaces? which tool does this?

its php-cs-fixer ... check scripts section in composer.json to automatically fix it.

@addison74
Copy link
Contributor

I would like to get PR ready on the next version, as it was proposed two months ago.

We have to discuss the texts used in the config area, then the title, content and buttons of the window (remained unchanged in its initial form). We will revise the code later. At first glance it looks fine but there are some getters and setters there. We'll see...

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2026

I would like to get PR ready on the next version, as it was proposed two months ago.

You can change milestones too.

As we have progess here thats fine.

@addison74
Copy link
Contributor

All right then. We will approve it then I will create a PR for solving the texts and especially the window which is not looking good.

@addison74 addison74 merged commit 2b50770 into OpenMage:main Jan 9, 2026
21 checks passed
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.

3 participants