wp_delete_auto_drafts passes post ID as string instead of as int

[kkmuffme]

provided type does not match required type according to phpdoc

Trac ticket: https://core.trac.wordpress.org/ticket/63975

[SirLouen]

@kkmuffme how have you detected this?
Are you passing some level of PHPStan/psalm or something?

$old_posts receives an array of post ID, which, according to the database table definition, have to be int (BIGINT) forcefully. So strong typing is good for quality purposes, but code is plagued with these.

What is weirder for me, is the force conversion to an array of $old_posts as get_col is always an array according to the phpdoc definition. But you will know definitely that the array values will be int always. So I would remove the (array) and add the (int) just to force typing, although its not strictly necessary (WP overall is not strongly typed, and common sense among data structures take a lot of place).

For context: This was built in [20453]/#19663, there are no design decisisons of why this (array) or this absence of (int).

[siliconforks]

Replying to SirLouen:

What is weirder for me, is the force conversion to an array of $old_posts as get_col is always an array according to the phpdoc definition.

foreach ( (array) ... is a very common PHP idiom - it is used several hundred times throughout the WordPress codebase:

$ grep 'foreach *( *( *array' -r wordpress-develop/src | wc -l
334

Many people probably write it that way out of habit (even for situations like this one where it does not actually do anything, since $old_posts should always be an array).

[SirLouen]

Replying to siliconforks:

foreach ( (array) ... is a very common PHP idiom - it is used several hundred times throughout the WordPress codebase:

Yeah, but I'm also trying to figure out how this proposed patch popped out and not the array one.

[kkmuffme]

I got this by adding declare( strict_types=1 ); on top of wp-includes/class-wp-hook.php to debug an issue where a plugin used native type hints and accidentally broke a "pre" hook

Anyway, while debugging, cron suddenly report a fatal TypeError for a method with type hint "int" on the delete action, since it received a string, even though it expected an int.

[SirLouen]

Replying to kkmuffme:

Anyway, while debugging, cron suddenly report a fatal TypeError for a method with type hint "int" on the delete action, since it received a string, even though it expected an int.

For full correctness, I would remove (array) as it is an extra unnecessary check and add the (int) as I said. But WP is not strongly typed, and obviously strict typing reports are not relevant unless real risks have been observed.

But I also know that @SergeyBiryukov loves this kind of patches, so I'm pinging him in case he wants to merge it :)

[westonruter]

Is casting to an int correct? When it is a string, what value does it have? If it isn't an int then it should just be skipped rather than attempting to delete a post with the ID of 0.

[kkmuffme]

Replying to SirLouen:

$old_posts receives an array of post ID, which, according to the database table definition, have to be int (BIGINT) forcefully. So strong typing is good for quality purposes, but code is plagued with these.

This is wrong. INT (or BIGINT) is always a (numeric-)string when using mysql in PHP on unix.

Replying to westonruter:

Is casting to an int correct? When it is a string, what value does it have? If it isn't an int then it should just be skipped rather than attempting to delete a post with the ID of 0.

Yes, bc this is how mysql drivers in PHP work. By (mysql table) definition it can only be a numeric string.

[SirLouen]

Replying to kkmuffme:

This is wrong. INT (or BIGINT) is always a (numeric-)string when using mysql in PHP on unix.

It's technically wrong (hence your strict typing alert or probably most static analyzers), but not conceptually wrong given that this is only going to receive an array of a very particular case of numerical strings that happen to be simplistically incremental and mandatory BIGINT IDs, so it's impossible that this can possibly fail.

Being sincere, I'm sure of what the general opinion is on these topics; there are probably a couple hundred like this (and they will keep rising as this is not strongly typed). I'm going to raise it in the next dev chat, would like to hear from others.

[kkmuffme]

Replying to SirLouen:

Replying to kkmuffme:

This is wrong. INT (or BIGINT) is always a (numeric-)string when using mysql in PHP on unix.

It's technically wrong (hence your strict typing alert or probably most static analyzers), but not conceptually wrong given that this is only going to receive an array of a very particular case of numerical strings that happen to be simplistically incremental and mandatory BIGINT IDs, so it's impossible that this can possibly fail.

Conceptually is irrelevant though, if this results in a fatal error, if some do_action in the delete function are documented with @param int but then provided a string type.
It might not be the do_action callback itself (since WP doesn't have strict_types set), but if the callback function calls another function, which expects an int, then you'll have a fatal too.

Being sincere, I'm sure of what the general opinion is on these topics; there are probably a couple hundred like this (and they will keep rising as this is not strongly typed). I'm going to raise it in the next dev chat, would like to hear from others.

With PHP natively supporting most phpdoc types as native type hints, starting with PHP 7.1 but essentially full support with PHP 8.3+, phpdoc annotations will become less relevant and type hints more common.
Therefore this is something that should be addressed and fixed in WP core to ensure that at least action/filter annotations are correct and the type specified is actually the type provided.

From my tests, there don't seem to be 100s like those, but only very few action/filter annotations where it's actually wrong, and I also don't think it's necessary to fix them all at once but just as they get reported.

[westonruter]

I can confirm that $wpdb->get_col( "SELECT ID FROM $wpdb->posts" ) returns string[], so it does make sense to cast this to an int.

Nevertheless, I'm not sure where the fatal error is coming from.

The wp_delete_post() function doesn't have any PHP type hint at all.

The function signature is

<?php
function wp_delete_post( $post_id = 0, $force_delete = false ) {}

As I understand, a fatal error would only occur if strict types were declared and the function signature was:

<?php
function wp_delete_post( int $post_id = 0, bool $force_delete = false ) {}

However, given the current lack of any PHP type hints, I don't see how a fatal error would occur at present. PHP doesn't apply type hints defined in phpdoc. See ​https://3v4l.org/0L4Ru

[SirLouen]

Replying to kkmuffme:

Therefore this is something that should be addressed and fixed in WP core to ensure that at least action/filter annotations are correct and the type specified is actually the type provided.

I'm going to try to bring up this discussion today.

But from my perspective, to accept these kinds of theoretical reports, I would seriously ask for a reproduction use case or close them as worksforme if it cannot be proven.

I know that in theory, typing makes a lot of sense

But demonstrating this should be a thing.
How to demonstrate this? A little plugin that triggers the error without the static cast.

[westonruter]

We discussed in core dev chat. I suggest we close this in favor of holistically addressing such static analysis issues with the adoption of PHPStan (#61175), specifically when increasing the ​rule levels which will automatically detect issues like this. They can then be fixed all together without thousands of one-off tickets and commits.

[SirLouen]

Now that I have a good protocol, I can easily triage through this type of ticket faster.

Before fully closing, I would wait to see if @kkmuffme can provide a reproduction example, to see how he can trigger this fatal error in a real WP environment. At first sight it doesn't seem to be possible, but who knows if there is an interesting edge case.

Also, I'm not sure if PHPStan will trigger this with the expected levels we are going to probably add at first. I'm going to track this report for a couple of weeks. .

[kkmuffme]

Sorry guys, you're both WP core contributors and I have to explain PHP basics we've covered in static analysis almost 10 years ago and with php-src type hints for string/int type with strict types also more than half a decade at this point.

Then claiming

these kinds of theoretical reports

just is the cherry on top.
The gatekeeping in WP core is worse than in many corporate jobs at this point and is quite different from what my experience with WP core in the past.
I guess that's a side-effect of having to check that pineapple is delicious on pizza when logging in, ensuring that sanity stays out the door (just kidding)

Anyway, here's a simple reproduction - which in fact you can use for all future reports reporting similar issue. Because the reproduction will always be the same, no matter the hook. You only need to change the type hint.
As you can see, this has nothing to do with static analysis.

<?php
declare(strict_types=1);
function foo( $post_id ) {
    bar( $post_id );
}
add_action( 'before_delete_post', 'foo' );

function bar( int $post_id ): void {}

wp_delete_post( '1' );

For non-scalar type hints, this is a fatal even without strict_types=1

[SirLouen]

I knew you were going to answer something like this and then put a reference to php-src (instead of PHP in general)

I'm unable to reproduce this Fatal error with the code you provided.

<?php
/*
Plugin Name: Testing
*/
function foo( $post_id ) {
    bar( $post_id );
}
function bar( int $post_id ): void {
    var_dump( 'bar' );
}
function maybe_delete_post() {
    if ( isset( $_GET['foo'] ) && $_GET['foo'] === 'bar' ) {
        add_action( 'before_delete_post', 'foo' );
        wp_delete_post( '12', true );
    }
}
add_action( 'init', 'maybe_delete_post' ); 

You have to understand one important thing. Even if I were able to reproduce the example you provided, it is impossible in real-life scenarios. Basically these are those edge cases that are great for academic purposes but should never happen. There are zillions of examples like this in Trac, you can browse for them.

Put it this way: Imagine that instead of just displaying your theoretical input, you file a ticket like this:

Title: Fatal error when trying to delete a post

I'm trying to do a plugin and when I add this code:

<?php
declare(strict_types=1);
function foo( $post_id ) {
    bar( $post_id );
}
add_action( 'before_delete_post', 'foo' );
function bar( int $post_id ): void {}
wp_delete_post( '1' );

I get a fatal error. I attach a patch to solve this problem

What kind of answers do you think you will receive?

I guarantee you that any single contributor will answer something like: "You are not doing this correctly, you should do this and that". Even if it's technically PHP correct code, you are doing things in an unexpected way or just for the sake of making things fail.

I've worked and even reported a couple tickets like this here in WP but I still believe they are a waste. Nevertheless, I'm not against solving them, but as Weston commented, not doing them one by one because there are thousands of confirmed real-life bugs that are not receiving attention, and resources, unfortunately, are substantially limited.

One of the proposals that was raised during the meeting yesterday was moving these kinds of tickets to the PHPStan task, which is slowly but steadily working to solve these just for the sake of correctness.

My best suggestion here: Create a new PR with the same code and link the new PR to this ticket #63268
And comment here once you have done, to close this ticket.

PS: Remove that (array) static cast if you send the PR.

[kkmuffme]

Replying to SirLouen:

I'm unable to reproduce this Fatal error with the code you provided.

Because you're not using the code I provided :-)
Can you go ahead and actually try the code I provided - including the declare(strict_types=1); ?

I'll wait with replying the rest of your response until then, just in case you want to edit it, since some claims ("it is impossible in real-life scenarios") show a complete lack of understanding of PHP's type system.

[SirLouen]

Replying to kkmuffme:

Can you go ahead and actually try the code I provided - including the declare(strict_types=1); ?

​Check this

as a lot of WordPress Core relies on the loose type nature of PHP, so this is not something to be undertaken lightly and is not on the agenda for the foreseeable future

[kkmuffme]

So, were you able to reproduce it?

Checked it, it says:

Strict typing should not be used in WordPress Core at this time.
as a lot of WordPress Core relies on the loose type nature of PHP

Totally agree, but the code that will result in a fatal is not in WordPress Core. My code is from a theme or a plugin. What if you need to use a library/composer package, that uses strict_types=1?

Also it's not a problem to use lose types. They just should be documented correctly then in the phpdoc, e.g. int|string (or int|numeric-string if one wants to be more specific)

[SirLouen]

Replying to kkmuffme:

Totally agree, but the code that will result in a fatal is not in WordPress Core. My code is from a theme or a plugin. What if you need to use a library/composer package, that uses strict_types=1?

We have to assume that this is something transitional. If you should not use it in core, you should not use it in plugins. You can do it, but unexpected things may arise. Remember that I said to you in the first message that I was not going to take strong typing into consideration, because code is plagued with this (the ones you can grep plus the ones I've manually encountered over the months that are countless).

Like I said, my recommendation is to move your PR where I suggested to you: #63268.
This has been the approved way to proceed in this scenario.

Be aware, that I'm not against you or your type proposal particularly. I was against of having to deal with a ton of trivial reports individually without a nice SOP to direct users to the right place.
Now that I have figured this out, I'm good to move forward.

Furthermore, there has been a lot of discussion on how to “upgrade” WP to be more consistent with this particular kind of "typing" issue when filters can be on the way.

Check
https://core.trac.wordpress.org/ticket/51525
​https://peterwilson.cc/type-declarations-with-wordpress-filters/
(and even @westonruter suggested adding this to the 6.9 roadmap as far as I can remember)

We are not foreign to this problem. In fact, one of the first things that came to my mind when I was doing docs review in the bbPress plugin was the fact that hooks had a weak typing protection because returns could become excessively unpredictable. This particular report is like a grain of sand of all the issues I've found because of this.

[westonruter]

@kkmuffme Thank you for providing a test case that would cause the fatal error.

You suggested making this change to wp_delete_auto_drafts():

- wp_delete_post( $delete, true );
+ wp_delete_post( (int) $delete, true );

But to actually fix the problem in a more robust way, any such casting should be done in wp_delete_post() instead where do_action() is called:

- do_action( 'before_delete_post', $post_id, $post );
+ do_action( 'before_delete_post', (int) $post_id, $post );

Or, even better, to coerce the $post_id at the top of the function so that all usages thereafter for all filters and actions will get the expected type passed:

  $post = get_post( $post );
+ $post_id = $post->ID;

This will then ensure that int is passed to the delete_post_{$post->post_type}, delete_post, deleted_post_{$post->post_type}, deleted_post, and after_delete_post actions. Then we wouldn't have ensure the proper type being passed into wp_delete_post() every time it is called. We only update it once in wp_delete_post() itself.

The issue here is somewhat distinct from the related issues with the data types being passed into filters, per #51525 and Peter's ​blog post, where the initial value in the filter callback cannot be strictly typed because themes and plugins can do bad things and return arbitrary values. I similarly advocate for a ​convention to used mixed as the type for the initial arg passed to a filter callback, for example:

<?php
/**
 * Filters foo.
 *
 * @param string|mixed $foo Foo.
 * @return string Foo.
 */
function filter_foo( mixed $foo, int $post_id ): string {
        if ( ! is_string( $foo ) ) {
                $foo = '';
        }
        /**
         * Because plugins do bad things.
         *
         * @var string $foo
         */

         // Filtering logic goes here.

         return $foo;
}
add_filter( 'foo', 'filter_foo', 10, 2 );

But the issue here are actions not filters. Even in my example here, you see I have int $post_id because I'm expecting that the phpdoc which documents this hypothetical filter foo is correct in that an int is always passed. This then also applies to all arguments passed to actions, which cannot have their values manipulated by callbacks. The phpdoc for the action should be a contract for what value is expected when the arguments are passed to the callback.

I support this change to ensure that $post_id gets programmatically coerced to int at the top of wp_delete_post(), if you want to prepare a patch.

[SirLouen]

Replying to westonruter:

I support this change to ensure that $post_id gets programmatically coerced to int at the top of wp_delete_post(), if you want to prepare a patch.

I was ranting in my head, thinking, "If we do this to this function, then we have to do this with many functions, lets try to show an example". And I did a search in post.php for any other function with an action hook, and got to stick_post and here is what I found:
​https://github.com/SirLouen/wordpress-develop/blob/d769c6608dbe3ed56478df6b9f8c4fe063fe25f0/src/wp-includes/post.php#L3303

It seems, that @SergeyBiryukov (who else could have been?, as I said, he is ultimately the king of these things), in [47557], [47550] already had this same idea (but forgot some functions like this one) to solve WordPress.PHP.StrictInArray.MissingTrueStrict

But the issue here are actions not filters

Nah, I just used these as an example because I wanted to illustrate that there are actually some proposals towards better typing (despite that WP has been almost completely typeless), and this was the most similar example I could find. He seemed angry because we were ignoring another typing request, and I would rather not imply that it was something personal. So I wanted to leave this clear with such a (weak) example. :)

[SirLouen]

@kkmuffme please send a ping here when you have updated the PR with the proposed changes.

Just to avoid this stalling and push it forward, I'm pushing the final patch here.

[SirLouen]

Testing Instructions

Not straightforward to test, but simply you can use any form of code similar to the one provided here. Without the patch, it should throw a typing error; with the patch, it shouldn't anymore.

[westonruter]

I'm just realizing the phpdoc for the $post_id param says "Optional. Post ID. Default 0." This actually is NOT optional. If a $post_id of 0 is passed, then the SQL query will look for a wp_posts row with an ID of 0 which should never happen. I think the phpdoc should be updated to say:

 * @param int  $post_id      Post ID.

The default value of 0 should still be kept for the $post_id param, just in case there is any plugin not supplying a value. I can't imagine why this function would have allowed the global to be used as the default, but maybe it used to be the case.

I'm not really seeing any good unit test coverage for this wp_delete_post() function. This seems like a good time to add it.

@SirLouen Do you want to do so?

See ​PR review.

[westonruter]

For reference, this wp_delete_post() function ​looked like this back in WP 1.5.0:

<?php
function wp_delete_post($postid = 0) {
        global $wpdb;
        $postid = (int) $postid;

        if ( !$post = $wpdb->get_row("SELECT * FROM $wpdb->posts WHERE ID = $postid") )
                return $post;

        if ( 'static' == $post->post_status )
                $wpdb->query("UPDATE $wpdb->posts SET post_parent = $post->post_parent WHERE post_parent = $postid AND post_status = 'static'");

        $wpdb->query("DELETE FROM $wpdb->posts WHERE ID = $postid");
        
        $wpdb->query("DELETE FROM $wpdb->comments WHERE comment_post_ID = $postid");

        $wpdb->query("DELETE FROM $wpdb->post2cat WHERE post_id = $postid");

        $wpdb->query("DELETE FROM $wpdb->postmeta WHERE post_id = $postid");
        
        return $post;
}

Even back then it had a default value of 0 for $postid, but it would never succeed back then either because it doesn't supply the global $post for the post ID when 0 is supplied.

Interestingly, back then it did have a cast to (int).

[SirLouen]

Replying to westonruter:

Interestingly, back then it did have a cast to (int).

And what was the reasoning to remove that cast back in the day?

@SirLouen Do you want to do so?

Let me check my backlog, but I remember I already have this in another pending PR or was another function?

EDIT: Nope, it was wp_update_post in #62468 I did an overhaul to move everything to an independent file and start adding specific unit tests for that.

Ok, let's do the same for wp_delete_post it has been forgotten, I also think there are a couple tests that can be reunified here.

@westonruter I've added some tests. There are a million tests we could add, I think this merits a sole blessed task through 6.9 and 7.0 to keep adding tests regularly.

[sajjad67]

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/10061

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 1.0.0
  • Test Reports 1.2.0

Before Patch

Fatal error: Uncaught TypeError: bar(): Argument #1 ($post_id) must be of type int, string given, called in /var/www/src/wp-content/plugins/testing.php on line 25 and defined in /var/www/src/wp-content/plugins/testing.php:29 Stack trace: #0 /var/www/src/wp-content/plugins/testing.php(25): bar('9') #1 /var/www/src/wp-includes/class-wp-hook.php(326): foo('9') #2 /var/www/src/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array) #3 /var/www/src/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #4 /var/www/src/wp-includes/post.php(3798): do_action('before_delete_p...', '9', Object(WP_Post)) #5 /var/www/src/wp-content/plugins/testing.php(34): wp_delete_post('9', true) #6 /var/www/src/wp-includes/class-wp-hook.php(324): maybe_delete_post('') #7 /var/www/src/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array) #8 /var/www/src/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #9 /var/www/src/wp-settings.php(730): do_action('init') #10 /var/www/wp-config.php(107): require_once('/var/www/src/wp...') #11 /var/www/src/wp-load.php(55): require_once('/var/www/wp-con...') #12 /var/www/src/wp-admin/admin.php(35): require_once('/var/www/src/wp...') #13 /var/www/src/wp-admin/_index.php(10): require_once('/var/www/src/wp...') #14 /var/www/src/wp-admin/index.php(10): require_once('/var/www/src/wp...') #15 {main} thrown in /var/www/src/wp-content/plugins/testing.php on line 29
There has been a critical error on this website. Please check your site admin email inbox for instructions. If you continue to have problems, please try the support forums.

Learn more about troubleshooting WordPress.

After Patch

1. ✅ Issue resolved with patch.

Calling wp_delete_post with no parameters

Notice: Function wp_delete_post was called incorrectly. The post ID must be greater than 0.
Please see Debugging in WordPress for more information.
(This message was added in version 6.9.0.) in /var/www/src/wp-includes/functions.php on line 6131

[SirLouen]

It's a good time to move this into 6.9.
Pretty much ready, maybe a last review by @westonruter to put the icing in the cake.

Ok, added back the =0 looks omega weird to me, and it's double-weird that someone could call a bare wp_delete_post() and potentially receive the fatal error. We are losing an opportunity to fix this, but 🤷.

[fakhriaz]

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested:https://github.com/WordPress/wordpress-develop/pull/10061

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: Linux
• Theme: Twenty Twenty-Five 1.3
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0
  • Trac 63975 Simple Type Test 1.0

Actual Results

1. ✅ Issue resolved with patch.

Additional Notes

• I used this code: ​https://codefile.io/f/WgbcZLQQ8d

Supplemental Artifacts

Link of the error image here:
​https://ibb.co/bRv7XKzk

[fakhriaz]

63975 fakhriaz

[westonruter]

While WP 1.5.0 included the cast:

$postid = (int) $postid;

And I found that it ended up getting removed in r6180 (​11e69b6) to fix #4553 as part of WP 2.5.0.

  function wp_delete_post($postid = 0) {
      global $wpdb, $wp_rewrite;
-     $postid = (int) $postid;
- 
-     if ( !$post = $wpdb->get_row("SELECT * FROM $wpdb->posts WHERE ID = $postid") )
+     if ( !$post = $wpdb->get_row($wpdb->prepare("SELECT * FROM $wpdb->posts WHERE ID = %d", $postid)) )
          return $post;

It looks like previously the casting to int was used as a way to construct safe SQL before $wpdb->prepare() was available, which has %d that also does this casting.

[SirLouen]

Replying to westonruter:

It looks like previously the casting to int was used as a way to construct safe SQL before $wpdb->prepare() was available, which has %d that also does this casting.

Yes, it appears that they realized that it was not necessary anymore for their use case

We can say that this patch is ready to move on.

[westonruter]

In 60906:

Posts, Post Types: Short-circuit wp_post_delete() when $post_id arg is not above zero after casting to int.

The casting to int ensures that the action callbacks for post deletion can safely use the int type hint for the $post_id argument, as otherwise a fatal error occurs when an integer string is passed. This function also originally had casting of the argument to an integer, going back to at least WP 1.5.0, since it was passed directly into an SQL query. The casting was removed in [6180] with the introduction of prepared SQL statements.

The wp_delete_post() function had $post_id = 0 defined as its argument, also going back at least to WP 1.5.0, perhaps as a way to indicate the type of the argument as being an integer before there was PHPDoc. Unlike with functions like get_post() which have $post = null as the default argument to fall back to getting the global post, no such fallback logic was added to wp_delete_post(), meaning that passing no argument would always result in a DB query to locate the post with an ID of 0, which will never happen. So this introduces a _doing_it_wrong() in case 0 is passed, and yet the default value of 0 is not removed from the function signature to not introduce a fatal error in case any existing code is not supplying the $post_id parameter (however unlikely this may be).

Unit tests have been fleshed out for wp_delete_post() to add coverage for what was previously missing.

Props SirLouen, kkmuffme, fakhriaz, sajjad67, siliconforks, peterwilsoncc, westonruter.
Fixes #63975.