Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#39195 closed defect (bug) (fixed)

Undefined index: extension in class-wp-image-editor-imagick.php on line 152

Reported by: jaworskimatt's profile JaworskiMatt Owned by: joemcgill's profile joemcgill
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch fixed-major
Focuses: Cc:

Description

This is a follow-up to #37608.

The EXACT SAME ERROR was just introduced with WordPress 4.7 in class-wp-image-editor-imagick.php line 152 and is now generating notices all over my development instance when uploading images via a plugin I'm developing

try {
    $this->image = new Imagick();
    $file_parts = pathinfo( $this->file );
    $filename = $this->file;

    if ( 'pdf' == strtolower( $file_parts['extension'] ) ) {
    $filename = $this->pdf_setup();
}

The $info variable is created using PHP's very own pathinfo() like this:

$info = pathinfo( $this->file );
$dir  = $info['dirname'];
$ext  = $info['extension'];

According to PHP documentation, the extension index can be missing (not set) if the file does not have one.

http://php.net/manual/en/function.pathinfo.php

The code above will obviously generate a notice in such a scenario. An easy way around this is NOT to assume the "extension" index is present, for example like this:

$dir  = pathinfo( $this->file, PATHINFO_DIRNAME );
$ext  = pathinfo( $this->file, PATHINFO_EXTENSION );

Attachments (1)

39195.patch (565 bytes) - added by chandrapatel 7 years ago.
Added ! empty() check for file extension.

Download all attachments as: .zip

Change History (11)

#1 @swissspidy
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7.1

@chandrapatel
7 years ago

Added ! empty() check for file extension.

#2 @chandrapatel
7 years ago

Hi @swissspidy

I have added patch. Please check and let me know if its fine.

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


7 years ago

#4 @swissspidy
7 years ago

  • Keywords has-patch added; needs-patch removed

Pinging @joemcgill for feedback

#5 follow-up: @joemcgill
7 years ago

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

Thanks all. 39195.patch looks like a straightforward way to fix this issue. Long-term, I wonder if we would want to do some mime/type checking here instead of relying only on the existence of a '.pdf' extension on the file itself? Either way, we can go with this approach for 4.7.1 and consider a mime check as an enhancement.

@chandrapatel would you mind adding a unit test for this bug to make sure we don't reintroduce it in the future?

#6 in reply to: ↑ 5 @chandrapatel
7 years ago

Replying to joemcgill:

Thanks all. 39195.patch looks like a straightforward way to fix this issue. Long-term, I wonder if we would want to do some mime/type checking here instead of relying only on the existence of a '.pdf' extension on the file itself? Either way, we can go with this approach for 4.7.1 and consider a mime check as an enhancement.

@chandrapatel would you mind adding a unit test for this bug to make sure we don't reintroduce it in the future?

Hello @joemcgill

Thanks for your response. I will try to add unit test for this.

#7 @dd32
7 years ago

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

In 39580:

PDF Images: Avoid a PHP Warning when attempting to process a file without an extension.

Props chandrapatel for initial patch.
Fixes #39195.

#8 follow-up: @dd32
7 years ago

  • Keywords fixed-major added; needs-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

re-opening for 4.7.1.

Sorry @chandrapatel I guess tests are no longer needed :) Please review [39580] and let me know if you see any issues with that approach though.

#9 in reply to: ↑ 8 @chandrapatel
7 years ago

Replying to dd32:

re-opening for 4.7.1.

Sorry @chandrapatel I guess tests are no longer needed :) Please review [39580] and let me know if you see any issues with that approach though.

Hi @dd32

Okay. That solution looks good to me. Thanks.

#10 @dd32
7 years ago

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

In 39607:

PDF Images: Avoid a PHP Warning when attempting to process a file without an extension.

Props chandrapatel for initial patch.
Merges [39580] to the 4.7 branch.
Fixes #39195.

Note: See TracTickets for help on using tickets.