humanmade / S3-Uploads

The WordPress Plugin to Store Uploads on Amazon S3
1.94k stars 391 forks source link

Significant Media Upload Issues after 5.7 upgrade #496

Open rpelletierrefocus opened 3 years ago

rpelletierrefocus commented 3 years ago

Wordpress was updated to 5.7 last night and since then Media Uploads are all over the place.

Sometimes the file will not upload at all.

Sometimes the file appears to duplicate itself.

Sometimes the files even seem to disappear.

We have been working directly with Automattic/Newspack on this issue without being able to track down the problem.

claudiulodro commented 3 years ago

Investigating into this further, it looks like it's this new, second $imageinfo param that was added to @getimagesize in WP 5.7..

When that parameter is present e.g. @getimagesize( 's3:// foo', $imageinfo ) returns false.

When that parameter is not present e.g. @getimagesize( 's3://foo' ) returns the image info correctly.

This is interesting behavior.

For background, in WP 5.6 the image functions directly called @getimagesize, but in WP 5.7 all those were updated to use wp_getimagesize.

claudiulodro commented 3 years ago

Upon even further investigation, I believe the problem is this issue with images exported from Photoshop. Not sure why it only happens when $imageinfo is present, possibly because it doesn't need to parse the image headers as much when the param isn't present.

terriann commented 3 years ago

My colleagues and I were able to reproduce a similar issue with this function and reported it in Trac earlier today https://core.trac.wordpress.org/ticket/52826

In our testing, the bug seemed to primarily impact images with large amounts of EXIF data.

claudiulodro commented 3 years ago

I agree: it's an issue with EXIF data. When running the following code, the issue doesn't happen any more:

/**
 * Strip out EXIF from image uploads.
 * This will prevent issues when using S3 and uploading images which
 * have been edited with Photoshop.
 *
 * @param array $file Uploaded file info.
 * @return array $file
 */
add_action( 'wp_handle_upload_prefilter', function( $file ) {
    if ( 'image/jpeg' === $file['type'] ) {
        $tmp_file = $file['tmp_name'];
        $cleaned_img = imagecreatefromjpeg( $tmp_file );
        imagejpeg( $cleaned_img, $tmp_file, 100 );
        imagedestroy( $cleaned_img );
    }

    return $file;
} );

Just stripping out EXIF data like this snippet does isn't ideal though, because there is legitimately some useful EXIF data which WordPress pulls in and populates on attachments (title, description, etc.).

DoomyDoomer commented 3 years ago

I agree: it's an issue with EXIF data. When running the following code, the issue doesn't happen any more:

/**
 * Strip out EXIF from image uploads.
 * This will prevent issues when using S3 and uploading images which
 * have been edited with Photoshop.
 *
 * @param array $file Uploaded file info.
 * @return array $file
 */
add_action( 'wp_handle_upload_prefilter', function( $file ) {
  if ( 'image/jpeg' === $file['type'] ) {
      $tmp_file = $file['tmp_name'];
      $cleaned_img = imagecreatefromjpeg( $tmp_file );
      imagejpeg( $cleaned_img, $tmp_file, 100 );
      imagedestroy( $cleaned_img );
  }

  return $file;
} );

Just stripping out EXIF data like this snipped does isn't ideal though, because there is legitimately some useful EXIF data which WordPress pulls in and populates on attachments (title, description, etc.).

Appreciate this work around. I can confirm it works on my website and the thumbnail generation is working again. The only issues is images come through upside down.

claudiulodro commented 3 years ago

Looks like it'll be fixed in WP core, following the above Trac ticket: https://core.trac.wordpress.org/ticket/52826 That should fix the issue in theory. Thanks for reporting it there @terriann!

terriann commented 3 years ago

The only issues is images come through upside down.

@DoomyDoomer with the image you're seeing that behavior, is orientation defined in the EXIF? Digital devices with orientation sensors will often use the EXIF to define the orientation. Exif - Wikipedia

DoomyDoomer commented 3 years ago

The only issues is images come through upside down.

@DoomyDoomer with the image you're seeing that behavior, is orientation defined in the EXIF? Digital devices with orientation sensors will often use the EXIF to define the orientation. Exif - Wikipedia

Yes, that's exactly what it is. Sorry, I didn't mean to come off as I wasn't sure why this was happening. I've actually dealt with this in Android App development years ago.