Correct image params - allow to disable image frame, fix image params hash

unicoder88 · unicoder88 · commit 6496f4e2bff8 · 2019-10-04T21:27:27.000+03:00
diff --git a/app/code/Magento/Catalog/Helper/Image.php b/app/code/Magento/Catalog/Helper/Image.php
@@ -213,31 +213,29 @@ protected function setImageProperties()
 
         // Set 'keep frame' flag
         $frame = $this->getFrame();
-        if (!empty($frame)) {
-            $this->_getModel()->setKeepFrame($frame);
-        }
+        $this->_getModel()->setKeepFrame($frame);
 
         // Set 'constrain only' flag
         $constrain = $this->getAttribute('constrain');
-        if (!empty($constrain)) {
+        if (null !== $constrain) {
             $this->_getModel()->setConstrainOnly($constrain);
         }
 
         // Set 'keep aspect ratio' flag
         $aspectRatio = $this->getAttribute('aspect_ratio');
-        if (!empty($aspectRatio)) {
+        if (null !== $aspectRatio) {
             $this->_getModel()->setKeepAspectRatio($aspectRatio);
         }
 
         // Set 'transparency' flag
         $transparency = $this->getAttribute('transparency');
-        if (!empty($transparency)) {
+        if (null !== $transparency) {
             $this->_getModel()->setKeepTransparency($transparency);
         }
 
         // Set background color
         $background = $this->getAttribute('background');
-        if (!empty($background)) {
+        if (null !== $background) {
             $this->_getModel()->setBackgroundColor($background);
         }
 
diff --git a/app/code/Magento/Catalog/Model/View/Asset/Image.php b/app/code/Magento/Catalog/Model/View/Asset/Image.php
@@ -200,11 +200,11 @@ private function convertToReadableFormat($miscParams)
         $miscParams['image_width'] = 'w:' . ($miscParams['image_width'] ?? 'empty');
         $miscParams['quality'] = 'q:' . ($miscParams['quality'] ?? 'empty');
         $miscParams['angle'] = 'r:' . ($miscParams['angle'] ?? 'empty');
-        $miscParams['keep_aspect_ratio'] = (isset($miscParams['keep_aspect_ratio']) ? '' : 'non') . 'proportional';
-        $miscParams['keep_frame'] = (isset($miscParams['keep_frame']) ? '' : 'no') . 'frame';
-        $miscParams['keep_transparency'] = (isset($miscParams['keep_transparency']) ? '' : 'no') . 'transparency';
-        $miscParams['constrain_only'] = (isset($miscParams['constrain_only']) ? 'do' : 'not') . 'constrainonly';
-        $miscParams['background'] = isset($miscParams['background'])
+        $miscParams['keep_aspect_ratio'] = (!empty($miscParams['keep_aspect_ratio']) ? '' : 'non') . 'proportional';
+        $miscParams['keep_frame'] = (!empty($miscParams['keep_frame']) ? '' : 'no') . 'frame';
+        $miscParams['keep_transparency'] = (!empty($miscParams['keep_transparency']) ? '' : 'no') . 'transparency';
+        $miscParams['constrain_only'] = (!empty($miscParams['constrain_only']) ? 'do' : 'not') . 'constrainonly';
+        $miscParams['background'] = !empty($miscParams['background'])
             ? 'rgb' . implode(',', $miscParams['background'])
             : 'nobackground';
         return $miscParams;
diff --git a/app/code/Magento/Catalog/Test/Unit/Helper/ImageTest.php b/app/code/Magento/Catalog/Test/Unit/Helper/ImageTest.php
@@ -29,6 +29,11 @@ class ImageTest extends \PHPUnit\Framework\TestCase
      */
     protected $assetRepository;
 
+    /**
+     * @var \Magento\Framework\Config\View|\PHPUnit\Framework\MockObject\MockObject
+     */
+    protected $configView;
+
     /**
      * @var \Magento\Framework\View\ConfigInterface|\PHPUnit_Framework_MockObject_MockObject
      */
@@ -58,6 +63,10 @@ protected function setUp()
             ->disableOriginalConstructor()
             ->getMock();
 
+        $this->configView = $this->getMockBuilder(\Magento\Framework\Config\View::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
         $this->viewConfig = $this->getMockBuilder(\Magento\Framework\View\ConfigInterface::class)
             ->getMockForAbstractClass();
 
@@ -151,23 +160,89 @@ public function initDataProvider()
         ];
     }
 
+    /**
+     * @param array $data - optional 'frame' key
+     * @param bool $whiteBorders view config
+     * @param bool $expectedKeepFrame
+     * @dataProvider initKeepFrameDataProvider
+     */
+    public function testInitKeepFrame($data, $whiteBorders, $expectedKeepFrame)
+    {
+        $imageId = 'test_image_id';
+        $attributes = [];
+
+        $productMock = $this->getMockBuilder(\Magento\Catalog\Model\Product::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $this->prepareAttributes($data, $imageId);
+
+        $this->configView->expects(isset($data['frame']) ? $this->never() : $this->once())
+            ->method('getVarValue')
+            ->with('Magento_Catalog', 'product_image_white_borders')
+            ->willReturn($whiteBorders);
+
+        $this->viewConfig->expects($this->once())
+            ->method('getViewConfig')
+            ->willReturn($this->configView);
+
+        $this->image->expects($this->once())
+            ->method('setKeepFrame')
+            ->with($expectedKeepFrame)
+            ->willReturnSelf();
+
+        $this->helper->init($productMock, $imageId, $attributes);
+    }
+
+    /**
+     * @return array
+     */
+    public function initKeepFrameDataProvider()
+    {
+        return [
+            // when frame defined explicitly, it wins
+            [
+                'mediaImage' => [
+                    'frame' => 1,
+                ],
+                'whiteBorders' => true,
+                'expected' => true,
+            ],
+            [
+                'mediaImage' => [
+                    'frame' => 0,
+                ],
+                'whiteBorders' => true,
+                'expected' => false,
+            ],
+            // when frame is not defined, var is used
+            [
+                'mediaImage' => [],
+                'whiteBorders' => true,
+                'expected' => true,
+            ],
+            [
+                'mediaImage' => [],
+                'whiteBorders' => false,
+                'expected' => false,
+            ],
+        ];
+    }
+
     /**
      * @param $data
      * @param $imageId
      */
     protected function prepareAttributes($data, $imageId)
     {
-        $configViewMock = $this->getMockBuilder(\Magento\Framework\Config\View::class)
-            ->disableOriginalConstructor()
-            ->getMock();
-        $configViewMock->expects($this->once())
+        $this->configView->expects($this->once())
             ->method('getMediaAttributes')
             ->with('Magento_Catalog', Image::MEDIA_TYPE_CONFIG_NODE, $imageId)
             ->willReturn($data);
 
         $this->viewConfig->expects($this->once())
             ->method('getViewConfig')
-            ->willReturn($configViewMock);
+            ->willReturn($this->configView);
     }
 
     /**
diff --git a/app/code/Magento/Catalog/Test/Unit/Model/View/Asset/ImageTest.php b/app/code/Magento/Catalog/Test/Unit/Model/View/Asset/ImageTest.php
@@ -7,6 +7,7 @@
 
 use Magento\Catalog\Model\Product\Media\ConfigInterface;
 use Magento\Catalog\Model\View\Asset\Image;
+use Magento\Framework\Encryption\Encryptor;
 use Magento\Framework\Encryption\EncryptorInterface;
 use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
 use Magento\Framework\View\Asset\ContextInterface;
@@ -103,9 +104,10 @@ public function testGetContext()
     /**
      * @param string $filePath
      * @param array $miscParams
+     * @param string $readableParams
      * @dataProvider getPathDataProvider
      */
-    public function testGetPath($filePath, $miscParams)
+    public function testGetPath($filePath, $miscParams, $readableParams)
     {
         $imageModel = $this->objectManager->getObject(
             Image::class,
@@ -118,11 +120,13 @@ public function testGetPath($filePath, $miscParams)
                 'miscParams' => $miscParams
             ]
         );
-        $miscParams['background'] = isset($miscParams['background']) ? implode(',', $miscParams['background']) : '';
         $absolutePath = '/var/www/html/magento2ce/pub/media/catalog/product';
-        $hashPath = md5(implode('_', $miscParams));
+        $hashPath = 'somehash';
         $this->context->method('getPath')->willReturn($absolutePath);
-        $this->encryptor->method('hash')->willReturn($hashPath);
+        $this->encryptor->expects(static::once())
+            ->method('hash')
+            ->with($readableParams, $this->anything())
+            ->willReturn($hashPath);
         static::assertEquals(
             $absolutePath . '/cache/'. $hashPath . $filePath,
             $imageModel->getPath()
@@ -132,9 +136,10 @@ public function testGetPath($filePath, $miscParams)
     /**
      * @param string $filePath
      * @param array $miscParams
+     * @param string $readableParams
      * @dataProvider getPathDataProvider
      */
-    public function testGetUrl($filePath, $miscParams)
+    public function testGetUrl($filePath, $miscParams, $readableParams)
     {
         $imageModel = $this->objectManager->getObject(
             Image::class,
@@ -147,11 +152,13 @@ public function testGetUrl($filePath, $miscParams)
                 'miscParams' => $miscParams
             ]
         );
-        $miscParams['background'] = isset($miscParams['background']) ? implode(',', $miscParams['background']) : '';
         $absolutePath = 'http://localhost/pub/media/catalog/product';
-        $hashPath = md5(implode('_', $miscParams));
+        $hashPath = 'somehash';
         $this->context->expects(static::once())->method('getBaseUrl')->willReturn($absolutePath);
-        $this->encryptor->expects(static::once())->method('hash')->willReturn($hashPath);
+        $this->encryptor->expects(static::once())
+            ->method('hash')
+            ->with($readableParams, $this->anything())
+            ->willReturn($hashPath);
         static::assertEquals(
             $absolutePath . '/cache/' . $hashPath . $filePath,
             $imageModel->getUrl()
@@ -166,23 +173,41 @@ public function getPathDataProvider()
         return [
             [
                 '/some_file.png',
-                [], //default value for miscParams
+                [], //default value for miscParams,
+                'h:empty_w:empty_q:empty_r:empty_nonproportional_noframe_notransparency_notconstrainonly_nobackground',
             ],
             [
                 '/some_file_2.png',
                 [
                     'image_type' => 'thumbnail',
                     'image_height' => 75,
                     'image_width' => 75,
-                    'keep_aspect_ratio' => 'proportional',
-                    'keep_frame' => 'frame',
-                    'keep_transparency' => 'transparency',
-                    'constrain_only' => 'doconstrainonly',
+                    'keep_aspect_ratio' => true,
+                    'keep_frame' => true,
+                    'keep_transparency' => true,
+                    'constrain_only' => true,
                     'background' => [233,1,0],
                     'angle' => null,
                     'quality' => 80,
                 ],
-            ]
+                'h:75_w:75_proportional_frame_transparency_doconstrainonly_rgb233,1,0_r:empty_q:80',
+            ],
+            [
+                '/some_file_3.png',
+                [
+                    'image_type' => 'thumbnail',
+                    'image_height' => 75,
+                    'image_width' => 75,
+                    'keep_aspect_ratio' => false,
+                    'keep_frame' => false,
+                    'keep_transparency' => false,
+                    'constrain_only' => false,
+                    'background' => [233,1,0],
+                    'angle' => 90,
+                    'quality' => 80,
+                ],
+                'h:75_w:75_nonproportional_noframe_notransparency_notconstrainonly_rgb233,1,0_r:90_q:80',
+            ],
         ];
     }
 }
