Skip to content

Only resize images for themes which are being used instead of all installed themes. #8142

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

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Jan 15, 2017

Currently Magento resizes catalog images in the frontend for all the installed themes.
So if you have the Magento/blank, Magento/luma and your own custom theme, it would resize the images for those three themes. Even if you only use your own custom theme in the frontend.

This PR should fix this, it will only resize catalog images for themes which are actually active.

This code is being executed when running the command: bin/magento catalog:images:resize and also in the afterSave method of the Magento\Catalog\Model\Product class.

@antonkril antonkril requested a review from kandy January 15, 2017 11:27
@paveq
Copy link
Contributor

paveq commented Feb 9, 2017

While this will improve current situation, am I correct that there is still possibility to generate images that are never used?

Consider scenario where there are two websites (with separate sets of products) with different themes inside single Magento installation. Since both themes are active, images will get generated for both themes even if the product is active only in one website.

Ideally we would generate image for themes that are used in websites where the product is active?

@hostep
Copy link
Contributor Author

hostep commented Feb 9, 2017

@paveq: correct!

This is a first attempt at reducing the amount of generated resized images. But this can be optimized further like you are suggesting.

@maksek maksek self-assigned this Feb 16, 2017
@maksek maksek self-requested a review February 16, 2017 14:45
@maksek maksek added this to the February 2017 milestone Feb 16, 2017
Copy link
Contributor

@vrann vrann left a comment

Choose a reason for hiding this comment

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

Please check and fix the case when theme is set in other places than store

$themesInUse = [];

$registeredThemes = $this->themeCollection->loadRegisteredThemes();
$storesByThemes = $this->themeCustomizationConfig->getStoresByThemes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Themes can be set not only on store level, but on product/category/cms. Please addthe logic which would include these cases as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hostep please see comment above

Copy link
Contributor

Choose a reason for hiding this comment

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

@vrann @hostep I've used this code and added themes checking for produt, category and pages.
denisristic@8308ae1#comments

Copy link
Contributor

Choose a reason for hiding this comment

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

@vrann Is it actually necessary to check for product/category/page?
Even if any of them have assigned custom theme that is disabled it would not matter because it would not be displayed because theme is disabled?

@paveq
Copy link
Contributor

paveq commented Feb 21, 2017

@vrann I'm not sure if it is even possible performance-wise to get all combinations of per page used themes per product. And even then, these can be easily changed by store admin, so can we ever be sure we pre-generate correct images?

I see one alternative way going forward: add a boolean toggle to admin "pre-generate images". Allow people to use old style M1 "on-fly" generation, which is always 100% guaranteed to generate only images that are ever used somewhere.

@hostep
Copy link
Contributor Author

hostep commented Feb 21, 2017

@vrann: thanks for the review. Since the changes you request will turn out to be really complicated, I'm no longer going to pursue this myself.
So if somebody else want to continue with this, be my guest :)

@maksek maksek modified the milestones: February 2017, March 2017 Mar 1, 2017
@okorshenko okorshenko added this to the March 2017 milestone Mar 1, 2017
@okorshenko okorshenko modified the milestones: March 2017, April 2017 Apr 2, 2017
@okorshenko okorshenko modified the milestones: April 2017, May 2017 May 9, 2017
@okorshenko okorshenko removed this from the April 2017 milestone May 9, 2017
@okorshenko
Copy link
Contributor

Closing this PR due to inactivity since Feb 21

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