Skip to content

Commit c0155cd

Browse files
westonruterwestonruter
committed
Media: Normalize unusable sizes attachment metadata.
Attachment metadata is untyped, and the `sizes` key is not guaranteed to be present or to hold an array. Sub-size generation can leave it out entirely, and a plugin filtering `wp_get_attachment_metadata` can replace it with anything. `wp_save_image()` validated only that the metadata itself was an array before passing `$meta['sizes']` to `array_merge()`, so an absent or scalar value raised a `TypeError` and the image editor returned an HTTP 500 mid-save. `wp_restore_image()` had the same gap at `$meta['sizes'][ $default_size ] = $data`, where a string raises "Cannot use a scalar value as an array" and `false` is deprecated as of PHP 8.1 and an error as of PHP 9. `wp_get_attachment_metadata()` now returns `false` whenever the metadata is not an array, on the `$unfiltered` path as well as after the filter, matching the documented `array|false` return. A `sizes` key holding a non-array is replaced with an empty array, so every caller can rely on the key being an array whenever it is present. The key is not invented when it is absent: audio, video and document attachments legitimately store metadata without it, and callers such as `wp-admin/post.php` read the metadata unfiltered in order to modify it and write it back, so normalizing there would persist into the database. The image editor entry points fill in the missing key themselves, and `wp_prepare_attachment_for_js()` now checks the dimensions of the `full` entry alongside its filename before reading them, removing the "Undefined array key" warnings raised for a `sizes` array that carries no usable `full` size. PHPUnit coverage is added for all three functions. Developed in WordPress#12744. Follow-up to r11965, r23873, r38949, r49084, r62978. Props josephscott, westonruter, mukesh27, irozum, ugyensupport, nazmulasif. See #65686, #64898. Fixes #65748. git-svn-id: https://develop.svn.wordpress.org/trunk@63002 602fd350-edb4-49c9-b593-d223f7449a82
1 parent 4af02ef commit c0155cd

6 files changed

Lines changed: 586 additions & 4 deletions

File tree

‎src/wp-admin/includes/image-edit.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -820,11 +820,13 @@ function wp_restore_image( $post_id ) {
820820
$restored = false;
821821
$msg = new stdClass();
822822

823-
if ( ! is_array( $backup_sizes ) ) {
823+
if ( ! is_array( $meta ) || ! is_array( $backup_sizes ) ) {
824824
$msg->error = __( 'Cannot load image metadata.' );
825825
return $msg;
826826
}
827827

828+
$meta['sizes'] ??= array();
829+
828830
$parts = pathinfo( $file );
829831
$suffix = time() . rand( 100, 999 );
830832
$default_sizes = get_intermediate_image_sizes();
@@ -983,6 +985,8 @@ function wp_save_image( $post_id ) {
983985
return $return;
984986
}
985987

988+
$meta['sizes'] ??= array();
989+
986990
if ( ! is_array( $backup_sizes ) ) {
987991
$backup_sizes = array();
988992
}

‎src/wp-includes/media.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4814,7 +4814,10 @@ function wp_prepare_attachment_for_js( $attachment ) {
48144814
}
48154815

48164816
$response = array_merge( $response, $sizes['full'] );
4817-
} elseif ( $meta['sizes']['full']['file'] ) {
4817+
} elseif (
4818+
! empty( $meta['sizes']['full']['file'] ) &&
4819+
isset( $meta['sizes']['full']['width'], $meta['sizes']['full']['height'] )
4820+
) {
48184821
$sizes['full'] = array(
48194822
'url' => esc_url_raw( $base_url . $meta['sizes']['full']['file'] ),
48204823
'height' => $meta['sizes']['full']['height'],

‎src/wp-includes/post.php‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7048,6 +7048,8 @@ function wp_delete_attachment_files( $post_id, $meta, $backup_sizes, $file ) {
70487048
*
70497049
* @since 2.1.0
70507050
* @since 6.0.0 The `$filesize` value was added to the returned array.
7051+
* @since 7.1.0 `false` is now returned if the metadata is not an array, and when the result is
7052+
* filtered the `sizes` key is always an array when present.
70517053
*
70527054
* @param int $attachment_id Attachment post ID. Defaults to global $post.
70537055
* @param bool $unfiltered Optional. If true, filters are not run. Default false.
@@ -7111,7 +7113,7 @@ function wp_get_attachment_metadata( $attachment_id = 0, $unfiltered = false ) {
71117113

71127114
$data = get_post_meta( $attachment_id, '_wp_attachment_metadata', true );
71137115

7114-
if ( ! $data ) {
7116+
if ( ! is_array( $data ) || ! $data ) {
71157117
return false;
71167118
}
71177119

@@ -7127,7 +7129,17 @@ function wp_get_attachment_metadata( $attachment_id = 0, $unfiltered = false ) {
71277129
* @param array $data Array of meta data for the given attachment.
71287130
* @param int $attachment_id Attachment post ID.
71297131
*/
7130-
return apply_filters( 'wp_get_attachment_metadata', $data, $attachment_id );
7132+
$data = apply_filters( 'wp_get_attachment_metadata', $data, $attachment_id );
7133+
7134+
if ( ! is_array( $data ) ) {
7135+
return false;
7136+
}
7137+
7138+
if ( array_key_exists( 'sizes', $data ) && ! is_array( $data['sizes'] ) ) {
7139+
$data['sizes'] = array();
7140+
}
7141+
7142+
return $data;
71317143
}
71327144

71337145
/**

‎tests/phpunit/tests/ajax/wpAjaxImageEditor.php‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,4 +194,136 @@ public function test_filesize_restored_after_restoring_original_image() {
194194

195195
$this->assertSameSetsWithIndex( $pre_file_sizes, $post_restore_file_sizes, 'Filesize should have restored after restoring the original image.' );
196196
}
197+
198+
/**
199+
* Ensure editing an image does not fatal when the attachment metadata has no usable `sizes` data.
200+
*
201+
* Attachment metadata is not guaranteed to contain a `sizes` array. It can be missing when
202+
* sub-size generation never ran or failed (for example `wp_create_image_subsizes()` returns an
203+
* empty array when the file cannot be parsed), or when it is removed by a plugin filtering
204+
* `wp_get_attachment_metadata`. `wp_save_image()` only validates that the metadata itself is an
205+
* array, then passes `$meta['sizes']` straight to `array_merge()`.
206+
*
207+
* @ticket 65748
208+
*
209+
* @covers ::wp_save_image
210+
*
211+
* @dataProvider data_save_image_with_unusable_sizes_metadata
212+
*
213+
* @param array{ sizes?: mixed } $meta Attachment metadata to store before editing, minus the file-specific keys.
214+
*/
215+
public function test_save_image_with_unusable_sizes_metadata( array $meta ) {
216+
require_once ABSPATH . 'wp-admin/includes/image-edit.php';
217+
218+
$filename = DIR_TESTDATA . '/images/canola.jpg';
219+
$contents = file_get_contents( $filename );
220+
$this->assertIsString( $contents );
221+
222+
$upload = wp_upload_bits( wp_basename( $filename ), null, $contents );
223+
$id = $this->_make_attachment( $upload );
224+
$this->assertIsInt( $id );
225+
226+
$original_meta = wp_get_attachment_metadata( $id );
227+
$this->assertIsArray( $original_meta );
228+
229+
// Keep the real file/dimension data, only make `sizes` unusable.
230+
$meta = array_merge(
231+
wp_array_slice_assoc( $original_meta, array( 'width', 'height', 'file', 'filesize' ) ),
232+
$meta
233+
);
234+
235+
wp_update_attachment_metadata( $id, $meta );
236+
237+
$_REQUEST['action'] = 'image-editor';
238+
$_REQUEST['context'] = 'edit-attachment';
239+
$_REQUEST['postid'] = $id;
240+
$_REQUEST['target'] = 'all';
241+
$_REQUEST['do'] = 'save';
242+
$_REQUEST['history'] = '[{"c":{"x":5,"y":8,"w":289,"h":322}}]';
243+
244+
$ret = wp_save_image( $id );
245+
246+
$this->assertObjectNotHasProperty( 'error', $ret, 'Saving the image should not have returned an error.' );
247+
248+
$saved_meta = wp_get_attachment_metadata( $id );
249+
250+
$this->assertIsArray( $saved_meta, 'The saved attachment metadata should be an array.' );
251+
$this->assertArrayHasKey( 'sizes', $saved_meta );
252+
$this->assertIsArray( $saved_meta['sizes'], 'The saved attachment metadata should contain a `sizes` array.' );
253+
$this->assertArrayHasKey( 'thumbnail', $saved_meta['sizes'], 'The edited image should have regenerated the thumbnail size.' );
254+
}
255+
256+
/**
257+
* Ensure restoring an image does not fatal when the attachment metadata has no usable `sizes` data.
258+
*
259+
* `wp_restore_image()` writes each backed up size with `$meta['sizes'][ $default_size ] = $data`
260+
* without ever checking that `$meta['sizes']` is an array. A scalar value raises
261+
* "Cannot use a scalar value as an array", and `false` is deprecated as of PHP 8.1 and
262+
* an error as of PHP 9. The same metadata that fatals `wp_save_image()` reaches this code.
263+
*
264+
* @ticket 65748
265+
*
266+
* @covers ::wp_restore_image
267+
*
268+
* @dataProvider data_save_image_with_unusable_sizes_metadata
269+
*
270+
* @param array{ sizes?: mixed } $meta Replacement `sizes` metadata to store before restoring.
271+
*/
272+
public function test_restore_image_with_unusable_sizes_metadata( array $meta ) {
273+
require_once ABSPATH . 'wp-admin/includes/image-edit.php';
274+
275+
$filename = DIR_TESTDATA . '/images/canola.jpg';
276+
$contents = file_get_contents( $filename );
277+
$this->assertIsString( $contents );
278+
279+
$upload = wp_upload_bits( wp_basename( $filename ), null, $contents );
280+
$id = $this->_make_attachment( $upload );
281+
$this->assertIsInt( $id );
282+
283+
$_REQUEST['action'] = 'image-editor';
284+
$_REQUEST['context'] = 'edit-attachment';
285+
$_REQUEST['postid'] = $id;
286+
$_REQUEST['target'] = 'all';
287+
$_REQUEST['do'] = 'save';
288+
$_REQUEST['history'] = '[{"c":{"x":5,"y":8,"w":289,"h":322}}]';
289+
290+
// Edit the image first so that `_wp_attachment_backup_sizes` holds the original sizes.
291+
wp_save_image( $id );
292+
293+
$this->assertNotEmpty(
294+
get_post_meta( $id, '_wp_attachment_backup_sizes', true ),
295+
'The image edit should have stored backup sizes to restore from.'
296+
);
297+
298+
// Keep the metadata written by the edit, only make `sizes` unusable.
299+
$edited_meta = wp_get_attachment_metadata( $id );
300+
$this->assertIsArray( $edited_meta );
301+
unset( $edited_meta['sizes'] );
302+
303+
wp_update_attachment_metadata( $id, array_merge( $edited_meta, $meta ) );
304+
305+
wp_restore_image( $id );
306+
307+
$restored_meta = wp_get_attachment_metadata( $id );
308+
$this->assertIsArray( $restored_meta );
309+
310+
$this->assertArrayHasKey( 'sizes', $restored_meta );
311+
$this->assertIsArray( $restored_meta['sizes'], 'The restored attachment metadata should contain a `sizes` array.' );
312+
$this->assertArrayHasKey( 'thumbnail', $restored_meta['sizes'], 'The restored image should have the thumbnail size restored from the backup sizes.' );
313+
}
314+
315+
/**
316+
* Data provider.
317+
*
318+
* @return array<non-empty-string, array{ 0: array{ sizes?: mixed } }>
319+
*/
320+
public function data_save_image_with_unusable_sizes_metadata(): array {
321+
return array(
322+
'no sizes key' => array( array() ),
323+
'null sizes' => array( array( 'sizes' => null ) ),
324+
'empty string' => array( array( 'sizes' => '' ) ),
325+
'string sizes' => array( array( 'sizes' => 'not-an-array' ) ),
326+
'boolean sizes' => array( array( 'sizes' => false ) ),
327+
);
328+
}
197329
}

‎tests/phpunit/tests/media.php‎

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -667,6 +667,153 @@ public function test_wp_prepare_attachment_for_js_without_image_sizes() {
667667
$this->assertArrayHasKey( 'sizes', $prepped );
668668
}
669669

670+
/**
671+
* Tests that an unusable `full` entry in the `sizes` metadata is skipped.
672+
*
673+
* Attachments that are not images, such as PDFs, are handled by a separate branch that reads
674+
* the `full` entry of the `sizes` metadata directly. That entry is not guaranteed to be there,
675+
* nor to carry dimensions when it is, and reading it unconditionally raises "Undefined array
676+
* key" warnings.
677+
*
678+
* @ticket 65748
679+
*
680+
* @dataProvider data_wp_prepare_attachment_for_js_unusable_full_size
681+
*
682+
* @covers ::wp_prepare_attachment_for_js
683+
*
684+
* @param array<string, mixed> $sizes Value to store as the `sizes` metadata.
685+
*/
686+
public function test_wp_prepare_attachment_for_js_with_an_unusable_full_size( array $sizes ) {
687+
$id = $this->create_pdf_attachment( $sizes );
688+
689+
$prepped = wp_prepare_attachment_for_js( $id );
690+
691+
$this->assertIsArray( $prepped );
692+
$this->assertArrayHasKey( 'sizes', $prepped );
693+
694+
$sizes = $prepped['sizes'];
695+
696+
$this->assertIsArray( $sizes );
697+
$this->assertArrayNotHasKey( 'full', $sizes, 'An unusable `full` size should not have been exposed.' );
698+
}
699+
700+
/**
701+
* Tests that a usable `full` entry in the `sizes` metadata is still exposed.
702+
*
703+
* @ticket 65748
704+
*
705+
* @covers ::wp_prepare_attachment_for_js
706+
*/
707+
public function test_wp_prepare_attachment_for_js_with_a_usable_full_size() {
708+
$id = $this->create_pdf_attachment(
709+
array(
710+
'full' => array(
711+
'file' => 'test-document-pdf.jpg',
712+
'width' => 232,
713+
'height' => 300,
714+
'mime-type' => 'image/jpeg',
715+
),
716+
)
717+
);
718+
719+
$prepped = wp_prepare_attachment_for_js( $id );
720+
721+
$this->assertIsArray( $prepped );
722+
$this->assertArrayHasKey( 'sizes', $prepped );
723+
724+
$sizes = $prepped['sizes'];
725+
726+
$this->assertIsArray( $sizes );
727+
$this->assertArrayHasKey( 'full', $sizes, 'A usable `full` size should have been exposed.' );
728+
729+
$full = $sizes['full'];
730+
731+
$this->assertIsArray( $full );
732+
$this->assertSame( 232, $full['width'] );
733+
$this->assertSame( 300, $full['height'] );
734+
$this->assertSame( 'portrait', $full['orientation'] );
735+
$this->assertIsString( $full['url'] );
736+
$this->assertStringEndsWith( '/test-document-pdf.jpg', $full['url'] );
737+
}
738+
739+
/**
740+
* Data provider.
741+
*
742+
* @return array<non-empty-string, array{ 0: array<string, mixed> }>
743+
*/
744+
public function data_wp_prepare_attachment_for_js_unusable_full_size(): array {
745+
return array(
746+
'no full size' => array(
747+
array(
748+
'thumbnail' => array(
749+
'file' => 'test-document-pdf-116x150.jpg',
750+
'width' => 116,
751+
'height' => 150,
752+
'mime-type' => 'image/jpeg',
753+
),
754+
),
755+
),
756+
'full without dimensions' => array(
757+
array(
758+
'full' => array(
759+
'file' => 'test-document-pdf.jpg',
760+
'mime-type' => 'image/jpeg',
761+
),
762+
),
763+
),
764+
'full without a height' => array(
765+
array(
766+
'full' => array(
767+
'file' => 'test-document-pdf.jpg',
768+
'width' => 232,
769+
'mime-type' => 'image/jpeg',
770+
),
771+
),
772+
),
773+
'full with an empty file' => array(
774+
array(
775+
'full' => array(
776+
'file' => '',
777+
'width' => 232,
778+
'height' => 300,
779+
'mime-type' => 'image/jpeg',
780+
),
781+
),
782+
),
783+
);
784+
}
785+
786+
/**
787+
* Creates a PDF attachment carrying the given `sizes` metadata.
788+
*
789+
* A PDF is used so that wp_prepare_attachment_for_js() takes the branch for attachments that
790+
* are not images, which is the one that reads the `full` entry of the `sizes` metadata.
791+
*
792+
* @param array<string, mixed> $sizes Value to store as the `sizes` metadata.
793+
* @return int Attachment ID.
794+
*/
795+
private function create_pdf_attachment( array $sizes ): int {
796+
$id = wp_insert_attachment(
797+
array(
798+
'post_title' => 'Attachment Title',
799+
'post_type' => 'attachment',
800+
'post_parent' => 0,
801+
'post_mime_type' => 'application/pdf',
802+
'guid' => home_url( '/wp-content/uploads/test-document.pdf' ),
803+
)
804+
);
805+
806+
wp_update_attachment_metadata(
807+
$id,
808+
array(
809+
'file' => 'test-document.pdf',
810+
'sizes' => $sizes,
811+
)
812+
);
813+
814+
return $id;
815+
}
816+
670817
/**
671818
* Tests that a `filesize` stored in the attachment metadata is normalized to a positive integer.
672819
*
@@ -3178,6 +3325,9 @@ public function test_get_image_send_to_editor_defaults_no_caption_no_rel() {
31783325
*
31793326
* @ticket 36246
31803327
* @requires function imagejpeg
3328+
*
3329+
* @covers ::wp_get_attachment_image
3330+
* @covers ::wp_get_attachment_metadata
31813331
*/
31823332
public function test_wp_get_attachment_image_should_use_wp_get_attachment_metadata() {
31833333
add_filter( 'wp_get_attachment_metadata', array( $this, 'filter_36246' ), 10, 2 );

0 commit comments

Comments
 (0)