34858: Adjust validation for upload ICO files #34883

mmezhensky · 2021-12-24T12:09:03Z

Description (*)

This PR adjusted validation for images files in ICO format.

Fixed Issues (if relevant)

Fixes Favicon Icon upload form in Magento does not actually support .ico file types #34858

Manual testing scenarios (*)

Login to Magento admin
Navigate to Content -> Design -> Configuration
Click 'Edit' on any of the records shown in the grid
Scroll to 'Other Settings'
Expand 'HTML Head'
Under the 'Favicon Icon' section, click 'Upload'
Select a valid .ico file, click 'Open'

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)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

m2-assistant · 2021-12-24T12:09:05Z

Hi @mmezhensky. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

mmezhensky · 2021-12-24T12:33:41Z

@magento run all tests

magento-automated-testing · 2021-12-24T12:33:49Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

ihor-sviziev · 2021-12-24T13:04:12Z

app/code/Magento/MediaStorage/Model/File/Validator/Image.php

+                    $imagick = new \Imagick();
+                    $imagick->setFormat('ICO');
+                    $image = $imagick->readImageBlob($this->file->fileGetContents($filePath));


I don't think it's a good option because the Imagick extension is currently optional, and by default, Magento uses lib gd.
Can it handle this case?

Hi @ihor-sviziev
Lib GD does not support the ICO format and error returns.
After a long search for a solution, I think Imagick is the most suitable option, or we will have to install a third-party extension.

@mmezhensky I'm OK with using imagick when it's available. But when it's not?

@ihor-sviziev Imagick is already used in Magento.

Yes, but only as optional dependence

Hi @Den4ik
Thanks for your information, but I tried to do it, and I always get an error for ICO files because GD2 does not support this format, and Magento\Framework\Image\Adapter\ImageMagick has no function for setting ICO format.

To use Magento\Framework\Image\Factory, we always need to pass $adapterName = "IMAGEMAGICK" and extend Magento\Framework\Image\Adapter\ImageMagick class to add the ability to install the form.
But I think it will be a significant change in the Magento Framework that may cause problems in the future, so I suggest using the fix option I suggested earlier.

СС @ihor-sviziev

Hi @mmezhensky. Thanks for your work.

we always need to pass $adapterName = "IMAGEMAGICK"

It's not quite true. AdapterFactory retrieve current selected adapter from stores configuration. And my mind is that we receive exception if imagick is not selected as adapter at Stores config. It's guarant that user/administrator that select imagick as adapter will be sure that it's installed for php.

@Den4ik
Yes, but even if imagick is installed in the configuration and it is installed on the server, we will still get an exception because we need to set ICO format for the imagick object (but we can't do it without changes in Magento\Framework\Image\Adapter\ImageMagick).

@mmezhensky I agree that it require some changes for ImageMagick class, but I don't see any problems use something like that in open method

if (is_callable('exif_imagetype')) { $fileType = exif_imagetype($this->_fileName); if ($fileType === IMAGETYPE_ICO) { $filename = 'ico:' . $filename; } } $this->_imageHandler = new \Imagick($filename);

It provide total support of ico files for image magick adapter
@ihor-sviziev @sidolov What do you think?

I prepared these changes.
Thx!

Den4ik · 2021-12-24T19:42:32Z

app/code/Magento/MediaStorage/Model/File/Validator/Image.php

+                    $imagick = new \Imagick();
+                    $imagick->setFormat('ICO');
+                    $image = $imagick->readImageBlob($this->file->fileGetContents($filePath));


@mmezhensky I'm agree with @ihor-sviziev and believe that we could use Magento\Framework\Image\Factory here because it will use default adapter selected in configs.
If call of open throw exception than image is not valid.

magento-engcom-team · 2022-02-03T22:08:36Z

Hi @Den4ik, thank you for the review.
ENGCOM-9397 has been created to process this Pull Request
✳️ @Den4ik, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

Den4ik · 2022-02-03T22:09:47Z

@mmezhensky Could you add unit tests for implemented changes?

engcom-Delta · 2022-02-09T07:50:44Z

❌ QA not Passed

Favicon Icon upload form does not support .ico file types in Magento 2.4 Develop.

Preconditions:

We should have fresh installation of Magento 2.4 Develop.
We should have .Ico files ready for uploading

Manual Scenario Steps

Log in to Magento admin
Navigate to Content -> Design -> Configuration
Click 'Edit' on any of the records shown in the grid
Scroll to 'Other Settings'
Expand 'HTML Head'
Under the 'Favicon Icon' section, click 'Upload'
Select a valid “.ico” extension file, click 'Open'
Validate the file upload results

✔️ Expected result After Fix

.Ico files should be uploaded successfully.

❌ Actual result After Fix

.Ico files are not getting uploaded, throwing the ''File validation Failed " popup

FYI- downloaded the ico files from link

engcom-Delta

Thanks for your contribution and collaboration @mmezhensky , @ihor-sviziev and @Den4ik
Based on the above comments moving to 'Request Changes' status

engcom-Lima · 2022-03-03T11:33:59Z

Hi @engcom-Delta,
Magento by default uses GD Library which does not support ico extension.
In order to verify the issue, the ImageMagick extension needs to be set in Stores->Configuration->Advanced->Developer->Image Processing Settings

Note: ImageMagic needs to be installed on server in order to set the ImagMagic option in magento.

Request you to kindly verify the issue after installing ImageMagic on server and setting it as the default adapter

Thanks!

engcom-Alfa · 2022-03-08T11:33:43Z

✔️ QA Passed

Preconditions:

Have ImageMagic installed in your system and extension enabled under server php.ini; REFER HERE
Have Magento latest installed.
Have some .ico files in the system to upload; here is - ico files downloadable link

Manual testing scenario:

Log in to Magento admin; Navigate to Content -> Design -> Configuration
Click 'Edit' on any of the records shown in the grid > Scroll to 'Other Settings' > Expand 'HTML Head'
Under the 'Favicon Icon' section, click 'Upload'; Select a valid “.ico” extension file, click 'Open'
Validate the file upload results

Before: ✖️ We were getting an error message "File Validation Failed".

After: ✔️ File is getting uploaded successfully.

Tried to save the uploaded ico file and getting it saved successfully.

m2-github-services added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Dec 24, 2021

magento-engcom-team added Component: MediaStorage Release Line: 2.4 labels Dec 24, 2021

m2-community-project bot added the Progress: pending review label Dec 24, 2021

ihor-sviziev requested changes Dec 24, 2021

View reviewed changes

m2-community-project bot assigned ihor-sviziev Dec 24, 2021

m2-community-project bot added Progress: needs update Progress: review and removed Progress: pending review Progress: needs update labels Dec 24, 2021

ihor-sviziev removed their assignment Dec 24, 2021

m2-community-project bot added Progress: pending review and removed Progress: review labels Dec 24, 2021

m2-community-project bot assigned ihor-sviziev Dec 24, 2021

m2-community-project bot added Progress: review and removed Progress: pending review labels Dec 24, 2021

ihor-sviziev removed their assignment Dec 24, 2021

m2-community-project bot added Progress: pending review and removed Progress: review labels Dec 24, 2021

Den4ik requested changes Dec 24, 2021

View reviewed changes

m2-community-project bot assigned Den4ik Dec 24, 2021

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Dec 24, 2021

m2-community-project bot added Progress: review and removed Progress: needs update labels Jan 3, 2022

m2-community-project bot added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jan 13, 2022

Den4ik added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Feb 3, 2022

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Feb 7, 2022

engcom-Delta self-assigned this Feb 7, 2022

m2-community-project bot added Progress: ready for testing and removed Progress: testing in progress labels Feb 7, 2022

m2-community-project bot added Progress: needs update Progress: testing in progress and removed Progress: ready for testing Progress: needs update labels Feb 9, 2022

engcom-Delta requested changes Feb 9, 2022

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: testing in progress labels Feb 9, 2022

engcom-Lima self-assigned this Feb 16, 2022

m2-community-project bot added Progress: ready for testing and removed Progress: needs update labels Mar 3, 2022

engcom-Alfa self-assigned this Mar 8, 2022

m2-community-project bot added Progress: needs update Progress: testing in progress and removed Progress: ready for testing labels Mar 8, 2022

m2-community-project bot added Progress: accept and removed Progress: needs update Progress: testing in progress labels Mar 8, 2022

magento-devops-reposync-svc merged commit b54be44 into magento:2.4-develop May 18, 2022

elidrissidev mentioned this pull request Jan 31, 2023

Cannot upload favicon in Windows ICO format (.ico) OpenMage/magento-lts#3001

Closed

34858: Adjust validation for upload ICO files #34883

34858: Adjust validation for upload ICO files #34883

Uh oh!

Conversation

mmezhensky commented Dec 24, 2021

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Dec 24, 2021

Uh oh!

mmezhensky commented Dec 24, 2021

Uh oh!

magento-automated-testing bot commented Dec 24, 2021

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!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Feb 3, 2022

Uh oh!

Den4ik commented Feb 3, 2022

Uh oh!

engcom-Delta commented Feb 9, 2022

Uh oh!

engcom-Delta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

engcom-Lima commented Mar 3, 2022

Uh oh!

engcom-Alfa commented Mar 8, 2022

Uh oh!

Uh oh!

engcom-Delta left a comment •

edited

Loading