get_default_post_to_edit() function is broken

[tfrommen]

The get_default_post_to_edit() function contains the following code (formatted for the sake of better readability) in case a new post should be inserted to the database:

<?php
$post_id = wp_insert_post( array(
    'post_title' => __( 'Auto Draft' ),
    'post_type' => $post_type,
    'post_status' => 'auto-draft',
) );
$post = get_post( $post_id );

Later in the function, we just work with the $post object.

The problem is, there are se-ve-ral rea-sons under which wp_insert_post() returns 0. In such a case, get_post(), in turn, will return null, which is neither a WP_Post object nor an object at all.

Then, setting a property on this null post will automatically create a new stdClass, resulting in the $post object only having the three explicitly set properties post_content, post_title and post_excerpt.

If you want to see how this affects WordPress, just put $post = null; before wp-admin/includes/post.php:L631.

[SirLouen]

I don't really get the problem.

How can you get wp_insert_post to return 0 with such limited logic?

The only parameter that can be affected is $post_type setting a wrong or unexistant post_type, in which case, even using a non-existent or invalid post_type it will never return 0.

The problem is, there are se-ve-ral rea-sons under which wp_insert_post() returns 0

So despite providing a whole bunch of examples where wp_insert_post this is not particularly affecting this code section.

I'm closing this because its not reproducible under the conditions provided (code-wise). Obviously if we alter the code to fit this, it will be reproducible, but this misses the whole point.

[siliconforks]

Replying to SirLouen:

How can you get wp_insert_post to return 0 with such limited logic?

Isn't it easy to trigger this?

<?php
add_filter( 'wp_insert_post_empty_content', '__return_true' );

It looks like @tfrommen is simply proposing that the return value of wp_insert_post() be checked for errors? That seems like it would be reasonable to me...

[SirLouen]

Reproduction Report

Description

✅ This report validates that the issue can be reproduced.

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.29
• Server: nginx/1.29.1
• Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
• Browser: Chrome 140.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.3
• MU Plugins: None activated
• Plugins:
  • Testing Dolly 1.0.0
  • Test Reports 1.2.0

Testing Instructions

1. Add the filter as shown in this comment
2. Create a new Post
3. 🐞 Bug will be triggered

Actual Results

1. ✅ Error condition occurs (reproduced).

Additional Notes

• Thanks @siliconforks for spotting this. I thought that wp_error was true by default so basically I was checking all the hooks, thinking they should trigger WP_Error in my mind.

• Checking the historic changes it seems that this false was introduced in [49172], #45114 by @peterwilsoncc. I can't see any specific reasoning behind this logic for this specific setting (nor for fire_after_hooks which was also later reverted in [49731])

• So I can also conclude, as @siliconforks suggests, that this might be as simple as calling wp_insert_post with wp_error set to true in get_default_post_to_edit.

[rishabhwp]

Hey @SirLouen,
I added a patch. Could you take a look whenever you get a chance? Thanks!

[siliconforks]

​PR #9681 is not really what I had in mind for addressing this issue. I was thinking it would be best to do what was described in this comment - i.e., try to set the second argument to wp_insert_post() to true in order to (hopefully) get a meaningful error message. (Then you could just call wp_die() with that error message.)

​PR #9681 seems like it is just hiding the problem rather than trying to report it to the user.

[SirLouen]

Replying to siliconforks:

​PR #9681 is not really what I had in mind for addressing this issue. I was thinking it would be best to do what was described in this comment - i.e., try to set the second argument to wp_insert_post() to true in order to (hopefully) get a meaningful error message. (Then you could just call wp_die() with that error message.)

Yep, I think I explained it here. @rishabhwp please review this again. We think its much easier than your proposed solution.

Also I've been thinking if a Unit Test is worthy here, that triggers at least the error. I believe that I will add as a personal "policy" to figure out a unit test if anything can manually tested. In this case is pretty straight forward: adding the hook like commented above, and then checking for the specific WP_Error happening.

​Something like this

[rishabhwp]

Thanks for the feedback! I've updated the PR with the requested changes and also added a corresponding test. Please let me know if I missed anything or if any further changes are needed.

