Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#36587 closed defect (bug) (fixed)

PHPUnit Tests fail with PHP7 and Imagick 3.4.x

Reported by: alpipego's profile alpipego Owned by: kirasong's profile kirasong
Milestone: Priority: high
Severity: normal Version:
Component: Media Keywords: upstream has-patch
Focuses: Cc:

Description

Imagick 3.4.0 started supporting PHP7. Presumably Imagick now throws an ImagickException where it previously issued a warning. This causes the PHPUnit test to throw errors (~290) and ultimately several failures.

This can be reproduced with the latest development version (at the time of writing: (commit:b26f070)) of a "naked" VVV.

At the moment the warnings get surpressed in multiple places, e.g. wp-admin/includes/image.php:347

$exif = @exif_read_data( $file );

This should ultimately be wrapped in a try-catch block, i.e.

try {
    $exif = @exif_read_data( $file );
} catch(Exception $e) {
    $exif = false;
}

Attachments (3)

phpunit.log (116.0 KB) - added by ocean90 8 years ago.
36587.patch (926 bytes) - added by ocean90 8 years ago.
36587.2.patch (974 bytes) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (31)

@ocean90
8 years ago

#1 @ocean90
8 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Priority changed from normal to high

Thanks for creating the ticket @alpipego!

This issue was discovered during the Contributor Day at WordCamp Nuremberg and took quite a while to find the cause. :(

This causes the PHPUnit test to throw errors (~290) and ultimately several failures.

Correct, most of the errors where caused by "headers already sent" warnings as soon as the image tests ran.

ImagickException: Cannot modify header information - headers already sent by (output started at /tests/phpunit/includes/bootstrap.php:56)

phpunit.log includes the output of a broken test run.

While it looks like it's currently only happing with our test image I'm sure that this could be an issue in a real life scenario too.

The change is probably related to http://php.net/manual/en/language.errors.php7.php.

Setup:

$ php -v
PHP 7.0.5 (cli) (built: Apr  1 2016 19:09:49) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies

$ php -i | grep imag
Additional .ini files parsed => /usr/local/etc/php/7.0/conf.d/ext-imagick.ini
imagick
imagick module => enabled
imagick module version => 3.4.1
imagick classes => Imagick, ImagickDraw, ImagickPixel, ImagickPixelIterator, ImagickKernel
Imagick compiled with ImageMagick version => ImageMagick 6.9.3-7 Q16 x86_64 2016-03-27 http://www.imagemagick.org
Imagick using ImageMagick library version => ImageMagick 6.9.3-7 Q16 x86_64 2016-03-27 http://www.imagemagick.org
imagick.locale_fix => 0 => 0
imagick.progress_monitor => 0 => 0
imagick.skip_version_check => 0 => 0
Last edited 8 years ago by ocean90 (previous) (diff)

#2 @dd32
8 years ago

This feels like a bug in the Imagick extension.. Even with the PHP7 exception/error changes that were made, I don't think it was ever intended that a BC break like this would occur.

The changelog doesn't mention this change, and although their documentation and tests covers Imagick() instances throwing exceptions, none of it covers the core PHP functions (and the PHP team doesn't specify exceptions can be thrown).

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


8 years ago

#5 @dd32
8 years ago

After digging through this, and seeing that indeed the methods used here such as exif_read_data() and getimagesize() don't appear to internally use Imagemagick when available (As I initially assumed from the logs),
I'm pretty sure this is a PHPUnit bug, or an issue with PHP Itself.

The actual exception thrown by the phpunit handler is getimagesize(http://localhost/foo-cropped.png): failed to open stream: HTTP request failed! HTTP/1.1 404 Not Found, which is then reported incorrectly. The prefix to the log message on the CLI is the Exception classname.

1) Tests_Image_Header::test_create_attachment_object
ImagickException: getimagesize(http://localhost/foo-cropped.png): failed to open stream: HTTP request failed! HTTP/1.1 404 Not Found

Potentially related is the fact that I keep triggering segmentation faults and OOM errors in PHP whenever I try to diagnose what's happening.

#6 @dd32
8 years ago

Did lots more digging, and is definitely caused by the extension or PHP internals and isn't a WordPress nor PHPUnit bug.

https://github.com/mkoppanen/imagick/issues/147#issuecomment-212297265
I'll copy it here for future reference:

The debug log from that PHPUnit run had a huge number of issues, but unfortunately it's definitely all caused by the Imagick extension (or potentially a bug within PHP itself). The reason the log was so full was that once a Imagick exception is triggered, every PHP warning thereafter is thrown as a ImagickException (where it shouldn't be thrown at all).

Here's a minimal script to reproduce it:

<?php
error_reporting( E_ALL ); ini_set( "display_errors", true );

try {
       $f = new Imagick( 'http://any/url/here' );
} catch( Exception $imagick_exception ) {
       var_dump( compact( 'imagick_exception' ) );
}

getimagesize( "something here" );

What you'd expect to see here is the imagick_exception data followed by a PHP Warning, instead you get the imagick_exception data followed by a PHP Fatal:

Fatal error: Uncaught ImagickException: getimagesize(something here): failed to open stream: No such file or directory

#7 @ocean90
8 years ago

  • Keywords upstream added
  • Version trunk deleted

A fix for the extension was committed yesterday, see https://github.com/mkoppanen/imagick/issues/147#issuecomment-212582207. I compiled the extension and verified that the tests are running correctly with the fix.

Let's leave this ticket open until a new version gets released.

#8 @dd32
8 years ago

The breakage caused by this version of the extension is bad enough, do we want to blacklist these bad versions?

