WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 4 months ago

#41148 new defect (bug)

Test for ticket 39875 can throw PHP error

Reported by: schlessera Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Media Keywords: has-patch
Focuses: Cc:

Description

The test that was written for #39875 can throw a PHP error under certain circumstances: Undefined index: sizes.

The problem arises when then wp_generate_attachment_metadata() call does not return the proper array data. As the preview path generation directly access a specific array index, this throws a PHP error.

Attachments (1)

41148.patch (927 bytes) - added by schlessera 6 months ago.
Check for valid $metadata array before accessing it

Download all attachments as: .zip

Change History (8)

@schlessera
6 months ago

Check for valid $metadata array before accessing it

#1 @schlessera
6 months ago

  • Keywords has-patch added

#2 @netweb
6 months ago

Related: #40537 PDF tests cannot be tested on Travis

#3 @gitlost
6 months ago

@schlessera there were similar checks in that patch originally but they were removed - see https://core.trac.wordpress.org/ticket/39875#comment:18 - and though I'd tend to add such checks I can see @joemcgill's point that it does bloat the tests...and as @netweb points out the PDF tests need fixing anyway...

#4 @schlessera
6 months ago

I agree that tests should be as lean as possible. But if you put these checks into a different test, you will still have this particular test throw a PHP error instead of failing. I think that tests should never throw errors, they should always only succeed or fail (or be skipped).

The problem with having the test be too complex in this case is an issue with the testability of the code, and not with the test itself.

#5 @gitlost
6 months ago

True enough. Under what circumstances is the PHP error happening?

#6 follow-up: @schlessera
4 months ago

The test error happens when some or all of the dependencies (ghostscript, imagemagick, imagick) are not properly installed. It will result in an empty array being returned by wp_generate_attachment_metadata().

Alternatively to the asserts I added, we could add a check to see whether the proper dependencies are installed, and skip the tests with a corresponding notice if not.

#7 in reply to: ↑ 6 @johnbillion
4 months ago

Related: #40536, #40537.

Replying to schlessera:

Alternatively to the asserts I added, we could add a check to see whether the proper dependencies are installed, and skip the tests with a corresponding notice if not.

There's already a bunch of test skipping in place for other tests that depend on Imagick. The same pattern can be used.

In the long term, I think tests that require Imagick, GS, etc should fail (instead of be skipped) if the dependencies are not installed.

Note: See TracTickets for help on using tickets.