Introduce filter webp_uploads_pre_generate_additional_image_source to short-circuit generating additional image sources on upload

[mehulkaklotar]

Summary

Fixes #307

Relevant technical choices

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[mitogh]

Some comments, would be great to add some tests around the filter as well.

[mitogh]

LGTM

However it looks like we might want to introduce a new filter in the return value as currently, we are assuming the image would be in the same directory as the WordPress installation and calling filesize for files stored in a bucket like S3 this is not the case, I think in those cases we can rely on a new filter being used so plugins can define the logic in how the filesize is calculated.

For example;

return apply_filters( 'filter_name', array... 

Specifically here:

• performance/modules/images/webp-uploads/helper.php

  Lines 89 to 92 in 2efa53a

  	return array(
  	'file' => $image['file'],
  	'filesize' => filesize( $image['path'] ),
  	);

[felixarntz]

@mehulkaklotar The implementation is missing support for the possible WP_Error return type of the filter, which is critical and needs to be fixed. Other than that, just a documentation enhancement suggestion.

cc @eugene-manuilov @mitogh

[felixarntz]

@mehulkaklotar I've updated the base branch to release/1.1.0, with the goal to get this into the upcoming release. It would be great if you could implement the requested changes by tomorrow so that we can include this filter in the release. 🙌

[mehulkaklotar]

@felixarntz I am still not sure about the $image_size parameter in the filter here. #318 (comment)

so are we going to make bydefault full for all image size in filter? because it doesn't really make sense as I tested it and it turns out all image size data is being worked on in that.

[felixarntz]

@mehulkaklotar Thank you for making the changes, I have two follow-up comments.

[felixarntz]

@mehulkaklotar

so are we going to make bydefault full for all image size in filter? because it doesn't really make sense as I tested it and it turns out all image size data is being worked on in that.

Ahh, sorry I had totally missed this. Yes, this absolutely shouldn't show full for any image size. We will need to provide the name of the image size where the $size_data comes from. We don't have access to that currently from this function, so let's add a new function parameter $size_name right before $size_data.

We then need to pass this new parameter accordingly everywhere that webp_uploads_generate_additional_image_source() is called. Can you please make that update here?

[mehulkaklotar]

I have added changes for this in the latest commit.

[felixarntz]

Thanks @mehulkaklotar. Please review my new feedback in #318 (review) - part of your new implementation isn't needed, as the one function by definition is only used for the full image size, so we don't need to try to find the matching size, it's always full in that scenario.

[felixarntz]

@mehulkaklotar One more follow-up around the newly added change, which can be simplified.

[mehulkaklotar]

@felixarntz We should look into this comment from @mitogh about introducing another filter for returning value for function.

Should we introduce that in this change or make new PR for it?

[jjgrainger]

@felixarntz We should look into this comment from @mitogh about introducing another filter for returning value for function.

Should we introduce that in this change or make new PR for it?

@mehulkaklotar Is there a reason we couldn't expect the webp_uploads_pre_generate_additional_image_source filter to return the image array with the file and filesize keys? This would match the comment for the filter and also allow plugins to calculate the filesize appropriately depending on how the image is stored.

I would want @felixarntz and @mitogh to input here just incase I've overlooked something.

Thanks!

[jjgrainger]

Looks good to me, I've added a comment based on adding an additional filter to confirm #318 (comment)

[felixarntz]

Thank you @mehulkaklotar!

Regarding @mitogh's comment, that's an important point. I think a good solution would be to allow for an optional path key within the return array, which we would use instead of file in case it is set. Let's open a follow-up issue for that and work on it for the 1.2.0 plugin release.