The Wayback Machine - https://web.archive.org/web/20221024152006/https://github.com/WordPress/performance/pull/319
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

Jump to bottom

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

Merged
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

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 Issues related to the Images focus area [Module] WebP Uploads Issues for the WebP Uploads module labels Apr 27, 2022
@mehulkaklotar mehulkaklotar self-assigned this Apr 27, 2022
mitogh
mitogh reviewed Apr 28, 2022
View 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 Apr 29, 2022
@mehulkaklotar mehulkaklotar requested a review from mitogh Apr 29, 2022
eugene-manuilov
eugene-manuilov requested changes May 2, 2022
View changes
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
tests/modules/images/webp-uploads/load-tests.php Show resolved Hide resolved
mehulkaklotar and others added 6 commits May 3, 2022
@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

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
@bethanylang bethanylang added the Needs Review Anything that requires code review label May 5, 2022
mitogh
mitogh approved these changes May 8, 2022
View changes
felixarntz
felixarntz reviewed May 9, 2022
View changes
Copy link
Member

@felixarntz felixarntz left a comment

@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.
modules/images/webp-uploads/load.php Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
modules/images/webp-uploads/load.php Outdated Show resolved Hide resolved
tests/modules/images/webp-uploads/load-tests.php Outdated Show resolved Hide resolved
mehulkaklotar and others added 2 commits May 10, 2022
@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
felixarntz
felixarntz approved these changes May 10, 2022
View changes
Copy link
Member

@felixarntz felixarntz left a comment

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 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 May 10, 2022
@felixarntz felixarntz merged commit 993157d into trunk May 10, 2022
11 checks passed
@felixarntz felixarntz deleted the enhancement/replace-additional-image-filter branch May 10, 2022
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 Uploads Issues for the WebP Uploads module Needs Review Anything that requires code review [Type] Enhancement A suggestion for improvement of an existing feature
6 participants
@mehulkaklotar @mukeshpanchal27 @eugene-manuilov @felixarntz @mitogh @bethanylang