Full sized alternate mime creation for PDF upload special handling#3048
Full sized alternate mime creation for PDF upload special handling#3048mehulkaklotar wants to merge 14 commits intoWordPress:trunkfrom
Conversation
src/wp-admin/includes/image.php
Outdated
|
|
||
| // Generate image sub-sizes for each mime type. | ||
| if ( ! empty( $mime_transforms ) ) { | ||
| foreach ( $mime_transforms as $mime_type ) { |
There was a problem hiding this comment.
generate the images for the primary type first, then loop thru the secondary types
src/wp-admin/includes/image.php
Outdated
| } | ||
|
|
||
| $editor->set_output_mime_type( $mime_type ); | ||
| $result = $editor->save( $editor->generate_filename( '', pathinfo( $image_file, PATHINFO_FILENAME ), $extension ) ); |
There was a problem hiding this comment.
do you need to generate the name here? $editor->save() should use the correct naming once you set the output mime type
There was a problem hiding this comment.
when we not generate the name by just using $editor->save(), it adds suffix of image resolution to the name.
There was a problem hiding this comment.
👍🏼 hmm, that might be ok to have the dimensions suffix? I'll review again
src/wp-admin/includes/image.php
Outdated
| @@ -900,7 +900,6 @@ function wp_generate_attachment_metadata( $attachment_id, $file ) { | |||
| $preview_file = $dirname . wp_unique_filename( $dirname, wp_basename( $file, $ext ) . '-pdf.jpg' ); | |||
There was a problem hiding this comment.
This block assumes the primary type is jpeg, lets stick that with a mapping in wp_upload_image_mime_transforms so this first full size image is the primary type from that (eg. it could be ONLY WebP)
There was a problem hiding this comment.
Added changes in latest commit.
adamsilverstein
left a comment
adamsilverstein
left a comment
There was a problem hiding this comment.
Left some feedback. This is going in the right direction however...
We can likely simplify this a bit by using _wp_get_primary_and_additional_mime_types and adding a mapping from 'application/pdf' => [ 'image/webp', 'image/jpeg'] in wp_upload_image_mime_transforms.
Importantly using the wp_upload_image_mime_transforms filter, developers should be able to output webp, jpeg or both versions for uploaded pdfs.
Can you also check that when inserting the PDF into a post (where the image is used as a preview) the WebP version shows?
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
@mehulkaklotar Left some feedback for the unit test.
|
@adamsilverstein In the latest change, the priority mime type for |
|
|
||
| $orig_file = DIR_TESTDATA . '/images/wordpress-gsoc-flyer.pdf'; | ||
| $test_file = get_temp_dir() . 'wordpress-gsoc-flyer.pdf'; | ||
| copy( $orig_file, $test_file ); |
There was a problem hiding this comment.
why does this need to make a copy of the original to test file, can it use the test file directly?
There was a problem hiding this comment.
@adamsilverstein actually we need the file to be in temp directory to be able to create the upload object for the attachment and later to delete it.
| $this->assertNotEmpty( $metadata ); | ||
| $this->assertArrayHasKey( 'sizes', $metadata ); | ||
|
|
||
| foreach ( $metadata['sizes'] as $size ) { |
There was a problem hiding this comment.
we should validate that webp versions were actually output (or all the types specified in wp_upload_image_mime_transforms) when supported
There was a problem hiding this comment.
if we want to test the output then some of the environments doesn't actually support WebP but it prioritise the jpeg over WebP so the metadata generated has WebP in sources but it doesn't actually output it unless filtered via hooks. for example in 7.0 WebP is supported but it doesn't prioritise it and main versions generated are jpeg ones.
There was a problem hiding this comment.
I have put checks for mime types to verify them in sources attributes.
|
This is no longer needed since multi-mime was reverted. I tested on trunk and verified that the JPEG output for PDF uploads are already converted to WebP output using the current approach:
|
3 participants
Trac ticket: https://core.trac.wordpress.org/ticket/55443
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.