Add checks before using size data in image_get_intermediate_size()
#3143
Conversation
| @@ -774,24 +774,28 @@ function image_get_intermediate_size( $post_id, $size = 'thumbnail' ) { | |||
| $imagedata['width'] = $imagedata['sizes']['full']['width']; | |||
| } | |||
|
|
|||
| foreach ( $imagedata['sizes'] as $_size => $data ) { | |||
| foreach ( $imagedata['sizes'] as $_size => $size_data ) { | |||
| if ( ! is_array( $size_data ) || ! isset( $size_data['width'] ) || ! isset( $size_data['height'] ) ) { | |||
There was a problem hiding this comment.
is_array() isn't needed here as the isset() is checking it.
| foreach ( $imagedata['sizes'] as $_size => $data ) { | ||
| foreach ( $imagedata['sizes'] as $_size => $size_data ) { | ||
| if ( ! is_array( $size_data ) || ! isset( $size_data['width'] ) || ! isset( $size_data['height'] ) ) { | ||
| continue; |
There was a problem hiding this comment.
Silently skipping isn't preferred here. Why?
wp_get_attachment_metadata() returns an array on success and 'sizes' is expected to be an array. If it's not an array, then something went wrong. Silently skipping makes it harder for developers to debug what happened and could lead to confusion for users.
There was a problem hiding this comment.
Thank you @Tabrisrp for submitting this PR.
The code expects the 'size' element to be an array as documented in wp_get_attachment_metadata().
While this PR adds defensive guards to protect it, it is silently skipping when 'sizes' is not an array or the 'width' or 'height' is not defined. I'm all for defensive coding.
But I wonder: Why are there no sizes for the attachment(s)? Could the issue be a callback hooked into filter? Or maybe the metadata is incomplete? Or did something happen when the attachment was added or edited?
Knowing the root cause will help contributors to determine if there's a bug to fix in Core, if defensive checks are needed here, and if an error or notice should be displayed to alert users of a problem with their attachment or developers with their code.
Also, the PR will need tests.
The size data is used in the foreach loop without checking if it's an array as expected, and also without checking the existence of the
widthandheightkeys.Also renamed the
$datavariable to$size_data, as the function already as a$datavariable declared before which is holding different values, and it can be confusing.Trac ticket: https://core.trac.wordpress.org/ticket/54943
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.