[SirLouen]

Hello @rishabhwp
Looks good

I'm not sure if this part is really necessary:

if ( is_wp_error( $post_id ) ) {
	wp_die( $post_id->get_error_message() );
}

Have you tested with and without this part? Any differences found?

[siliconforks]

Replying to SirLouen:

I'm not sure if this part is really necessary:

if ( is_wp_error( $post_id ) ) {
	wp_die( $post_id->get_error_message() );
}

Don't you need that though? That's the actual check to see whether the wp_insert_post() call failed. Without that, the code is just going to keep running (and presumably crash at some later point).

[SirLouen]

Replying to siliconforks:

Don't you need that though? That's the actual check to see whether the wp_insert_post() call failed. Without that, the code is just going to keep running (and presumably crash at some later point).

Last time I tested without this, it stopped execution with the error (but not sure where in the code exactly was the execution stopped). But to be sincere, I did not check if it continued execution after ​this line, so I wanted to double check.

[rishabhwp]

Without that part, ​the test I added in the PR fails. It also seems more appropriate to stop the execution with a proper message.

[SirLouen]

Replying to rishabhwp:

Without that part, ​the test I added in the PR fails. It also seems more appropriate to stop the execution with a proper message.

Yeah I know that the test fails, because you have adapted your test to that part.
But I would like to hear, if the execution continues ​after this line or not, without that code block. I don't have the machine ready to test it right now, otherwise I will test it later.

[siliconforks]

Well, if you don't have the is_wp_error() check, presumably it's going to crash at some later point - but that may be confusing to someone trying to troubleshoot the problem, because it will be crashing on a line of code that isn't the actual line which caused the error.

It might crash on the very next line (the get_post() line) - but that's still not the line where the original error actually occurred.

The idea of the is_wp_error() check is to detect the error as soon as it occurs, then ​fail fast immediately.

[rishabhwp]

Replying to SirLouen:

if the execution continues ​after this line or not, without that code block.

Yes, the execution proceeds and returns a WP_Post object with an ID of 0:

WP_Post Object
(
    [ID] => 0
    [post_author] => 0
    [post_date] => 0000-00-00 00:00:00
    [post_date_gmt] => 0000-00-00 00:00:00
    [post_content] => 
    [post_title] => 
    [post_excerpt] => 
    [post_status] => publish
    [comment_status] => open
    [ping_status] => open
    [post_password] => 
    [post_name] => 
    [to_ping] => 
    [pinged] => 
    [post_modified] => 0000-00-00 00:00:00
    [post_modified_gmt] => 0000-00-00 00:00:00
    [post_content_filtered] => 
    [post_parent] => 0
    [guid] => 
    [menu_order] => 0
    [post_type] => post
    [post_mime_type] => 
    [comment_count] => 0
    [filter] => raw
    [errors] => Array
        (
            [empty_content] => Array
                (
                    [0] => Content, title, and excerpt are empty.
                )

        )

    [error_data] => Array
        (
        )

)

[SirLouen]

Replying to siliconforks:

The idea of the is_wp_error() check is to detect the error as soon as it occurs, then ​fail fast immediately.

Yes, you are correct. Obviously for compatibility, this is how it is. But I wonder if it would have been better to fail fast directly inside the call, instead of returning the error, and expecting the caller to fail after this. It's like forcing every single call to wp_insert_post (and many other functions), to add a failover afterward forcefully (which has not been done throughout the code as we can see in this report). I suppose that this gives more flexibility to do anything else before failing in case it's needed.

Replying to rishabhwp:

Replying to SirLouen:

if the execution continues ​after this line or not, without that code block.

Great then, I think we are ready to move on here.

[SergeyBiryukov]

In 60701:

Posts, Post Types: Check the result of creating a draft in get_default_post_to_edit().

As wp_insert_post() can return an error for various reasons, this commit ensures that this scenario is properly handled and an error message is displayed.

Follow-up to [12987].

Props rishabhwp, tfrommen, SirLouen, siliconforks, SergeyBiryukov.
Fixes #37441.