-
Notifications
You must be signed in to change notification settings - Fork 392
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
Save filesize with attachment metadata on upload #493
Changes from 6 commits
e1222c6
fe01c24
ce82bc4
4ff03c1
018a780
7564fbc
4c50bf1
069cab8
ec01ea6
20d5407
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,7 @@ public function setup() { | |
add_filter( 'wp_resource_hints', [ $this, 'wp_filter_resource_hints' ], 10, 2 ); | ||
|
||
add_action( 'wp_handle_sideload_prefilter', [ $this, 'filter_sideload_move_temp_file_to_s3' ] ); | ||
add_filter( 'wp_generate_attachment_metadata', [ $this, 'set_filesize_in_attachment_meta' ], 10, 2 ); | ||
|
||
add_action( 'wp_get_attachment_url', [ $this, 'add_s3_signed_params_to_attachment_url' ], 10, 2 ); | ||
add_action( 'wp_get_attachment_image_src', [ $this, 'add_s3_signed_params_to_attachment_image_src' ], 10, 2 ); | ||
|
@@ -129,6 +130,7 @@ public function tear_down() { | |
remove_filter( 'upload_dir', [ $this, 'filter_upload_dir' ] ); | ||
remove_filter( 'wp_image_editors', [ $this, 'filter_editors' ], 9 ); | ||
remove_filter( 'wp_handle_sideload_prefilter', [ $this, 'filter_sideload_move_temp_file_to_s3' ] ); | ||
remove_filter( 'wp_generate_attachment_metadata', [ $this, 'set_filesize_in_attachment_meta' ] ); | ||
|
||
remove_action( 'wp_get_attachment_url', [ $this, 'add_s3_signed_params_to_attachment_url' ] ); | ||
remove_action( 'wp_get_attachment_image_src', [ $this, 'add_s3_signed_params_to_attachment_image_src' ] ); | ||
|
@@ -376,7 +378,6 @@ public function filter_editors( array $editors ) : array { | |
|
||
return $editors; | ||
} | ||
|
||
/** | ||
* Copy the file from /tmp to an s3 dir so handle_sideload doesn't fail due to | ||
* trying to do a rename() on the file cross streams. This is somewhat of a hack | ||
|
@@ -396,6 +397,32 @@ public function filter_sideload_move_temp_file_to_s3( array $file ) { | |
return $file; | ||
} | ||
|
||
/** | ||
* Store the attachment filesize in the attachment meta array. | ||
* | ||
* Getting the filesize of an image in S3 involves a remote HEAD request, | ||
* which is a bit slower than a local filesystem operation would be. As a | ||
* result, operations like `wp_prepare_attachments_for_js' take substantially | ||
* longer to complete against s3 uploads than if they were performed with a | ||
* local filesystem.i | ||
* | ||
* Saving the filesize in the attachment metadata when the image is | ||
* uploaded allows core to skip this stat when retrieving and formatting it. | ||
* | ||
* @param array{file?: string} $metadata Attachment metadata. | ||
* @param int $attachment_id Attachment ID. | ||
* @return array{file?: string, filesize: int} Attachment metadata array, with "filesize" value added. | ||
*/ | ||
function set_filesize_in_attachment_meta( array $metadata, int $attachment_id ) : array { | ||
$file = get_attached_file( $attachment_id ); | ||
|
||
if ( file_exists( $file ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should return early if filesize is already set too, that will mean it's future compatible when images add it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was thinking that too. Adding it now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4c50bf1 |
||
$metadata['filesize'] = filesize( $file ); | ||
} | ||
|
||
return $metadata; | ||
} | ||
|
||
/** | ||
* Filters wp_read_image_metadata. exif_read_data() doesn't work on | ||
* file streams so we need to make a temporary local copy to extract | ||
|
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.
This should also be
filesize?
as it's possible the array won't contain it (if the file doesn't exist). I'd have thought Psalm would have caught that 🤔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.
Makes sense, but that didn't seem to fix the build either 🤔