Skip to content

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8308ae1
ADDED getThemesInUse (using [@vrann fix](https://github.com/magento/m…
Oct 16, 2017
09a7cb2
Removed ObjectManager from Cache Class
Oct 17, 2017
823ef40
UPDATED as @ishakhsuvarov suggested
Oct 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 118 additions & 2 deletions app/code/Magento/Catalog/Model/Product/Image/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
use Magento\Catalog\Helper\Image as ImageHelper;
use Magento\Catalog\Model\Product;
use Magento\Theme\Model\ResourceModel\Theme\Collection as ThemeCollection;
use Magento\Theme\Model\Config\Customization as ThemeCustomizationConfig;
use Magento\Framework\App\Area;
use Magento\Framework\View\ConfigInterface;
use Magento\Eav\Model\Config;
use Magento\Eav\Model\Entity\Attribute;

class Cache
{
Expand All @@ -23,11 +26,27 @@ class Cache
*/
protected $themeCollection;

/**
* @var ThemeCustomizationConfig
*/
protected $themeCustomizationConfig;
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@denisristic denisristic Oct 18, 2017

Choose a reason for hiding this comment

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

changed to private


/**
* @var ImageHelper
*/
protected $imageHelper;

/**
* @var Config
*/
protected $eavConfig;
Copy link
Contributor

@sidolov sidolov Nov 21, 2017

Choose a reason for hiding this comment

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

Please use private modifier for object dependencies


/**
* @var Attribute
*/
protected $attribute;
Copy link
Contributor

@sidolov sidolov Nov 21, 2017

Choose a reason for hiding this comment

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

Please use private modifier for object dependencies



/**
* @var array
*/
Expand All @@ -41,11 +60,17 @@ class Cache
public function __construct(
ConfigInterface $viewConfig,
ThemeCollection $themeCollection,
ImageHelper $imageHelper
ImageHelper $imageHelper,
ThemeCustomizationConfig $themeCustomizationConfig,
Copy link
Contributor

@sidolov sidolov Nov 21, 2017

Choose a reason for hiding this comment

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

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

Config $eavConfig,
Attribute $attribute
) {
$this->viewConfig = $viewConfig;
$this->themeCollection = $themeCollection;
$this->imageHelper = $imageHelper;
$this->themeCustomizationConfig = $themeCustomizationConfig;
$this->eavConfig = $eavConfig;
$this->attribute = $attribute;
}

/**
Expand All @@ -58,8 +83,10 @@ public function __construct(
protected function getData()
{
if (!$this->data) {
$themesInUse = $this->getThemesInUse();

/** @var \Magento\Theme\Model\Theme $theme */
foreach ($this->themeCollection->loadRegisteredThemes() as $theme) {
foreach ($themesInUse as $theme) {
$config = $this->viewConfig->getViewConfig([
'area' => Area::AREA_FRONTEND,
'themeModel' => $theme,
Expand Down Expand Up @@ -127,4 +154,93 @@ protected function processImageData(Product $product, array $imageData, $file)

return $this;
}

/**
* Get themes in use
*
* @return array
*/
public function getThemesInUse()
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@denisristic denisristic Oct 18, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@denisristic denisristic Oct 18, 2017

Choose a reason for hiding this comment

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

added

{
$themesInUse = [];

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

foreach ($registeredThemes as $registeredTheme) {
if (array_key_exists($registeredTheme->getThemeId(), $storesByThemes)) {
Copy link
Contributor

@sidolov sidolov Nov 21, 2017

Choose a reason for hiding this comment

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

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.

$themesInUse[] = $registeredTheme;
}
}

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

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

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


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

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

Is there a way to avoid hardcoding the 4?

Copy link
Contributor Author

@denisristic denisristic Oct 18, 2017

Choose a reason for hiding this comment

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

fixed


$productSql = $connection
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov Oct 18, 2017

Choose a reason for hiding this comment

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

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

->select()
->from(
['eav' => $connection->getTableName('catalog_product_entity_varchar')],
['value']
)
->where('eav.attribute_id = ?', $productCustomDesignAttributeId)
->where('eav.value > 0')
->group('value');

$productThemeIds = $connection->fetchCol($productSql);

if (count($productThemeIds)) {
foreach ($productThemeIds as $productThemeId) {
if (array_key_exists($productThemeId, $storesByThemes)
&& !array_key_exists($productThemeId, $themesInUse) ) {
$themesInUse[] = $this->themeCollection->load($productThemeId);
}
}
}

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

$categorySql = $connection
->select()
->from(
['eav' => $connection->getTableName('catalog_category_entity_varchar')],
['value']
)
->where('eav.attribute_id = ?', $categoryCustomDesignAttributeId)
->where('eav.value > 0')
->group('value');

$categoryThemeIds = $connection->fetchCol($categorySql);

if (count($categoryThemeIds)) {
foreach ($categoryThemeIds as $categoryThemeId) {
if (array_key_exists($categoryThemeId, $storesByThemes)
&& !array_key_exists($categoryThemeId, $themesInUse) ) {
$themesInUse[] = $this->themeCollection->load($categoryThemeId);
}
}
}

$pageSql = $connection
->select()
->from(
['page' => $connection->getTableName('cms_page')],
['custom_theme']
)
->where('custom_theme > 0')
->group('custom_theme');

$pageThemeIds = $connection->fetchCol($pageSql);

if (count($pageThemeIds)) {
foreach ($pageThemeIds as $pageThemeId) {
if (array_key_exists($pageThemeId, $storesByThemes)
&& !array_key_exists($pageThemeId, $themesInUse) ) {
$themesInUse[] = $this->themeCollection->load($pageThemeId);
}
}
}

return $themesInUse;
}
}
87 changes: 87 additions & 0 deletions app/code/Magento/Catalog/Test/Unit/Model/Product/Image/CacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use Magento\Framework\App\Area;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use Magento\Framework\ObjectManagerInterface;

/**
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
Expand Down Expand Up @@ -43,6 +44,44 @@ class CacheTest extends \PHPUnit\Framework\TestCase
*/
protected $themeCollection;

/**
* @var \Magento\Theme\Model\Config\Customization|\PHPUnit_Framework_MockObject_MockObject
*/
protected $themeCustomizationConfig;

/**
* @var \Magento\Eav\Model\Config|\PHPUnit_Framework_MockObject_MockObject
*/
protected $eavConfig;

/**
* @var \Magento\Eav\Model\Entity\Attribute|\PHPUnit_Framework_MockObject_MockObject
*/
protected $attribute;

/**
* @var \Magento\Framework\DB\Select|\PHPUnit_Framework_MockObject_MockObject
*/
protected $select;

/** @var \Magento\Framework\DB\Adapter\AdapterInterface|\PHPUnit_Framework_MockObject_MockObject */
protected $adapter;

/**
* @var \Magento\Framework\DB\Adapter\Pdo\Mysql|\PHPUnit_Framework_MockObject_MockObject
*/
protected $connection;

/**
* @var \Magento\Framework\Model\ResourceModel\Db\AbstractDb|\PHPUnit_Framework_MockObject_MockObject
*/
protected $resource;

/**
* @var \Psr\Log\LoggerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $loggerMock;

/**
* @var \Magento\Catalog\Helper\Image|\PHPUnit_Framework_MockObject_MockObject
*/
Expand Down Expand Up @@ -74,6 +113,47 @@ protected function setUp()
->disableOriginalConstructor()
->getMock();

$this->themeCustomizationConfig = $this->getMockBuilder(\Magento\Theme\Model\Config\Customization::class)
->disableOriginalConstructor()
->getMock();

$this->eavConfig = $this->getMockBuilder(\Magento\Eav\Model\Config::class)
->disableOriginalConstructor()
->getMock();

$this->loggerMock = $this->createMock(\Psr\Log\LoggerInterface::class);

$this->select = $this->getMockBuilder(\Magento\Framework\DB\Select::class)
->disableOriginalConstructor()
->getMock();
$this->select->expects($this->any())->method('from')->will($this->returnValue($this->select));
$this->select->expects($this->any())->method('where')->will($this->returnValue($this->select));

$this->adapter = $this->getMockBuilder(\Magento\Framework\DB\Adapter\AdapterInterface::class)
->disableOriginalConstructor()
->setMethods([])
->getMock();
$this->adapter->expects($this->any())->method('fetchRow')->willReturn([1]);

$this->connection = $this->getMockBuilder(\Magento\Framework\DB\Adapter\Pdo\Mysql::class)
->disableOriginalConstructor()
->getMock();
$this->connection->expects($this->any())->method('select')->willReturn($this->select);

$this->resource = $this->getMockBuilder(\Magento\Framework\Model\ResourceModel\Db\AbstractDb::class)
->disableOriginalConstructor()
->setMethods(['getConnection', 'getTable'])
->getMockForAbstractClass();
$this->resource->expects($this->any())->method('getConnection')->willReturn($this->connection);
$this->resource->expects($this->any())->method('getTable')->willReturn('test');

$this->attribute = $this->getMockBuilder(\Magento\Eav\Model\Entity\Attribute::class)
->disableOriginalConstructor()
->setMethods(['getResource', 'loadByCode'])
->getMock();
$this->attribute->expects($this->any())->method('getResource')->willReturn($this->resource);
$this->attribute->expects($this->any())->method('loadByCode')->willReturnSelf();

$this->mediaGalleryCollection = $this->getMockBuilder(\Magento\Framework\Data\Collection::class)
->disableOriginalConstructor()
->getMock();
Expand All @@ -84,7 +164,10 @@ protected function setUp()
[
'viewConfig' => $this->viewConfig,
'themeCollection' => $this->themeCollection,
'themeCustomizationConfig' => $this->themeCustomizationConfig,
'imageHelper' => $this->imageHelper,
'config' => $this->eavConfig,
'attribute' => $this->attribute
]
);
}
Expand Down Expand Up @@ -126,6 +209,10 @@ public function testGenerate()
->method('loadRegisteredThemes')
->willReturn([$themeMock]);

$this->themeCustomizationConfig->expects($this->once())
->method('getStoresByThemes')
->willReturn([$themeMock->getThemeId() => [1]]);

$this->viewConfig->expects($this->once())
->method('getViewConfig')
->with([
Expand Down