`media_handle_upload()` may unexpectedly return 0 on error

[flixos90]

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.

[joemcgill]

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.

[flixos90]

@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]

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

[mrasharirfan]

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!

[flixos90]

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.

[joemcgill]

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

[joemcgill]

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.