WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#32991 new defect (bug)

wp_delete_post() does not return a WP_Post object

Reported by: johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: needs-docs 2nd-opinion has-patch
Focuses: Cc:
PR Number:

Description

The inline doc for wp_delete_post() states that it returns a WP_Post object, but it actually returns a stdClass object (the result of $wpdb->get_row()).

There's most likely a reason that a raw SQL query is performed here rather than using get_post(). It would be interesting to know what that reason is, and to update the code or the docs accordingly.

Anyone want to investigate?

Attachments (1)

32991.patch (2.9 KB) - added by flixos90 4 years ago.
wp_delete_post, wp_delete_attachment, wp_trash_post, wp_untrash_post return WP_Post

Download all attachments as: .zip

Change History (8)

#1 follow-up: @DrewAPicture
4 years ago

  • Milestone changed from Awaiting Review to Future Release

Turns out that the return documentation is actually still incorrect because wp_trash_post() can return a WP_Post object. Should be array|false|stdClass|WP_Post, though once we get above about three or four types it makes a lot more sense to use something like a mixed type for readability (what it used to be prior to [29093]).

That said, I definitely introduced the incorrect return documentation in [29093], not paying close enough attention that we were returning the row directly (looks like a WP_Post object, talks like a WP_Post object, but isn't one).

Don't know that there's any other explanation than that maybe wp_delete_post() was introduced (sometime before [1353]) before get_post() [2478] and never got switched over from wpdb->get_row() to get_post().

For that matter, looks like wp_delete_attachment() also returns the row instead of a WP_Post object though wp_trash_post() returns a WP_Post object.

Seems like we might be safe in switching it now, especially since these two functions seem to be the exception. Also worth noting that clean_post_cache() takes the object through get_post(), so we might as well bring wp_delete_post() and wp_delete_attachment() into the fold.

#2 in reply to: ↑ 1 ; follow-up: @flixos90
4 years ago

Replying to DrewAPicture:

Turns out that the return documentation is actually still incorrect because wp_trash_post() can return a WP_Post object. Should be array|false|stdClass|WP_Post, though once we get above about three or four types it makes a lot more sense to use something like a mixed type for readability (what it used to be prior to [29093]).

Same goes for the documentation of wp_delete_post_revision() where it also says it can return array|false|WP_Post|WP_Error|null. As far as I can see it, the function can only return array|false|stdClass currently (not sure where the WP_Error and null come from).

For that matter, looks like wp_delete_attachment() also returns the row instead of a WP_Post object though wp_trash_post() returns a WP_Post object.

I'm wondering why wp_trash_post() and wp_untrash_post() return a post array, shouldn't they be adjusted to return an actual WP_Post as well to be conform (documentation is incorrect as well, not stating that an array could be returned)? Since the caller of the trash/untrash functions must assume the return value might be a WP_Post anyway, there should not be any compatibility issues I think. This way all those functions would always return a WP_Post, never a post array.

@flixos90
4 years ago

wp_delete_post, wp_delete_attachment, wp_trash_post, wp_untrash_post return WP_Post

#3 @flixos90
4 years ago

This patch adjusts wp_delete_post(), wp_delete_attachment(), wp_trash_post() and wp_untrash_post() to all return a WP_Post object (or null if object cannot be found or false on failure). What do you think, is that an adequate solution?

Another question is whether the null is really needed. We could just return false on any kind of failure (like the documentation states). It would be more straightforward - on the other hand, maybe people use the exact return value to distinguish between the type of error (null always means "post not found").

#4 @flixos90
4 years ago

  • Keywords has-patch added; needs-patch removed

#5 in reply to: ↑ 2 @SergeyBiryukov
4 years ago

Replying to flixos90:

Same goes for the documentation of wp_delete_post_revision() where it also says it can return array|false|WP_Post|WP_Error|null. As far as I can see it, the function can only return array|false|stdClass currently (not sure where the WP_Error and null come from).

wp_delete_post_revision() can return null if wp_get_post_revision() returned null. Not sure where WP_Error comes from.

#6 @wonderboymusic
4 years ago

When deleting items, I think the SQL is to ensure we aren't doing anything destructive based on information from a potentially stale cache.

#7 @flixos90
2 years ago

Can you please double-check @johnbillion @SergeyBiryukov, but as far as I can see this was fixed with [41642]?

Note: See TracTickets for help on using tickets.