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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent resizing an image if it was already resized before #26801

Merged
merged 3 commits into from
Apr 4, 2020

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Feb 10, 2020

Description (*)

Images shouldn't get resized again if they were already resized before when running bin/magento catalog:images:resize.
In Magento 2.1 they didn't get resized when they were already resized before. Somewhere between Magento 2.1.18 and 2.2.11 that got changed for some reason.

I also added some cleanup to that class in a second commit, because some use statements were missing meaningful aliases and some member variables weren't being used.

Related Pull Requests

Fixed Issues (if relevant)

  1. Running 'bin/magento catalog:images:resize' doesn't check if an image was already resized before, therefore it is superslow as it always resizes all images聽#26796: Running 'bin/magento catalog:images:resize' doesn't check if an image was already resized before, therefore it is superslow as it always resizes all images

Manual testing scenarios (*)

  1. Have a product with an image
  2. Remove the directory pub/media/catalog/product/cache/
  3. Run bin/magento catalog:images:resize
  4. Look at the generated images in pub/media/catalog/product/cache/ and check their modified timestamps
  5. Wait a bit
  6. Run bin/magento catalog:images:resize again
  7. Look at the generated images in pub/media/catalog/product/cache/ and check their modified timestamps

Please also test when using media storage in the database, I haven't tested that but the code should work I believe.

Questions or comments

I know the unit tests will fail, will take a look later, not that much time currently. Feel free to pick this up yourselves, I don't mind 馃檪 - Update: Already Done

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Feb 10, 2020

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@hostep
Copy link
Contributor Author

hostep commented Mar 17, 2020

Rebased on 2.4-develop and force pushed because 2 recent commits caused merge conflicts:

@hostep
Copy link
Contributor Author

hostep commented Mar 18, 2020

Rebased and force pushed again to fix static tests (hopefully).

@hostep hostep force-pushed the fix-for-issue-26796 branch 2 times, most recently from c9d8235 to bd695f3 Compare March 18, 2020 12:13
@magento-engcom-team magento-engcom-team added this to Pending Review in Pull Requests Dashboard Mar 24, 2020
@hostep
Copy link
Contributor Author

hostep commented Mar 25, 2020

Rebased again, because a new commit caused a merge conflict yet again: c093733

@lenaorobei lenaorobei added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix labels Mar 30, 2020
@ghost ghost moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Mar 30, 2020
@ghost ghost assigned lenaorobei Mar 30, 2020
@magento-engcom-team
Copy link
Contributor

Hi @lenaorobei, thank you for the review.
ENGCOM-7234 has been created to process this Pull Request

@engcom-Alfa engcom-Alfa self-assigned this Apr 1, 2020
@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Apr 2, 2020
@engcom-Alfa
Copy link
Contributor

鉁旓笍 QA Passed

Tested with File System and Database media storages

Case 1.

File System

  1. Have a product with an image
  2. Remove the directory pub/media/catalog/product/cache/
  3. Run bin/magento catalog:images:resize
  4. Look at the generated images in pub/media/catalog/product/cache/ and check their modified timestamps
  5. Wait a bit
  6. Run bin/magento catalog:images:resize again

鉁旓笍 The images didn't resize and the timestamp didn't change

Case 2.

DB media storage

  1. Configure media storage to Database;
  2. Have a product with an image
  3. Go to Admin->System->Cache Management;
  4. Click on Flush Catalog Images Cache button;
  5. Run bin/magento catalog:images:resize;
  6. Look at the generated images in pub/media/catalog/product/cache/ and check their modified timestamps
  7. Wait a bit
  8. Run bin/magento catalog:images:resize again

鉁旓笍 The images didn't resize and the timestamp didn't change

@engcom-Alfa engcom-Alfa moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Apr 2, 2020
@engcom-Echo engcom-Echo self-assigned this Apr 2, 2020
@engcom-Echo engcom-Echo moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Apr 2, 2020
@magento-engcom-team magento-engcom-team merged commit 88b200c into magento:2.4-develop Apr 4, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 4, 2020

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@VivekShingala
Copy link

We are on Magento 2.3.5-p2. This patch is a lifesaver when images are stored in the file system, however, it doesn't work when we have CDN (S3 Bucket) enabled for our website.

Any help to fix it would be very helpful.

@hostep
Copy link
Contributor Author

hostep commented Nov 7, 2020

@VivekShingala: can you open a new issue for this with as much information as possible like how you are connecting with S3 and such? Thanks!

@VivekShingala
Copy link

Hi @hostep

This patch is working fine for Magento 2.3.5 but not for 2.3.2. Any solution for M2.3.2 available?

@hostep
Copy link
Contributor Author

hostep commented Apr 23, 2021

@VivekShingala

What you can try is:

  • Clone the Magento 2 repository on a local machine
  • Checkout the 2.3.2 tag
  • Download the https://patch-diff.githubusercontent.com/raw/magento/magento2/pull/26801.diff patch
  • Apply that patch on the Magento 2 repository using something like git apply -v 26801.diff
  • If it doesn't work, try to resolve conflicts somehow until it works (or apply the changes manually line by line)
  • Then generate a new patch out of the changed files in a new file using something like git diff app/code/Magento/MediaStorage > my-new-patch.diff

That's how I usually try to backport patches that can't be applied cleanly.
It might be that it won't work in this particular case, I don't know, but you could try, good luck! 馃檪

@hostep
Copy link
Contributor Author

hostep commented Apr 23, 2021

Ow, you're in luck, it looks like we do have a patch for this for Magento 2.3.2, it's an earlier version of the commits I pushed here, but it probably works equally as well:

PR-26801-MediaStorage.txt

Please test very well, no guarantees that it is flawless, but I think it should work ok if you don't store the media files in the database 馃檪

@VivekShingala
Copy link

Hi @hostep

The patch you provided works perfectly on Magento 2.3.2 setup. Thank you very much.

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.

None yet

6 participants