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

Introduce filter to use a custom mechanism to create additional mime images #307

Closed
mitogh opened this issue Apr 19, 2022 · 6 comments · Fixed by #318
Closed

Introduce filter to use a custom mechanism to create additional mime images #307

mitogh opened this issue Apr 19, 2022 · 6 comments · Fixed by #318
Assignees
Labels
[Focus] Images Issues related to the Images focus area [Module] WebP Support Issues for the WebP Support module Needs Review Anything that requires code review [Type] Enhancement A suggestion for improvement of an existing feature
Milestone

Comments

@mitogh
Copy link
Member

mitogh commented Apr 19, 2022

The main objective of this filter is to allow third party plugins or developers to replace the current mechanism provided by this plugin in order to introduce his own logic or mechanism to create the additional mime types.

This is a sub-issue of #160 and related to #311.

Filter

webp_uploads_pre_generate_additional_image_source

Paramaters

  • image - By default a null value, or either WP_Error or an array with the details of the created image, in the same way the save() method is used.
  • attachment_id: The ID of the attachment being processed
  • image_size: The name of the image size we are looking for. When creating the additional mime type for the full image this value would be full in the same way it occurs in WordPress core.
  • image_details: An array containing the properties of image size.
  • mime_type: The targeted mime type of the file we are looking for.

Integration

This filter should be located when a new image is created in order to avoid using the performance plugin mechanism.

Suggested location for creation of images, as the first section of the function, if the filter return an array with the file and path property return that value.

Note: This filter should cover only the first time an image is created rather than including the subsequent edited versions to an image, a new filter would be created for subsequent edits to an image. For more details see: #307 (comment)

Suggested location for subsequent updates.

When an image is updated from within the image editor, this location in order to be fired for every single image size.

@mitogh mitogh added [Focus] Images Issues related to the Images focus area [Module] WebP Support Issues for the WebP Support module [Type] Enhancement A suggestion for improvement of an existing feature labels Apr 19, 2022
@mitogh mitogh added this to the First release after 1.0.0 milestone Apr 19, 2022
@eclarke1 eclarke1 added this to Backlog in [Focus] Images via automation Apr 19, 2022
@felixarntz
Copy link
Member

@mitogh Issue description looks good to move forward from my perspective, just a few details:

The name of the image size we are looking for. When creating the additional mime type for the full image this value would be null.

This feels a bit odd to me. Shouldn't we use full for the full size? full is already used in core to represent the full image size, and that is typically not a size found within the metadata sizes array, but refers to the "original" full image. So I would be in favor to use full here for the full image.

Suggested location for subsequent updates.

Good point that we need to cover this usage as well. Since the functionality there is somewhat different though, my hunch is that we may be better off with a separate filter for this one. It may also require a few different parameters. But at a minimum, I think we should use a different filter name because many developers are probably not familiar with that area, and we should ensure they don't accidentally modify the edit process when they think they are only modifying the regular image generation process. Maybe we can open yet another (third) issue to explore a separate filter for the "generate additional sizes on edit" use-case? We may wanna call that something like webp_uploads_pre_edit_additional_image_source? I'd prefer if we dealt with that separately.

@mitogh
Copy link
Member Author

mitogh commented Apr 26, 2022

This feels a bit odd to me. Shouldn't we use full for the full size? full is already used in core to represent the full image size, and that is typically not a size found within the metadata sizes array, but refers to the "original" full image. So I would be in favor to use full here for the full image.

The main reason for going with this approach was due we don't really have a size name, however I do agree that Core already uses full as the size name to represent this size, however you can create a new image size with the name full and that can collide with existing subsizes. So the main logic behind this decision was to avoid any possible collision in case a full size is defined in the installation, but let me know what do you think about that.

Good point that we need to cover this usage as well. Since the functionality there is somewhat different though, my hunch is that we may be better off with a separate filter for this one. It may also require a few different parameters. But at a minimum, I think we should use a different filter name because many developers are probably not familiar with that area, and we should ensure they don't accidentally modify the edit process when they think they are only modifying the regular image generation process. Maybe we can open yet another (third) issue to explore a separate filter for the "generate additional sizes on edit" use-case? We may wanna call that something like webp_uploads_pre_edit_additional_image_source? I'd prefer if we dealt with that separately.

Agree that would simplify things, so let me update the original issue to only reference to the first time the image is created.

@felixarntz

@felixarntz
Copy link
Member

@mitogh Thanks for the quick reply! It's a fair point that developers could also create their own size called full, however per core using full as a special scenario already, that would be a clear case of a developer doing it wrong. For example, when editing an image there would then suddenly be conflicts between full-orig storage key (is that the full image, or just the full image size, whatever that would be?).

In other words, I think it's okay here to use the term full, which can be considered a WordPress core size, just not present in the sizes array in most cases.

@mitogh
Copy link
Member Author

mitogh commented Apr 26, 2022

Thanks @felixarntz sounds good to me just wanted to make sure the original scenario was covering that but I agree that sticking to what core already does makes more sense in this case in order to be consistent.

I'm updating the original issue description to note this scenario instead.

@eclarke1
Copy link

Thanks both - just a note that @mehulkaklotar is going to pick up the Execution of this issue now it has been finalised. Along with #311

@felixarntz
Copy link
Member

Fixed via #318.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Images Issues related to the Images focus area [Module] WebP Support Issues for the WebP Support module Needs Review Anything that requires code review [Type] Enhancement A suggestion for improvement of an existing feature
Projects
5 participants