Skip to content

Introduce filter webp_uploads_pre_replace_additional_image_source to short-circuit replacing additional image sources in frontend content #319

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.

Sign up for GitHub

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

Jump to bottom
Merged
felixarntz merged 15 commits into from
May 10, 2022
Merged

Introduce filter webp_uploads_pre_replace_additional_image_source to short-circuit replacing additional image sources in frontend content #319

felixarntz merged 15 commits into from
May 10, 2022

Conversation

@mehulkaklotar
Copy link
Member

@mehulkaklotar mehulkaklotar commented Apr 27, 2022

Summary

Fixes #311

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.

@mehulkaklotar mehulkaklotar 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) labels Apr 27, 2022
@mehulkaklotar mehulkaklotar self-assigned this Apr 27, 2022
mitogh
mitogh reviewed Apr 28, 2022
View reviewed changes
modules/images/webp-uploads/load.php Outdated
* @param string $target_mime The target mime in which the image should be created.
* @param string $context The context where this is function is being used.
*/
$image = (string) apply_filters( 'webp_uploads_pre_replace_additional_image_source', $image, $attachment_id, 'full', $target_mime, $context );
Copy link
Member

@mitogh mitogh Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filter should be placed above the line 510 and the code from str_replace would be executed only if the image is different, take a look at suggested approach here:

The full value in this instance should be the name of the size being modified at this point in time.

Copy link
Member

@mitogh mitogh Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of tests would be great as well.

Copy link
Member Author

@mehulkaklotar mehulkaklotar Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitogh I have added changes as per your feedback in the latest commits.

@mehulkaklotar mehulkaklotar marked this pull request as ready for review April 29, 2022 13:44
@mehulkaklotar mehulkaklotar requested a review from mitogh April 29, 2022 13:44
eugene-manuilov
eugene-manuilov requested changes May 2, 2022
View reviewed changes
mehulkaklotar and others added 6 commits May 3, 2022 15:42
@mehulkaklotar @eugene-manuilov
Update modules/images/webp-uploads/load.php
96700c0 
Co-authored-by: Eugene Manuilov <manuilov@google.com>
@mehulkaklotar @eugene-manuilov
Update modules/images/webp-uploads/load.php
fc0794e 
Co-authored-by: Eugene Manuilov <manuilov@google.com>
@mitogh @mehulkaklotar
Replace the featured image to a modern format
d5dbcf2 
When a modern format is available to render the featured image
make sure that the replacement image uses the same logic used
to replace the images in the content in order to be consistent
with the rest of rendered format.

Fixes #288
@felixarntz @mehulkaklotar
Remove blank line above add_filter call.
eca8f6e
@mehulkaklotar
php unit test case - remove all filter added for test
9ee668a
@mehulkaklotar
Merge branch 'trunk' into enhancement/replace-additional-image-filter
9ba42a0
@mukeshpanchal27
Copy link
Member

mukeshpanchal27 commented May 4, 2022 •
edited
Loading

Hi there!

Can we adjust the webp_uploads_pre_replace_additional_image_source filter code and doc block so it can be documented only one time and for other instances, we can add the below context as we use in WordPress?

/** This filter is documented in modules/images/webp-uploads/load.php */

Code changes that require for this.

Replace this line https://github.com/WordPress/performance/blob/enhancement/replace-additional-image-filter/modules/images/webp-uploads/load.php#L503 code with foreach ( $metadata['sizes'] as $size => $size_data ) {

Replace this whole doc block with https://github.com/WordPress/performance/blob/enhancement/replace-additional-image-filter/modules/images/webp-uploads/load.php#L516-L528 below context.

/** This filter is documented in modules/images/webp-uploads/load.php */
$filtered_image = (string) apply_filters( 'webp_uploads_pre_replace_additional_image_source', $image, $attachment_id, $size, $target_mime, $context );

Pull request: #324

Please let me it and let me know your thoughts.

@mehulkaklotar @mukeshpanchal27
filter php doc updated.
6ce6293 
Co-authored-by: Mukesh Panchal <mukeshpanchal27@gmail.com>
@mehulkaklotar mehulkaklotar requested a review from felixarntz as a code owner May 4, 2022 13:51
felixarntz
felixarntz reviewed May 9, 2022
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehulkaklotar Production code looks good to me. I have two minor documentation suggestions, more importantly though the test here should be updated to be a more appropriate usage: The filter is for an image HTML tag, so it shouldn't be used to replace the image tag with just a URL.

mehulkaklotar and others added 2 commits May 10, 2022 20:02
@mehulkaklotar @felixarntz
Update modules/images/webp-uploads/load.php
e35fccd 
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@mehulkaklotar @felixarntz
Update modules/images/webp-uploads/load.php
305b44a 
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@mehulkaklotar mehulkaklotar requested a review from felixarntz May 10, 2022 16:25
felixarntz
felixarntz approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you @mehulkaklotar!

@felixarntz felixarntz changed the title Introduce filter webp_uploads_pre_replace_additional_image_source to use a custom logic to find additinal mime types images Introduce filter webp_uploads_pre_replace_additional_image_source to short-circuit replacing image sources in frontend content May 10, 2022
@felixarntz felixarntz changed the title Introduce filter webp_uploads_pre_replace_additional_image_source to short-circuit replacing image sources in frontend content Introduce filter webp_uploads_pre_replace_additional_image_source to short-circuit replacing additional image sources in frontend content May 10, 2022
@felixarntz felixarntz merged commit 993157d into trunk May 10, 2022
@felixarntz felixarntz deleted the enhancement/replace-additional-image-filter branch May 10, 2022 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eugene-manuilov eugene-manuilov eugene-manuilov approved these changes

@felixarntz felixarntz felixarntz approved these changes

@mitogh mitogh mitogh approved these changes

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@getsource getsource Awaiting requested review from getsource

Assignees

@mehulkaklotar mehulkaklotar

Labels

[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

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

Introduce filter to use custom logic to find additional mime types images

7 participants

@mehulkaklotar @mukeshpanchal27 @eugene-manuilov @felixarntz @mitogh @mxbclang