-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fix: PDF thumbnails to WEBP/AVIF #1868
base: trunk
Are you sure you want to change the base?
Fix: PDF thumbnails to WEBP/AVIF #1868
Conversation
- Add application/pdf as default transforms - Fix image_path issue to handle files other than images - Add unit test to check pdf is included in transforms
As mentioned in the issue comment, the solution doesn't seem work when we test it on the dev environment run using
This appears to be an issue with ![]() |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1868 +/- ##
==========================================
+ Coverage 66.70% 66.72% +0.01%
==========================================
Files 88 88
Lines 7029 7032 +3
==========================================
+ Hits 4689 4692 +3
Misses 2340 2340
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
plugins/webp-uploads/helper.php
Outdated
@@ -144,7 +145,7 @@ function webp_uploads_generate_additional_image_source( int $attachment_id, stri | |||
return new WP_Error( 'image_mime_type_not_supported', __( 'The provided mime type is not supported.', 'webp-uploads' ) ); | |||
} | |||
|
|||
$image_path = wp_get_original_image_path( $attachment_id ); | |||
$image_path = 'application/pdf' !== get_post_mime_type( $attachment_id ) ? wp_get_original_image_path( $attachment_id ) : get_attached_file( $attachment_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor point: This is easier to read and it would facilitate ensuring test coverage is present for both code paths. Also, in the future maybe there will be more such special cases.
$image_path = 'application/pdf' !== get_post_mime_type( $attachment_id ) ? wp_get_original_image_path( $attachment_id ) : get_attached_file( $attachment_id ); | |
if ( 'application/pdf' === get_post_mime_type( $attachment_id ) { | |
$image_path = get_attached_file( $attachment_id ); | |
} else { | |
$image_path = wp_get_original_image_path( $attachment_id ); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking something like this:
$image_path = wp_attachment_is_image( $attachment_id ) ? wp_get_original_image_path( $attachment_id ) : get_attached_file( $attachment_id );
This way we can use wp_get_original_image_path()
only when attatchment is an image and in all other cases we can use get_attached_file ()
This is also more in accordance with latest suggestions made on the issue thread: #1766 (comment)
plugins/webp-uploads/helper.php
Outdated
@@ -144,7 +145,7 @@ function webp_uploads_generate_additional_image_source( int $attachment_id, stri | |||
return new WP_Error( 'image_mime_type_not_supported', __( 'The provided mime type is not supported.', 'webp-uploads' ) ); | |||
} | |||
|
|||
$image_path = wp_get_original_image_path( $attachment_id ); | |||
$image_path = wp_attachment_is_image( $attachment_id ) ? wp_get_original_image_path( $attachment_id ) : get_attached_file( $attachment_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's still avoid the ternary for readability and to ensure we have code coverage for both conditions:
$image_path = wp_attachment_is_image( $attachment_id ) ? wp_get_original_image_path( $attachment_id ) : get_attached_file( $attachment_id ); | |
if ( wp_attachment_is_image( $attachment_id ) ) { | |
$image_path = wp_get_original_image_path( $attachment_id ); | |
} else { | |
$image_path = get_attached_file( $attachment_id ); | |
} |
Summary
This PR adds support for generating Modern Format (webp/aif) image thumbnails from PDF files.
Fixes #1766
Relevant technical choices
application/pdf
mimetype to the default transforms list and useget_attached_file()
function for getting the original pdf file path if mime type isapplication/pdf
to bypass$image_path = wp_get_original_image_path( $attachment_id );
Screen records
Screen.Recording.2025-02-12.at.8.48.56.AM.mov