Hopefully usage of it will be few and far between, those who install PHP7 are more likely to keep it updated.. so i'm on the ropes.

@ocean90
8 years ago

#9 follow-up: @ocean90
8 years ago

@dd32 Good idea, haven't thought about that. 36587.patch blacklists version 3.4 - 3.4.2, assuming that the next version is 3.4.2. Haven't added a check for PHP7 but that might sense too.

#10 in reply to: ↑ 9 ; follow-up: @joemcgill
8 years ago

Replying to ocean90:

@dd32 Good idea, haven't thought about that. 36587.patch blacklists version 3.4 - 3.4.2, assuming that the next version is 3.4.2. Haven't added a check for PHP7 but that might sense too.

36587.patch will make Imagick unavailable for anyone using those versions of Imagick, and not just people on PHP7, correct? I don't think we want to remove Imagick support to people who are currently using those versions on non-PHP7 environments.

#11 in reply to: ↑ 10 @ocean90
8 years ago

Replying to joemcgill:

36587.patch will make Imagick unavailable for anyone using those versions of Imagick, and not just people on PHP7, correct?

Correct, thus my comment "Haven't added a check for PHP7 but that might sense too." part.

@ocean90
8 years ago

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


8 years ago

#13 @ocean90
8 years ago

#36643 was marked as a duplicate.

#14 @wonderboymusic
8 years ago

In 37306:

Unit Tests: prevent rewrite spillage from @group author unit tests.

See #36587.

#15 @ocean90
8 years ago

  • Keywords has-patch added

Noting that 3.4.2 with the fix was released on April 25th.

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


8 years ago

#17 @joemcgill
8 years ago

@ocean90 since Imagick only started supporting PHP7 in version 3.4.0, perhaps we should drop the version_compare( $imagick_version, '3.4', '>=' ) part of that check. Otherwise, 36587.2.patch looks good and media tests pass.

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


8 years ago

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


8 years ago

#20 @kirasong
8 years ago

I’m concerned that, presuming I understand the bug properly, and after testing (DH is affected by this), for most users, prior to applying the patch, Imagick will usually work fine for resizing images.

After the patch, WP will stop using Imagick even if it was, from those users' perspectives, working as intended.

I don’t feel strongly against the suggested route, but if we do blacklist the version, we need to notify hosts ASAP so that when 4.6 comes out, they’ve updated already.

Basically, we'd need to be loud about it.

Either way, a post on Make as a notification, followed by outreach to get Imagick upgraded makes sense to me here.

#21 follow-up: @dd32
8 years ago

I’m concerned that, presuming I understand the bug properly, and after testing (DH is affected by this), for most users, prior to applying the patch, Imagick will usually work fine for resizing images.

I agree, however, the extent of breakage of that particular version has with PHP7 is significant if triggered.
Any host which is running PHP7 & Imagick is one which is keeping on top of updates (I would hope) given the rapid release rate of core and extensions - I'm kind of hoping that affected hosts would've upgraded the extension before we actually have to blacklist it.

#22 in reply to: ↑ 21 ; follow-up: @kirasong
8 years ago

Replying to dd32:

I’m concerned that, presuming I understand the bug properly, and after testing (DH is affected by this), for most users, prior to applying the patch, Imagick will usually work fine for resizing images.

I agree, however, the extent of breakage of that particular version has with PHP7 is significant if triggered.

What are the ways in which users are most likely to experience this problem in the real world? (want to be sure I'm not missing cases here)

Any host which is running PHP7 & Imagick is one which is keeping on top of updates (I would hope) given the rapid release rate of core and extensions - I'm kind of hoping that affected hosts would've upgraded the extension before we actually have to blacklist it.

I'd hope this also, *if* we give hosts enough runway/notice to do it. In reality, some, but not all hosts will be upgraded, so I'd like to look at this from the user perspective.

#23 in reply to: ↑ 22 @dd32
8 years ago

Replying to mikeschroder:

What are the ways in which users are most likely to experience this problem in the real world? (want to be sure I'm not missing cases here)

One way of looking at it; It upgrades any PHP Warnings thrown by any code to effectively PHP Fatals. That would be triggered (as in this case) by either malformed exif data (or other plugin issues) where warnings were expected and were explicitly silenced by @.

I'd hope this also, *if* we give hosts enough runway/notice to do it. In reality, some, but not all hosts will be upgraded, so I'd like to look at this from the user perspective.

I firmly believe the user experience of not having a broken response is the primary objective. We shouldn't have to tell hosts to fix broken stuff or that a recently released extension on a brand new PHP branch is faulty - that's their responsibility to keep up to speed on. Our responsibility is to offer a seamless experience for our users, and degrade gracefully where appropriate.

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


8 years ago

#25 @joemcgill
8 years ago

  • Milestone changed from 4.6 to Future Release
  • Owner set to mikeschroder
  • Status changed from new to assigned

I'm punting this out of the current milestone based on our last Slack conversation, but we should keep an eye on it.

It seems like this is a very edgy case, which is reproducible in unit tests, but not during normal operations performed in core. If that's the case, the effect of breaking functionality that has been working in production seems to be worse than the edge case we're trying to avoid.

If @dd32 or @mikeschroder agree that this should be fixed in 4.6, feel free to move back into the milestone.

Last edited 8 years ago by joemcgill (previous) (diff)

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


7 years ago

#27 @joemcgill
7 years ago

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

This seems to be addressed upstream by Imagick. Since there have been no additional reports, I'm going to mark this as fixed.

#28 @netweb
7 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.