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

Allow controlling which image format sources in the frontend can be replaced #238

Closed
Tracked by #22
felixarntz opened this issue Mar 18, 2022 · 5 comments · Fixed by #262
Closed
Tracked by #22

Allow controlling which image format sources in the frontend can be replaced #238

felixarntz opened this issue Mar 18, 2022 · 5 comments · Fixed by #262
Assignees
Labels
Needs Review Anything that requires code review [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Mar 18, 2022

Follow-up to #187: The webp_uploads_content_image_mimes currently controls which image formats can be used as replacements in the frontend. However, the formats that can currently be replaced in the frontend are still hard-coded - at the moment it is just JPEG (specifically its extensions jpg and jpeg).

We need to implement a way so that these source image formats can also be controlled through a filter. For example, if somebody implemented support to create WebP versions for PNG uploads, it should be possible to use the WebP Uploads module's logic to replace PNGs as well.

Related is #237, however let's keep these two decoupled. Depending on the decision in #237, it may simply be that the expected order of the filtered items changes, which would be trivial to update here.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) Needs Discussion Anything that needs a discussion/agreement labels Mar 18, 2022
@felixarntz
Copy link
Member Author

felixarntz commented Mar 18, 2022

I think we can reuse the existing webp_uploads_content_image_mimes filter for that, we don't need to introduce yet another filter for modern image formats.

More concretely, I'm suggesting the following approach to make the sources for the replacement more dynamic:

  • Run the webp_uploads_content_image_mimes filter before matching the source URLs on the $image.
  • Consider all $target_mimes except for the last as potential source MIME types.
  • Replace the hardcoded $target_image_extensions list with a dynamic list of extensions based on the above "potential source MIME types", using wp_get_mime_types(), doing an array_flip on it, and look up the extensions for all of these MIME types.
  • We can then implode all of these extensions into the preg_match_all call.
  • Side-note: Let's rename the $target_image_extensions variable to $src_extensions, since it's actually not about the "target", but the "source".

The above would mean that for the current default filter value array( 'image/jpeg', 'image/webp' ), the dynamic $target_image_extensions would be array( 'jpg|jpeg|jpe' ), so the preg_match_all call would look the same as it does now (except that it also would include jpe which is missing in today's hardcoded list). Here are some other examples for how the filter value would affect the dynamic $target_image_extensions:

  • Filter array( 'image/jpeg', 'image/webp', 'image/avif' ) → extensions array( 'jpg|jpeg|jpe', 'webp' )
  • Filter array( 'image/png', 'image/jpeg', 'image/webp' ) → extensions array( 'png', 'jpg|jpeg|jpe', 'webp' )
  • Filter array( 'image/tiff', 'image/png', 'image/jpeg', 'image/webp' ) → extensions array( 'tiff|tif', 'png', 'jpg|jpeg|jpe', 'webp' )

See the webp_uploads_img_tag_update_mime_type() function implementation for context.

Let me know your thoughts on the above. I think it would be a simple and reliable solution to allow control over which formats to potentially replace. While it would technically also enable doing "odd" things like replacing PNG with JPEG, the eventual replacement still always depends on which formats exist for the attachment, so it wouldn't realistically happen - unless a developer added support for generating JPEG from PNG image uploads (which may not even be a bad idea!).

@eugene-manuilov
Copy link
Contributor

@felixarntz, just want to re-iterate my thoughts that we discussed yesterday. Do we really need to check format of the original image? If we have a replacement for it in a modern format that has a smaller size, then we can replace the image without checking its format/extension. If there is no an alternative size for an image, then we shouldn't touch it and leave as is. Does it make sense to you?

cc @adamsilverstein

@felixarntz
Copy link
Member Author

felixarntz commented Mar 22, 2022

@eugene-manuilov Double-checking, this would mean we would no longer rely on preg_match to get all URLs within the image tag, correct? From a performance perspective this feels like a wise choice.

It may not work for certain plugins that e.g. integrate with a CDN and also modify frontend URLs on the fly, but I like your approach here, specifically because it actually feels safer. This way we only replace data based on the original attachment metadata, and if that is for some reason not what's used in the content, at least we're not breaking anything.

Feel free to open a PR with your proposed approach, I think that sounds solid. The main achievement here needs to be that we don't have it hard-coded to just JPEGs.

@eugene-manuilov
Copy link
Contributor

@felixarntz, I have prepared #262, do you mind looking at it? As to the CDN, I don't think it will be an issue. Changes in my PR don't modify the whole URL, we just replace the filename itself. So CDN URLs should continue working.

@eugene-manuilov eugene-manuilov moved this from To do to Review in [Focus] Images Mar 24, 2022
@eugene-manuilov eugene-manuilov added Needs Review Anything that requires code review and removed Needs Discussion Anything that needs a discussion/agreement labels Mar 24, 2022
@felixarntz
Copy link
Member Author

Fixed via #262.

[Focus] Images automation moved this from Review to Done Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Anything that requires code review [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature
Projects
No open projects
3 participants