WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#35480 closed defect (bug) (fixed)

Some notice and warning in wp_calculate_image_srcset() function media.php

Reported by: overclokk Owned by: joemcgill
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4.1
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: template Cc:

Description

In a site I'm working I get some notice and warning in file media.php:

Warning: Illegal string offset 'file' in wp-includes/media.php on line 1088
Notice: Uninitialized string offset: 0 in wp-includes/media.php on line 1088
Warning: Illegal string offset 'width' in wp-includes/media.php on line 1101
Warning: Illegal string offset 'width' in wp-includes/media.php on line 1108
Notice: Uninitialized string offset: 0 in wp-includes/media.php on line 1108

To solve this I put this line of code to check the variable $image (at line 1085 just after the foreach) if it is an array:

<?php
        foreach ( $image_sizes as $image ) {

        if ( ! is_array( $image ) ) {
                continue;
        }

I also add the file patch.

I don't know how it can be reproduced because the site was already built but with a search on google I found many site with the same problems, here a link to google serp.

Attachments (6)

general-template.php.patch (2.2 KB) - added by overclokk 5 years ago.
media.php.patch (459 bytes) - added by overclokk 5 years ago.
35480.patch (621 bytes) - added by jaspermdegroot 5 years ago.
35480.1.patch (3.5 KB) - added by jaspermdegroot 5 years ago.
35480.2.patch (3.5 KB) - added by joemcgill 5 years ago.
35480.3.patch (1.8 KB) - added by joemcgill 5 years ago.

Download all attachments as: .zip

Change History (27)

@overclokk
5 years ago

#1 @overclokk
5 years ago

  • Keywords has-patch added

Sorry, wrong file, only media.php, not general-template.php

#2 @joemcgill
5 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.5
  • Owner set to joemcgill
  • Status changed from new to accepted

Hi @overclokk,

Thanks for the report. The fix in media.php.patch looks good to me.

#3 @overclokk
5 years ago

Hi @joemcgill
you're welcome :-)

#4 @dd32
5 years ago

Under what scenarios would a non-array item be present in the array?

It seems that this could be the wrong fix, and instead it should be checked earlier on in the function somewhere.

#5 @joemcgill
5 years ago

@dd32 It seems as though sizes are registered in the attachment metadata, but the data for each size isn't an array (as expected).

@overclokk could you dump an example of some output from wp_get_attachment_metadata() so we can se what the data looks like when this bug is present?

#6 @ocean90
5 years ago

I've noticed a similar warning at https://make.wordpress.org/flow/2014/05/12/visual-record-front-page-to-new-post-on-an-ipad-air/img_0806/ where for some reasons the meta data is corrupted.

Warning: strpos(): Empty needle in wp-includes/media.php on line 1099

// var_dump( $image_meta );
array (size=5)
  'width' => int 2048
  'height' => int 1536
  'file' => string '/' (length=1) // Oops?!
  'sizes' => 
    array (size=3)
      'thumbnail' => 
        array (size=4)
          'file' => string 'IMG_0806-150x150.png' (length=20)
          'width' => int 150
          'height' => int 150
          'mime-type' => string 'image/png' (length=9)
      'medium' => 
        array (size=4)
          'file' => string 'IMG_0806-300x225.png' (length=20)
          'width' => int 300
          'height' => int 225
          'mime-type' => string 'image/png' (length=9)
      'large' => 
        array (size=4)
          'file' => string 'IMG_0806-1024x768.png' (length=21)
          'width' => int 1024
          'height' => int 768
          'mime-type' => string 'image/png' (length=9)
  'image_meta' => 
    array (size=10)
      'aperture' => int 0
      'credit' => string '' (length=0)
      'camera' => string '' (length=0)
      'caption' => string '' (length=0)
      'created_timestamp' => int 0
      'copyright' => string '' (length=0)
      'focal_length' => int 0
      'iso' => int 0
      'shutter_speed' => int 0
      'title' => string '' (length=0)
// var_dump( $image_sizes );
array (size=4)
  'thumbnail' => 
    array (size=4)
      'file' => string 'IMG_0806-150x150.png' (length=20)
      'width' => int 150
      'height' => int 150
      'mime-type' => string 'image/png' (length=9)
  'medium' => 
    array (size=4)
      'file' => string 'IMG_0806-300x225.png' (length=20)
      'width' => int 300
      'height' => int 225
      'mime-type' => string 'image/png' (length=9)
  'large' => 
    array (size=4)
      'file' => string 'IMG_0806-1024x768.png' (length=21)
      'width' => int 1024
      'height' => int 768
      'mime-type' => string 'image/png' (length=9)
  'full' => 
    array (size=3)
      'width' => int 2048
      'height' => int 1536
      'file' => string '' (length=0)

I'm not sure if wp_calculate_image_srcset() needs to handle this case though.

#7 @overclokk
5 years ago

@dd32 it has to be an array because when is not it returns the errors I wrote above.

@joemcgill the $image variable return an array like this:

array (size=3)
  'file' => string 'planet-pro-40-30-30-crackers-30-g--115x120.jpg' (length=46)
  'width' => string '115' (length=3)
  'height' => string '120' (length=3)

When is not an array it return an empty string.

#8 @ocean90
5 years ago

#35900 was marked as a duplicate.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


5 years ago

#11 @azaozz
5 years ago

  • Keywords needs-patch added; has-patch removed

...for some reasons the meta data is corrupted.
I'm not sure if wp_calculate_image_srcset() needs to handle this case though.

Right. To work, wp_calculate_image_srcset() needs all of the meta. Bypassing the image when some meta is missing or corrupted seems best.

The is_array( $image ) check is good, needs another for the image file name.

Last edited 5 years ago by azaozz (previous) (diff)

#12 @jaspermdegroot
5 years ago

  • Keywords has-patch added; needs-patch removed

35480.patch checks if $image is an array and if file name isn't corrupted (if it contains a . and at least one character in front of it).

Note that in the report of @ocean90 only the file name of the full size image was corrupted, while this patch checks the file name of each image size. I was in doubt whether we should play safe and check all or only check the file name of the full size image until someone reports a bug that the file name of an image size is corrupted.

#13 @jaspermdegroot
5 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

In 35480.1.patch: I changed the file name check so it only checks the name of the original / full size image in the beginning. If it's incorrect we won't get the correct baseurl so we shouldn't continue. Also added a unit test.

#14 @joemcgill
5 years ago

  • Keywords commit added

35480.1.patch Looks good to me, unless there are valid cases we need to handle where the filename would begin with a '.' character.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


5 years ago

@joemcgill
5 years ago

#16 @joemcgill
5 years ago

35480.2.patch uses strlen( $image_meta['file'] ) < 4 to test filenames instead of 1 > strpos( $image_meta['file'], '.' ) so we could support filenames like .123.jpg if needed.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


5 years ago

#18 @azaozz
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 37002:

Responsive images: do not attempt to create srcset when the image meta is missing or corrupted.

Props overclokk, jaspermdegroot, joemcgill.
Fixes #35480.

@joemcgill
5 years ago

#19 @joemcgill
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

35480.3.patch also catches cases where $image_meta['file'] is completely missing. Otherwise, we could still get indexing errors. Also updates unit tests.

#20 @ocean90
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37018:

Responsive images: Skip images with a missing $image_meta['file'] value.

Props joemcgill.
See [37002].
Fixes #35480.

#21 @overclokk
5 years ago

Thank you guys :-)

Note: See TracTickets for help on using tickets.