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

Update WordPress core functions to support multiple mime types #155

Open
mitogh opened this issue Feb 7, 2022 · 13 comments
Open

Update WordPress core functions to support multiple mime types #155

mitogh opened this issue Feb 7, 2022 · 13 comments
Assignees
Labels
Needs Discussion Anything that needs a discussion/agreement [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@mitogh
Copy link
Member

mitogh commented Feb 7, 2022

Feature Description

Functions such as:

  • wp_get_attachment_image_src
  • wp_get_attachment_image_srcset
  • wp_get_attachment_image_url

Should be updated in order to support an additional parameter or to update the icon parameter in some cases to behave as mime type support parameter. This parameter would allow specifying the desired mime type for an image in case we want to render the JPEG, WebP, Avif or any other available mime type for an image based on the specified image size.

This is a subsequent step on the work for:

@felixarntz felixarntz added [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) [Type] Enhancement A suggestion for improvement of an existing feature labels Feb 7, 2022
@eclarke1
Copy link

eclarke1 commented Feb 8, 2022

@mitogh assigning this to you if you agree as it relates so closely to #142 and #149 where you're already working

@mitogh
Copy link
Member Author

mitogh commented Feb 8, 2022

Sounds good to me I'm assigning @jjgrainger as well to help with the collection of functions

@adamsilverstein
Copy link
Member

adamsilverstein commented Feb 14, 2022

I'm also going to start looking at this based on the data stored in #154 #147

@mxbclang mxbclang added the Needs Discussion Anything that needs a discussion/agreement label Mar 4, 2022
@jjgrainger
Copy link
Contributor

I am working on a proposal as to what changes are to be made. The WIP document is available here and open to early feedback.

@jjgrainger
Copy link
Contributor

jjgrainger commented Mar 8, 2022

The proposal for this is complete and open to feedback. Document is here.

The proposal is to add an additional $mime_type parameter to existing WordPress core functions to request a specific mime type for the attachment.

If the requested mime type is not available, other mime types will be checked for and eventually falling back to the attachments original mime type. A filter to define this order is being discussed in #187

@mitogh mitogh removed their assignment Mar 9, 2022
@mitogh
Copy link
Member Author

mitogh commented Mar 10, 2022

Thanks @jjgrainger the document looks solid and defines a clear path moving forward.

I'd suggest we can summarize the 3 options and share them during the next call on Tuesday so the rest of the community can provide any concerns/feedback or vote on the different approaches, but let me know what you think about that @adamsilverstein @bethanylang @jjgrainger .

@mxbclang
Copy link
Contributor

@mitogh That makes sense to me, but @adamsilverstein and @felixarntz, please confirm.

@jjgrainger Once you would summarize the options on next week's chat, I can 1) drop a comment for voting here with those summaries and ask folks to leave an emoji for whichever option they'd prefer, 2) add a Needs Decision label, and 3) set a deadline for voting, which is typically two weeks from vote start.

@felixarntz
Copy link
Member

@jjgrainger @mitogh @bethanylang I think this topic is a bit too close to code to efficiently discuss in the chat meeting. I'd say we should rather encourage providing more feedback on the doc. Maybe we can just leave a message in the channel anytime today sharing the doc and asking for feedback and preferred approach?

I'm going to have a look today as well. @adamsilverstein Can you please review this doc too? It would be crucial since it closely affects your core patch work.

On that note, I don't think this issue will require any plugin engineering since it's all related to the WP core implementation. That may be obvious, but I certainly wanted to clarify.

@jjgrainger
Copy link
Contributor

Thanks all,

Just wanted to provide an overview of the document shared last week. We are looking to update WordPress core functions to provide a means to retrieve a specific mime type for an attachment.

We're currently exploring 3 options, with a potential fourth that combines some of these. The current 3 within the document are:

  • Updating the $icon attribute in core functions to accept a mime type value
  • Updating core functions with an additional $mime_type parameter
  • Creating new core functions to work with attachment mime types

These options and others are currently being explored and discussed in the document. This document is open to everyone and welcome to feedback and suggestions on a preferred approach.

If you have anything you want to add please leave all feedback in the document.

I have left a message in the channel also requesting feedback.

@jjgrainger
Copy link
Contributor

I have updated the document with a fourth approach. This approach combines elements of the others outlined and explores creating a new wp_get_attachment_icon() function.

This is open and ready for review/feedback in the document.

@mukeshpanchal27
Copy link
Member

I've come across another support ticket where a user encountered a similar issue: https://wordpress.org/support/topic/how-to-generate-img-secret-including-webp-versions/. The user utilized the wp_get_attachment_image_url and wp_get_attachment_image_srcset functions to retrieve WebP images.

Interestingly, when we disable the Generate JPEG files in addition to WebP option from Settings > Media, these functions still return WebP images, which appears to be a strange behaviour.

cc. @felixarntz @adamsilverstein @joemcgill @eclarke1

@joemcgill
Copy link
Member

Thanks @mukeshpanchal27. Are you thinking that this is a bug with the WebP plugin, or with core? If the latter, I think a ticket in Trac would be best.

@mukeshpanchal27
Copy link
Member

@joemcgill, I spent some time checking if it's a core issue or a plugin issue. I also reviewed discussions on other issues/PRs attached to this issue. Since we don't make WebP the default in core, we should fix possible functions that support additional MIME types through action/filters. Something similar to what was in #517.

We can't introduce MIME types in core at the moment, or we can start adding them in separate pieces of work to extend those functions defined here #523. If we agree to make these changes, we can move forward with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Anything that needs a discussion/agreement [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
None yet
Development

No branches or pull requests

9 participants