Skip to content
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.

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

Allowed to upload images in WebP format #3384

Merged
merged 21 commits into from
Jul 19, 2023
Merged

Allowed to upload images in WebP format #3384

merged 21 commits into from
Jul 19, 2023

Conversation

empiricompany
Copy link
Contributor

@empiricompany empiricompany commented Jul 16, 2023

Description (*)

Since the webp format is now supported by all modern browsers *, I have added the ability to upload images in webp format.

This PR allow to upload an image in webp format and add support to resize and cache it correctly in frontend.

PS: I know that there are extensions/techniques to dynamically generate the WebP version on the frontend, but based on the target audience, with this PR, we can decide to upload them directly in the correct format.

Manual testing scenarios (*)

Test all scenarios with an image in webp format

Tested scenarios:

  • Product images
  • Product Custom Options Input Type: File
    immagine
  • Category Image and Thumbnail Image
  • CMS WYSIWYG Media Storage

Untested scenarios:

  • Import/export
  • API

I create this pull request as a draft since there are areas that I have not yet tested.
Tested on PHP 7.4.29 with GD 2.1.0 (support to webp should be enabled by default)

@github-actions github-actions bot added Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: lib/Varien Relates to lib/Varien Component: Eav Relates to Mage_Eav Component: Adminhtml Relates to Mage_Adminhtml Component: Media Relates to Mage_Media Component: ImportExport Relates to Mage_ImportExport Component: Uploader Relates to Mage_Uploader Component: lib/* Relates to lib/* htaccess labels Jul 16, 2023
@empiricompany
Copy link
Contributor Author

empiricompany commented Jul 16, 2023

@fballiano can you rebase this PR since this commit?
first draft webp support catalog media product and cms

Sorry if I always make a huge mess 😅 but what matters is the substance

@fballiano
Copy link
Contributor

the PR looks ok, I don't think there's anything to rebase :-)

@fballiano
Copy link
Contributor

IMHO it would be great to have a single

    /**
     * @var string[]
     */
    public const ALLOWED_IMAGES_EXTENSIONS = ['webp', 'jpg', 'jpeg', 'png', 'gif', 'bmp'];

instead of many arrays throughout the code, all of the code should be referencing to that constant :-)

@empiricompany
Copy link
Contributor Author

IMHO it would be great to have a single

    /**
     * @var string[]
     */
    public const ALLOWED_IMAGES_EXTENSIONS = ['webp', 'jpg', 'jpeg', 'png', 'gif', 'bmp'];

instead of many arrays throughout the code, all of the code should be referencing to that constant :-)

yes but it was keep untouched for backward compatibility reasons
maybe we can create new constant and return it keeping in the deprecated methods?

@fballiano
Copy link
Contributor

the one that I posted is a new one ;-)

@ADDISON74
Copy link
Contributor

Personally, I would not put webp as the first image format in the list that is seen in the Backend. I would list it at the end, because it is a format after all the ones that have been so far.

@empiricompany
Copy link
Contributor Author

Personally, I would not put webp as the first image format in the list that is seen in the Backend. I would list it at the end, because it is a format after all the ones that have been so far.

sorry i can't figure out where is showed

@fballiano
Copy link
Contributor

he says only in the array list

@empiricompany
Copy link
Contributor Author

Personally, I would not put webp as the first image format in the list that is seen in the Backend. I would list it at the end, because it is a format after all the ones that have been so far.

done

@empiricompany
Copy link
Contributor Author

please test importexport and api scenarios

@fballiano
Copy link
Contributor

I've unified all into a single constant, see if you like it :-)

@empiricompany
Copy link
Contributor Author

I've unified all into a single constant, see if you like it :-)

perfect

@fballiano fballiano marked this pull request as ready for review July 18, 2023 18:39
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

seems to work fine to me!

@fballiano fballiano changed the title Allow to upload images in Webp format Allowed to upload images in WebP format Jul 18, 2023
@ADDISON74
Copy link
Contributor

So far so good.

I would like to test "Image type product options" mentioned in the description, but I don't know anything about this feature. Any help?

@fballiano
Copy link
Contributor

not sure but probably you have to create a new product attribute of type image:

Screenshot 2023-07-18 alle 21 26 14

@ADDISON74
Copy link
Contributor

That type of attribute adds a new column in the Images section. I asked because he made me curious about what it could be and I don't already know.

screenshot

@fballiano
Copy link
Contributor

ahhhh ok then I don't know 😅

@empiricompany
Copy link
Contributor Author

empiricompany commented Jul 19, 2023

I would like to test "Image type product options" mentioned in the description, but I don't know anything about this feature. Any help?

@ADDISON74 you have to create a product custom options of type image that allow customers to upload an image in the frontend

immagine

@ADDISON74
Copy link
Contributor

Now I understand what it is about. It should have been mentioned as "File type in custom options" . A word led to misunderstanding.

Copy link
Contributor

@ADDISON74 ADDISON74 left a comment

Choose a reason for hiding this comment

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

I tested this PR and encountered no issues. It is a new feature in OpenMage much discussed in the forums of the time.

@empiricompany
Copy link
Contributor Author

Now I understand what it is about. It should have been mentioned as "File type in custom options" . A word led to misunderstanding.

yes sorry i've updated PR description

@fballiano
Copy link
Contributor

great, let's merge it, it's a nice little new addition for the upcoming release :-)

@fballiano fballiano merged commit 0cb5439 into OpenMage:main Jul 19, 2023
15 checks passed
@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 19, 2023

[Only if it is confirmed]
I would add a warning in the README file that issues may arise with Magmi. Here is a report dweeves/magmi-git#596.

[Photoshop Users]
WebP has support for Photoshop native (since version 23.2) and based on a module https://github.com/webmproject/WebPShop. I used both without any issues.

@ADDISON74 ADDISON74 removed their assignment Jul 19, 2023
@empiricompany
Copy link
Contributor Author

I would add a warning in the README file that issues may arise with Magmi. Here is a report dweeves/magmi-git#596.

Maybe now it works, we need to try

@ADDISON74
Copy link
Contributor

First we have to do a test for the Import feature we already have in OpenMage and confirm that it works (CSV file, a few images uploaded, ...). I will check these days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Eav Relates to Mage_Eav Component: ImportExport Relates to Mage_ImportExport Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Media Relates to Mage_Media Component: Uploader Relates to Mage_Uploader htaccess
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants