Skip to content
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.

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

Fix the correct removal of the images and the removal of all images in the catalog [Backport] #11153

Merged
merged 9 commits into from
Oct 12, 2017

Conversation

raumatbel
Copy link
Contributor

@raumatbel raumatbel commented Sep 30, 2017

Related with the PR#11147
Fix the correct removal of the images and the removal of all images in the catalog

Description

When you want remove image from admin panel, this file is not removed correctly.
If the path of the image saved in the table catalog_product_entity_media_gallery is empty, this remove all the catalog.

Manual testing scenarios

For the first case.

  1. Upload image from admin panel
  2. Save product
  3. Remove this image
  4. Save product
    The file is not removed in pub/media/catalog/product/...

For the second case.

  1. Upload image from admin panel
  2. Save product
  3. Go to the DB to the table catalog_product_entity_media_gallery and remove the value of this image
  4. Remove this image from admin panel
  5. Save product
    All the files are removed in pub/media/catalog/product/...

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 on Travis CI are green)

@raumatbel raumatbel closed this Sep 30, 2017
@raumatbel raumatbel reopened this Sep 30, 2017
@vkublytskyi vkublytskyi self-assigned this Oct 3, 2017
@vkublytskyi vkublytskyi added this to the October 2017 milestone Oct 3, 2017
@vkublytskyi
Copy link

@raumatbel Thank you for your contribution. We will merge your fixes to all branches shortly.

$catalogPath = $this->mediaConfig->getBaseMediaPath();
$isFile = $this->mediaDirectory->isFile($catalogPath . $image['file']);
// only delete physical files if they are not used by any other products and if this file exist
if (!($this->resourceModel->countImageUses($image['file']) > 1) && $isFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check "$isFile" is more faster, so would be great to swap them.

Copy link
Contributor Author

@raumatbel raumatbel Oct 4, 2017

Choose a reason for hiding this comment

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

Can you explain me again? I don't understand well.
Do you want say me that I must not use the variable $isFile and I must specify it in the same line?.

   if (!($this->resourceModel->countImageUses($image['file']) > 1) && $this->mediaDirectory->isFile($this->mediaConfig->getBaseMediaPath() . $image['file']);) {

Choose a reason for hiding this comment

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

@raumatbel code if ($isFile && !($this->resourceModel->countImageUses($image['file']) > 1)) would be more optimal from performance perspective as if this is not a file then usage of image will not be checked.
I left up to you to make this fix or not as it may be considered as premature optimization. I think in code like this readability has higher priority.

magento-team pushed a commit that referenced this pull request Oct 5, 2017
@raumatbel raumatbel changed the title Fix the correct removal of the images and the removal of all images in the catalog Fix the correct removal of the images and the removal of all images in the catalog [Backport] Oct 9, 2017
@okorshenko okorshenko self-assigned this Oct 11, 2017
@magento-team magento-team merged commit 10ef2e7 into magento:develop Oct 12, 2017
magento-team pushed a commit that referenced this pull request Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants