Opened 8 years ago
Closed 8 years ago
#39195 closed defect (bug) (fixed)
Undefined index: extension in class-wp-image-editor-imagick.php on line 152
Reported by: | JaworskiMatt | Owned by: | 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)
Change History (11)
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#5
follow-up:
↓ 6
@
8 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
@
8 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.
#8
follow-up:
↓ 9
@
8 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.
Added ! empty() check for file extension.