WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#41726 closed defect (bug) (fixed)

`media_handle_upload()` may unexpectedly return 0 on error

Reported by: flixos90 Owned by: mrasharirfan
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Media Keywords: good-first-bug has-patch commit
Focuses: Cc:

Description

The documentation of media_handle_upload() states that it will always return the attachment ID on success, or a WP_Error object on failure, which makes sense in my opinion.
However, it currently may return 0 as well if something fails with the attachment post insertion. That is because wp_insert_attachment() and wp_insert_post() do not return WP_Error objects by default, and media_handle_upload() for some reason does not change this like it should.

This requires a simple fix: call wp_insert_attachment() with the fourth parameter set to true.

Attachments (1)

41726.diff (519 bytes) - added by mrasharirfan 2 years ago.
Added extra parameter to return WP_Error object if any error occurs.

Download all attachments as: .zip

Change History (9)

#1 @joemcgill
2 years ago

Thanks @flixos90.

wp_insert_attachment() has only included the fourth parameter for returning a WP_Error as of 4.7.0. It's possible that someone is relying on the current behavior, but it would indeed be undocumented, as you point out. It would make sense for us to make use of the ability to return a WP_Error from wp_insert_attachment() here.

#2 @flixos90
2 years ago

@joemcgill
I wasn't aware this has only been there since 4.7, this would make sense to explain why it's not correctly handled in media_handle_upload().

Regarding BC, I think it should be safe to fix this for the following reasons:

  • People right now need to check is_wp_error() anyway. If they have an extra condition to check whether the return value is 0, that will just be skipped, but it would very likely just be another error handler anyways.
  • It has always been documented to only return WP_Error on errors, as you've pointed out.

Let's move this into 4.9 if you agree.

@mrasharirfan
2 years ago

Added extra parameter to return WP_Error object if any error occurs.

#3 @mrasharirfan
2 years ago

Hi,

I have created a patch to include the fourth parameter to return WP_Error object just in case any error occurs. IMHO, I agree with @flixos90. As the documentation says, that media_handle_upload() returns either ID of the attachment or a WP_Error object on failure, so backward compatibility will not be an issue.

Cheers!

#4 @mrasharirfan
2 years ago

  • Keywords has-patch added; needs-patch removed

#5 @flixos90
2 years ago

  • Owner set to mrasharirfan
  • Status changed from new to assigned

Thanks for the patch @mrasharirfan, looks good!

Marking the ticket as claimed, and we can likely get it into 4.9, unless there are objections about fixing this.

#6 @joemcgill
2 years ago

  • Keywords commit added

Thanks @mrasharirfan and @flixos90. The patch and approach looks good to me.

#7 @joemcgill
2 years ago

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

In 41323:

Media: Ensure 'media_handle_upload()' returns 'WP_Error' on failure.

This fixes an issue where failures when inserting the attachment post via
wp_insert_attachment() would result in a return value of 0 instead of a
WP_Error object, as documented. This is addressed by passing true as the
fourth param (added in WP 4.7.0) when calling wp_insert_attachment().

Props mrasharirfan, flixos90.
Fixes #41726.

#8 @flixos90
2 years ago

  • Milestone changed from Awaiting Review to 4.9
Note: See TracTickets for help on using tickets.