Fix #303: native WebP and AVIF support for image upload and processing - #317
Fix #303: native WebP and AVIF support for image upload and processing#317jeanmarcos-dev wants to merge 13 commits into
Conversation
|
Initial thoughts:
Setting aside the question of whether it's clean and proper in the big scheme, the solution I would like to see is adding the ability to upload WebP images with minimal changes to make it happen. Adding its mime type and extension to lists, adding resize + parsing support, etc. Open to discussing it further. |
Thanks, this is fair feedback and I agree the PR is doing too much. Let me split it into three, ordered so the part you actually asked for lands on its own:
Happy to reduce this PR down to scope (1) with a force-push so the discussion and the issue link stay in one place, or close it and open a fresh one — whichever you prefer. |
|
Force push is fine -- thanks |
f096e57 to
e390eb3
Compare
This PR is now scope (1) only: WebP and AVIF as input formats. |
|
Tentatively, this looks good to me now. Still need to QA and check for completeness. Thank you @jeanmarcos-dev |
|
Please check:
Worth a second look on the deliberate exclusions Favicon and the PDF logo were excluded, correctly. But the email/HTML logo (Email\Model\Design\Backend\Logo, Config\Backend\Image\Logo) now accepts WebP/AVIF, and Outlook desktop and several mail clients render neither. That's the same class of problem the favicon exclusion is guarding against — either exclude it too or put a warning in the design_config_form notice. Verified as already covered nginx.conf.sample and pub/{media,static}/.htaccess already ship webp/avif upstream; Filesystem\Driver\File\Mime (native mime_content_type); WYSIWYG and the standalone Media Gallery upload (both route through Cms/etc/di.xml); theme file browser (Backend\Block\Media\Uploader defaults + Theme\Helper\Storage); watermark and product placeholder (Config\Backend\Image); Gd2::_getImageNeedMemorySize() tolerates WebP's missing channels/bits; ProductVideo remote-image retrieval uses NotProtectedExtension; import/export and REST/GraphQL base64 (ImageContentValidator, ImageProcessor, MimeTypeExtensionMap); renditions; and MediaStorage\Service\ImageResize / product image cache have no extension filters at all. |
…he admin file viewer
97b3a92 to
2ba5514
Compare
|
Thanks for the review — all points are addressed.
I also rebased onto One change worth flagging: |
|
Thanks. New code looks good. Will need to check it against the list. Still needs QA. |
Description (*)
Makes WebP and AVIF first-class input formats: they can be uploaded, validated, resized, browsed and served everywhere Magento already accepts an image. This is the reduced scope agreed in the discussion above — the format registry and the generated-copies feature are out, and the diff is down from 84 files to 52, from +2562/-154 to +357/-51.
WebP was blocked at two independent levels, and both had to be addressed for uploading to actually work:
The engine.
Gd2::$_callbacksmapped only GIF/JPEG/PNG/XBM/WBMP, soopen()raised "Unsupported image format" before any allow-list mattered. It now carries the WebP and AVIF entries. Four related fixes on the same path:save()honours the configured quality. WebP previously fell through todefault, so$qualitystayednullandsystem/upload_configuration/jpeg_qualitywas ignored._getTransparency()assumed "transparency is supported by gif/png only", which flattened WebP alpha onto black. WebP and AVIF are always decoded into a truecolor resource, and detecting real alpha means parsing container-specific flags — getting that wrong is what produces the black background. Treating them as potentially-alpha routes them down the same path as a truecolor PNG, which is correct either way.crop()only preserved alpha for PNG; it now covers WebP and AVIF too._getCallback()checks the GD helper exists. GD only declaresimagewebp/imageavifwhen it was built against libwebp/libavif, so without this a build lacking the codec raised "Call to undefined function". It now surfaces as theInvalidArgumentExceptionfor an unsupported format, which the rest of the stack already handles.ImageMagick::_applyOptions()forcedCOMPRESSION_JPEGunconditionally, which on WebP or AVIF output yields a container whose declared compression contradicts its payload. It is now applied only to the other formats, leaving Imagick's own default in place.The allow-lists.
jpg jpeg gif pngwas hardcoded in about twenty places:di.xmlarrays,ui_component<allowedExtensions>,_getAllowedExtensions()overrides, MIME maps inFramework\File\MimeandFilesystem\Driver\File\Mime,Framework\Api\ImageProcessorandImageContentValidator,Catalog's gallery processor and image attribute backend,MediaGalleryUi\Ui\Component\ImageUploader, theCatalogImportExportuploader, and theMediaGalleryRenditionsfilename pattern. Each gainedwebpandavifalongside the existing entries.Two of them are worth calling out:
MediaStorage\Model\File\Validator\Imagereadsif (mime is in the list) { try to open it }and returnstrueotherwise. Adding WebP there adds real content validation rather than removing it — the opposite of how the diff reads at a glance.media-uploader.js,base-image-uploader.js,new-video-dialog.jsand the configurable-productbulk.jswere updated, the admin refused.webpwhile the server already accepted it.Error reporting. The gallery upload controller only translated
LocalizedException, so the image adapter's own message was replaced by "Something went wrong while saving the file(s)." — which is exactly what a perfectly valid AVIF gets on a build whose GD lacks libavif, now that the format is allowed through. It now surfaces those messages, so the merchant reads "Unsupported image format." and can act on it.Caveats worth a reviewer's attention
function_exists()guard and the error-reporting fix are part of this PR rather than a follow-up — without them an AVIF upload on a common build is a fatal error or an unactionable message. WebP is safe: libwebp has been standard in distro GD builds for years.jpeis not included. The JPEG entries stayjpg/jpegas they are today, so nothing widens beyond WebP and AVIF.Out of scope
Two parts were split out during review, per the thread above: the format registry (opening separately) and generated WebP/AVIF copies of cached catalog images (dropped).
Beyond that, three paths deliberately keep their current behaviour:
MediaGalleryMetadataimplements each container separately —Model/Pngis 974 lines,Model/Gif736 — so WebP needs an equivalentModel/Webpparsing and rewriting the RIFFEXIF/XMPchunks: a binary-format implementation in a module this PR does not otherwise touch, comparable in size to the whole PR, and not something Native WebP support #303 asks for. The consequence is cosmetic and I verified both directions, since they handle errors differently: reading falls intoFile\ExtractMetadata's catch and yields empty metadata, and writing hitsFile\AddMetadata'sValidatorExceptionbranch and returns silently, because all three readers throw exactly that on a signature mismatch. So a.webpasset simply has no title, description or keywords imported from the file, and admin-entered ones live only in Magento's database. Worth its own PR.Magento\Framework\Pdfcannot embed WebP, soConfig\Backend\Image\Pdfkeeps overriding the list withtif/tiff/pdfand still rejects it.Theme\Model\Design\Backend\Faviconkeeps its ownico/png/gif/jpg/jpeg/apnglist. Browser favicon support does not track<img>support, and.ico/.pngremain the safe choices.Related Pull Requests
The image format registry, as a follow-up to this PR.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Verified on PHP 8.3 with GD (libwebp and libavif) and Imagick with both delegates, driving the real admin forms in a browser. The unsupported-build path was exercised too: on the same install before GD was rebuilt with libavif, an AVIF upload was refused with an unsupported-format error rather than a fatal.
.webpto a product's Images and Videos gallery — it uploads, the thumbnail renders, the storefront shows it..webpbase_imageURL, andPOST /V1/products/:sku/mediawith a base64 WebP..webpand confirm the cache images keep transparency instead of turning black..webpis still rejected for the PDF print logo..avifto a product gallery, with the adapter set toGD2and again toIMAGEMAGICK.Questions or comments
Images (...)file-picker labels.Backend\Block\Media\Uploaderand the product galleryContentblock each carry a translatable label listing the extensions, which now has to be kept in step with thefilesarray beside it by hand. Both were already out of sync onmain(the gallery label said.gif, .jpg, .pngwhile its filter accepted.jpeg), so I extended them literally rather than deriving them. Happy to leave them as is or take another approach.Customer\Model\Metadata\Form\ImageandEav\Model\Attribute\Data\Imagekeyed their maps on the bare integers1,2,3. Adding18and19for WebP and AVIF would have been inscrutable, so those five entries now use theIMAGETYPE_*constants. Same values, no behaviour change.Contribution checklist (*)
setup:di:compile, one PHPUnit run per module asnx affecteddoes). Every affected module is green:Catalog2639,Customer1401,View810,Backend583,Config436,Theme435,Eav422,Cms289,CatalogImportExport243,ConfigurableProduct242,Email181,Filesystem112,Swatches110,MediaStorage76,Api74,ProductVideo36,File33,Image25,MediaGalleryUi17,MediaGalleryRenditions6. Alsosetup:di:compile, and phpcs clean at the gate's own severity (--severity=5 --warning-severity=8 --error-severity=8), with no new warnings on any touched file. The integration and MFTF suites were run on the original combined branch, not re-run after the reduction, so I am deferring those to CI — the reduction only removes code, and the four integration tests that covered the dropped output-format API were removed with it. From those runs:AdminProductImageAvifUploadTestpassed consistently, andAdminSimpleProductImagesTestis flaky in my environment — four runs with unmodified upstream JS failed in three different places, and it already carriespr_excludefor AC-12124 — but three of four runs reached and passed the new WebP step.