Making configurable settings for MAX_IMAGE_WIDTH and MAX_IMAGE_HEIGHT #15942

eduard13 · 2018-06-07T16:47:50Z

Description

It was added a new system fieldset, where the max-width and max-height can be set.
Stores / Configuration / Advanced / System / Images Configuration

So these 2 values are more flexible now for those which need to have bigger images in their shops.

Fixed Issues (if relevant)

Wysiwyg > Image Uploader >Max width/height #13747: Wysiwyg > Image Uploader >Max width/height
...

Manual testing scenarios

Get an image of 3000x700
https://dummyimage.com/3000x700
Open any wysiwyg editor
Upload an image
The uploaded image should have maximum configured size

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)

rogyar · 2018-06-11T18:52:30Z

Hello @eduard13, thank you for your PR. Please, consider fixing the failed static tests

…ce to a public type.

eduard13 · 2018-06-15T10:35:20Z

Hi @rogyar, the failed tests were fixed.
Thank you.

magento-engcom-team · 2018-06-20T14:27:43Z

@eduard13 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

rogyar · 2018-06-20T14:58:07Z

@eduard13 great job!
I've added some minor changes. Among them, I've reverted the max width from 3000 to the 1920 as we had previously to prevent unexpected issues on the live installations. So, if there's a necessity to increase the limits for uploading images, an admin has this possibility.

orlangur · 2018-06-20T20:32:40Z

app/code/Magento/Backend/Block/Media/Uploader.php

+    /**
+     * @var \Magento\Framework\Image\Adapter\ConfigInterface
+     */
+    protected $imageConfig;


Always use private for new fields.

orlangur · 2018-06-20T20:35:21Z

app/code/Magento/Backend/Block/Media/Uploader.php

        array $data = []
    ) {
        $this->_fileSizeService = $fileSize;
+        $this->imageConfig = $imageConfig;
        parent::__construct($context, $data);


Please check https://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#php on how to add new dependency properly, always try to make parent constructor called first.

orlangur · 2018-06-20T20:37:56Z

app/code/Magento/Backend/Block/Media/Uploader.php

+     */
+    public function getImageConfigService()
+    {
+        return $this->imageConfig;


Such method must be eliminated as it violates Law of Demeter, inject needed classes into blocks directly and introduce methods like getImageMaxWidth.

orlangur · 2018-06-20T20:38:16Z

app/code/Magento/MediaStorage/etc/adminhtml/system.xml

+            <group id="media_configuration" translate="label" type="text" sortOrder="1000" showInDefault="1" showInWebsite="1" showInStore="1">
+                <label>Images Upload</label>
+                <field id="max_width" translate="label comment" type="text" sortOrder="100" showInDefault="1" showInWebsite="0" showInStore="0" canRestore="1">
+                    <label>Max Width</label>


Max -> Maximum

orlangur · 2018-06-20T20:38:26Z

app/code/Magento/MediaStorage/etc/adminhtml/system.xml

+                <label>Images Upload</label>
+                <field id="max_width" translate="label comment" type="text" sortOrder="100" showInDefault="1" showInWebsite="0" showInStore="0" canRestore="1">
+                    <label>Max Width</label>
+                    <validate>validate-zero-or-greater validate-digits</validate>


I don't think zero should be allowed. Validate integer instead of digits.

orlangur · 2018-06-20T20:39:27Z

app/code/Magento/MediaStorage/etc/adminhtml/system.xml

+                <field id="max_height" translate="label comment" type="text" sortOrder="200" showInDefault="1" showInWebsite="0" showInStore="0" canRestore="1">
+                    <label>Max Height</label>
+                    <validate>validate-zero-or-greater validate-digits</validate>
+                    <comment>Max allowed height for uploaded image.</comment>


Same three notes as for width should be applied here.

orlangur · 2018-06-20T20:40:44Z

lib/internal/Magento/Framework/Image/Adapter/Config.php

@@ -43,4 +47,24 @@ public function getAdapters()
    {
        return $this->config->getValue(self::XML_PATH_IMAGE_ADAPTERS);
    }
+
+    /**
+     * Get Max Image Width resolution in pixels. For image resizing on client side


Max -> Maximum

orlangur · 2018-06-20T20:40:49Z

lib/internal/Magento/Framework/Image/Adapter/Config.php

+    }
+
+    /**
+     * Get Max Image Height resolution in pixels. For image resizing on client side


Max -> Maximum

orlangur · 2018-06-20T20:41:23Z

lib/internal/Magento/Framework/File/Uploader.php

-    /**
-     * Max Image Height resolution in pixels. For image resizing on client side
-     */
-    const MAX_IMAGE_HEIGHT = 1200;


Constants cannot be removed, see http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/. Just deprecate them adding @see section.

orlangur · 2018-06-20T20:44:10Z

app/code/Magento/MediaStorage/etc/config.xml

+<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:module:Magento_Store:etc/config.xsd">
+    <default>
+        <system>
+            <media_configuration>


This seems to be uploader_configuration, isn't it?

Currently these changes look flawed from modularity perspective. Configuration added to MediaStorage module but no logic in it, some code added to framework. Please arrange your changes properly.

…13747

# Conflicts: # lib/internal/Magento/Framework/File/Uploader.php

rogyar · 2018-08-09T09:41:36Z

Please, resolve conflicts with the mainline branch

# Conflicts: # app/code/Magento/Backend/Block/Media/Uploader.php

…required one

eduard13 · 2018-08-12T08:00:16Z

@rogyar the conflicts were fixed. Moreover, I've changed the construct dependency into an optional one. Thank you.

sidolov · 2018-08-15T08:55:34Z

lib/internal/Magento/Framework/Image/Adapter/ConfigInterface.php

+    /**
+     * @return int
+     */
+    public function getMaxWidth();


We can't add new methods to existing interfaces, it would be better to introduce a new interface and implement this methods in Magento/Framework/Image/Adapter/Config.php

@sidolov a new interface was added and implemented. Thank you.

sidolov

Great! Thank you! Just a few minor changes are required

sidolov · 2018-08-15T09:23:39Z

lib/internal/Magento/Framework/Image/Test/Unit/AdapterFactoryTest.php

@@ -18,7 +18,7 @@ protected function setUp()
    {
        $this->configMock = $this->createPartialMock(
            \Magento\Framework\Image\Adapter\ConfigInterface::class,
-            ['getAdapterAlias', 'getAdapters']
+            ['getAdapterAlias', 'getAdapters', 'getMaxWidth', 'getMaxHeight']


Please, remove redundant method mocks

sidolov · 2018-08-15T09:24:38Z

lib/internal/Magento/Framework/File/Uploader.php

-     * Max Image Height resolution in pixels. For image resizing on client side
+     * Maximum Image Height resolution in pixels. For image resizing on client side
+     * @deprecated
+     * @see ConfigInterface::getMaxHeight()


I guess UploadConfigInterface should be here instead of ConfigInterface

…stants

…_IMAGE_HEIGHT #15942

magento-engcom-team · 2018-08-16T08:01:06Z

Hi @eduard13. Thank you for your contribution.
We will aim to release these changes as part of 2.2.7.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

Making configurable settings for MAX_IMAGE_WIDTH and MAX_IMAGE_HEIGHT

933e496

eduard13 requested a review from rogyar June 7, 2018 16:50

eduard13 assigned rogyar Jun 7, 2018

Added new methods to ConfigInterface Mock

a579bff

eduard13 added 2 commits June 11, 2018 23:15

Removed unused properties and fixed the dependencies' order

4ecce14

Declared strict_types. Fixed the params order and changed the referen…

c51b1d4

…ce to a public type.

eduard13 mentioned this pull request Jun 17, 2018

[Backport] 2.1 develop PR port 15942 #16200

Closed

4 tasks

magento-engcom-team added this to the Release: 2.2.6 milestone Jun 20, 2018

magento-engcom-team added Release Line: 2.2 Area: Frontend labels Jun 20, 2018

rogyar added 2 commits June 20, 2018 16:45

Updated default configuration to match the previous values

1d5e56b

Added comments for settings

f7b4a66

eduard13 mentioned this pull request Jun 20, 2018

[Forwardport] 2.3 develop PR port 15942 #16277

Closed

4 tasks

orlangur self-assigned this Jun 20, 2018

orlangur requested changes Jun 20, 2018

View reviewed changes

orlangur added the Progress: needs update label Jun 20, 2018

Merge remote-tracking branch 'upstream/2.2-develop' into 2.2-develop-…

e1591e4

…13747

eduard13 closed this Jul 8, 2018

eduard13 reopened this Jul 8, 2018

magento-engcom-team removed the Progress: needs update label Jul 13, 2018

eduard13 and others added 2 commits July 25, 2018 22:06

Merge branch '2.2-develop' into 2.2-develop-13747

7b4b32f

# Conflicts: # lib/internal/Magento/Framework/File/Uploader.php

Minor naming adjustments

0f63017

rogyar approved these changes Aug 5, 2018

View reviewed changes

magento-engcom-team added the Progress: accept label Aug 5, 2018

rogyar added the Progress: needs update label Aug 9, 2018

eduard13 added 2 commits August 12, 2018 10:12

Merge branch '2.2-develop' into 2.2-develop-13747

128724f

# Conflicts: # app/code/Magento/Backend/Block/Media/Uploader.php

Refactoring the construct to have an optionally dependency instead a …

30822ef

…required one

Removed extra semicolon

2e69236

magento-engcom-team modified the milestones: Release: 2.2.6, Release: 2.2.7 Aug 13, 2018

magento-engcom-team added Area: Frontend and removed Component: MediaStorage Progress: needs update labels Aug 13, 2018

sidolov requested changes Aug 15, 2018

View reviewed changes

Adding a new interface and implementing it

614570f

sidolov requested changes Aug 15, 2018

View reviewed changes

Adjusting the Unit Test and changing the interface for deprecated con…

507d183

…stants

magento-engcom-team assigned sidolov Aug 15, 2018

magento-engcom-team added the Area: Frontend label Aug 15, 2018

magento-engcom-team mentioned this pull request Aug 16, 2018

Wysiwyg > Image Uploader >Max width/height #13747

Closed

magento-engcom-team merged commit 507d183 into magento:2.2-develop Aug 16, 2018

magento-engcom-team pushed a commit that referenced this pull request Aug 16, 2018

ENGCOM-2070: Making configurable settings for MAX_IMAGE_WIDTH and MAX…

596a8e6

…_IMAGE_HEIGHT #15942

magento-engcom-team added the Award: special achievement label Aug 16, 2018

nmalevanec mentioned this pull request Aug 28, 2018

[Forwardport] Making configurable settings for MAX_IMAGE_WIDTH and MAX_IMAGE_HEIGHT #17826

Merged

4 tasks

Making configurable settings for MAX_IMAGE_WIDTH and MAX_IMAGE_HEIGHT #15942

Making configurable settings for MAX_IMAGE_WIDTH and MAX_IMAGE_HEIGHT #15942

Uh oh!

Conversation

eduard13 commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

rogyar commented Jun 11, 2018

Uh oh!

eduard13 commented Jun 15, 2018

Uh oh!

magento-engcom-team commented Jun 20, 2018

Uh oh!

rogyar commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogyar commented Aug 9, 2018

Uh oh!

eduard13 commented Aug 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidolov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Aug 16, 2018

Uh oh!

Uh oh!

eduard13 commented Jun 7, 2018 •

edited

Loading