Only resize images (cache) for themes which are being used instead of all installed themes #11514

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.

Sign up for GitHub

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

Jump to bottom

Closed

denisristic wants to merge 3 commits into magento:2.2-develop from denisristic:issue-10363

Contributor

denisristic commented Oct 17, 2017

Only resize images (cache) for themes which are being used instead of all installed themes

Description

Pull request is using @vrann fix and it is upgraded to check (as suggested in comments) for used (custom) themes in products/categories/cms pages.

Fixed Issues (if relevant)

Manual testing scenarios

Add custom theme for product/category/cms page
Disable some theme
Upload product image
Check if cache image is generated for disabled theme

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)

Denis Ristic added 2 commits

October 16, 2017 13:48


          ADDED getThemesInUse (using [@vrann fix](magento@35e3225#diff-dbf0423…

8308ae1

…2abcedbe2e67996b043041b48) + added check for used themes in products, categories and cms pages)

REF magento2/issues#10363

UnitTests are failing due to mocking problems:
```
1) Magento\Catalog\Test\Unit\Model\Product\Image\CacheTest::testGenerate
Error: Call to a member function getId() on null
``


          Removed ObjectManager from Cache Class

09a7cb2

Refactored Unit Test

denisristic changed the title ~~Issue 10363~~ Only resize images (cache) for themes which are being used instead of all installed themes

ishakhsuvarov reviewed

View reviewed changes

app/code/Magento/Catalog/Model/Product/Image/Cache.php Outdated

@@ @@ -24,11 +27,27 @@ class Cache @@
                   protected $themeCollection;
                   /**
+                   * @var ThemeCustomizationConfig
+                   */
+                  protected $themeCustomizationConfig;

Contributor

ishakhsuvarov Oct 18, 2017

Adding new protected properties is expanding the contract of the class, which is not really desired in this case. Please change to private

Contributor Author

denisristic Oct 18, 2017

changed to private

ishakhsuvarov requested changes

View reviewed changes

app/code/Magento/Catalog/Model/Product/Image/Cache.php Outdated

+                   *
+                   * @return array
+                   */
+                  public function getThemesInUse()

Contributor

ishakhsuvarov Oct 18, 2017

Does this method need to be public?
Please consider extracting it into appropriate service, as getThemesInUse does not seem to have too much in common with Product Image Cache Model.

Contributor Author

denisristic Oct 18, 2017

Where would you suggest I should put it? Inside themes?

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                          }
+                      }
+                      $connection = $this->attribute->getResource()->getConnection();

Contributor

ishakhsuvarov Oct 18, 2017

Getting dependencies in such way (one dependency via another one) violates the principle of least knowledge.
Please consider refactoring this.

app/code/Magento/Catalog/Model/Product/Image/Cache.php Outdated


		$productCustomDesignAttributeId = $this->attribute->loadByCode(4, 'custom_design')->getId();

		$productSql = $connection

Contributor

ishakhsuvarov Oct 18, 2017

Direct access to the data layer is out of responsibilities scope for Magento Models. I suggest to move it into appropriate resource model. This would also help to resolve the commend above (on the least knowledge principle).

app/code/Magento/Catalog/Model/Product/Image/Cache.php Outdated


		$connection = $this->attribute->getResource()->getConnection();

		$productCustomDesignAttributeId = $this->attribute->loadByCode(4, 'custom_design')->getId();

Contributor

ishakhsuvarov Oct 18, 2017

Is there a way to avoid hardcoding the 4?

Contributor Author

denisristic Oct 18, 2017

fixed

app/code/Magento/Catalog/Model/Product/Image/Cache.php Outdated

+                   *
+                   * @return array
+                   */
+                  public function getThemesInUse()

Contributor

ishakhsuvarov Oct 18, 2017

Please add PHP 7.0 specific type hinting and return types to any newly introduced methods.

Contributor Author

denisristic Oct 18, 2017

added


          UPDATED as @ishakhsuvarov suggested

823ef40

- Adding new protected properties is expanding the contract of the class, which is not really desired in this case. Please change to private
- Please add PHP 7.0 specific type hinting and return types to any newly introduced methods.
- Is there a way to avoid hardcoding the 4?

ishakhsuvarov self-assigned this

ishakhsuvarov added this to the October 2017 milestone

ishakhsuvarov added Release Line: 2.2 2.2.x bug report Component: Catalog improvement Reproduced on 2.1.x Reproduced on 2.2.x Reproduced on 2.3.x labels

okorshenko modified the milestones: October 2017, November 2017

ishakhsuvarov removed their assignment

magento-engcom-team added bugfix Reproduced on develop labels

okorshenko removed the Reproduced on develop label

okorshenko assigned magento-engcom-team

sidolov self-assigned this

sidolov requested changes

View reviewed changes

app/code/Magento/Catalog/Model/Product/Image/Cache.php


		$productEntityTypeId = \Magento\Catalog\Setup\CategorySetup::CATALOG_PRODUCT_ENTITY_TYPE_ID;

		$productCustomDesignAttributeId = $this->attribute->loadByCode($productEntityTypeId, 'custom_design')->getId();

Contributor

sidolov Nov 21, 2017

Please use \Magento\Eav\Model\Config::getAttribute instead of load entity by model

app/code/Magento/Catalog/Model/Product/Image/Cache.php


		$connection = $this->attribute->getResource()->getConnection();

		$productEntityTypeId = \Magento\Catalog\Setup\CategorySetup::CATALOG_PRODUCT_ENTITY_TYPE_ID;

Contributor

sidolov Nov 21, 2017

Please use constant directly instead of assign to variable

app/code/Magento/Catalog/Model/Product/Image/Cache.php

@@ @@ -41,11 +60,17 @@ class Cache @@
                   public function __construct(
                       ConfigInterface $viewConfig,
                       ThemeCollection $themeCollection,
-                      ImageHelper $imageHelper
+                      ImageHelper $imageHelper,
+                      ThemeCustomizationConfig $themeCustomizationConfig,

Contributor

sidolov Nov 21, 2017

Dependencies added with backward compatibility standards violation, right way described in http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/ section: Adding a constructor parameter

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                      $storesByThemes   = $this->themeCustomizationConfig->getStoresByThemes();
+                      foreach ($registeredThemes as $registeredTheme) {
+                          if (array_key_exists($registeredTheme->getThemeId(), $storesByThemes)) {

Contributor

sidolov Nov 21, 2017

Use $registeredTheme->getCode() instead of $registeredTheme->getThemeId() otherwise this expression will return false permanently and $themesInUse array will be always empty. Without this fix cache directory is empty.

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                   *
+                   * @return array
+                   */
+                  private function getThemesInUse(): array

Contributor

sidolov Nov 21, 2017

Consider refactoring this logic with introducing composite object (with configuration via DI) for themes in use and move logic to corresponded modules (pages to CMS module; product, category -> Catalog module etc)

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                  /**
+                   * @var Config
+                   */
+                  protected $eavConfig;

Contributor

sidolov Nov 21, 2017

Please use private modifier for object dependencies

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                  /**
+                   * @var Attribute
+                   */
+                  protected $attribute;

Contributor

sidolov Nov 21, 2017

Please use private modifier for object dependencies

app/code/Magento/Catalog/Model/Product/Image/Cache.php

+                          }
+                      }
+                      $categoryEntityTypeId = \Magento\Catalog\Setup\CategorySetup::CATEGORY_ENTITY_TYPE_ID;

Contributor

sidolov Nov 21, 2017

Please use constant directly instead of assign to variable

app/code/Magento/Catalog/Model/Product/Image/Cache.php


		$categoryEntityTypeId = \Magento\Catalog\Setup\CategorySetup::CATEGORY_ENTITY_TYPE_ID;

		$categoryCustomDesignAttributeId = $this->attribute->loadByCode($categoryEntityTypeId, 'custom_design')->getId();

Contributor

sidolov Nov 21, 2017

Please use \Magento\Eav\Model\Config::getAttribute instead of load entity by model

magento-team added the Progress: needs update label

okorshenko modified the milestones: November 2017, December 2017

Contributor

sidolov commented Dec 5, 2017

@denisristic, I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for collaboration

sidolov closed this

okorshenko unassigned magento-engcom-team

eharvest commented Aug 30, 2018

Firstly I really want to thank @sidolov for making such effort to offer this solution and I want to give a bit feedback of how it worked for me.

I have a Magento 2.2.3 store (now in production). It has 20k products. Before I run catalog:images:resize the overall size is less than 20G, after run the command a few times (I always delete product image cache before run the command), the size is now growing to nearly 40G. And this what I get:

without applying the fix, the command took up to 22 hours to finish and my product image cache grows to 22G!!!
after applying the fix, the command took about half the time and my product image cache still as big as 18G. I saved 4G but it's still a lot bigger than I expected. I'm using a child theme, would that be the issue?

I can see there're quite a few developers have been complaining about this, and I really think this is a big performance issue, now we have been forced to use a much bigger server. Hopefully this can be fixed in 2.3 where catalog:images:resize can take args and only apply to the theme in use.

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

Labels

bug report bugfix Component: Catalog improvement Progress: needs update Release Line: 2.2 Reproduced on 2.1.x Reproduced on 2.2.x Reproduced on 2.3.x