Reassigning posts while deleting a user can result in lost posts if the target user does not exist

[franz-josef-kaiser]

Feature Request

• Yes, I reviewed the contribution guidelines.

Describe your use case and the problem you are facing

When deleting a user, associated posts get deleted as well. If one uses the --reassign=<user-ID>, the soon-to-be-deleted users posts get assigned to the user whose ID gets passed with the --reassign flag. The problem is that if the user ID points to a non-existing user, the posts are lost to a non-existing user.

Describe the solution you'd like

A possible fix would be to check if the user that should get the posts assigned does not exist, an exception gets thrown, the command does not proceed and the user gets informed that the target user does not exist. Maybe the cli could even tell to re-check the list of possible target users by using wp users list.

Addendum

The "fault" here is not WP CLI (link to source), but WordPress core command wp_delete_user() in its current 6.4 version:

# source: wp_delete_user()
$wpdb->update( $wpdb->posts, array( 'post_author' => $reassign ), array( 'post_author' => $id ) );

WordPress does not perform a check as it assumes that the user was properly pre-selected. This is something that can, due to the absence of an GUI that might stop without a form value somewhere, hardly be assumed in an CLI environment.

A possible Solution might be to utilize the WP_User::get_data_by() method:

} else {
+ 	if ( ! WP_User::get_data_by( 'id', $reassign ) {
+ 		return ['error', $message = "No user with ID `{$user_id}` found.\nTry using `wp users list` to identify the correct user." ]
+ 	}
	$result  = wp_delete_user( $user_id, $reassign );
	$message = "Removed user {$user_id} from " . home_url() . '.';
}

[danielbachhuber]

Thanks for the report, @franz-josef-kaiser

Thinking out loud..

It's not so much that the data was lost though, right? The post_author simply ends up with a bad value, which you'd have to update with a SQL query.

It looks like the internals of wp_delete_user() haven't changed in years, so this isn't a new problem. However, the REST API does a confidence check for this scenario.

I think I'm indifferent on the proposed change. On one hand, it would be a nice thing to have. On the other hand, it could be considered a breaking change.

Can you share a bit more detail about how you ran into this issue?

[franz-josef-kaiser]

Thanks for your thoughts, @danielbachhuber .

The situation that got me into this: We just had to quickly take over some WP sites. As the first thing I needed to change was getting rid of previous users and reassigning their posts, I ran through the cmd site by site. Now the thing is that all sites shared the same set of basic users, which seem to have been added in the same order and thus all had the same IDs. The new users that I reassigned the posts to came much later and therefore had different IDs on all sites. Out of convenience, I used the CLI history and repeated the CMD, but changed only the IDs. The point where I ran into a problem was when I hit one entry too far up in history and therefore assigned all posts to the wrong user as I was targeting a similar named, but completely different site where the target user did not exist at all.
I then was facing the challenge of searching up all posts manually and changing them via SQL directly, writing a quick and dirty custom command or plugin or relying on a backup and just repeating the user deletions. End of story: I went with the backup as everything else would have been too much work. As the command didn't throw an error because of the missing checks, I did not notice my mistake.

Your comparison to the WP Rest API is a good one and fits the narrative of

"perform a check as there's no GUI that would hold you back or give you feedback".

I think you are making a pretty good point with this example.

If you can point me somewhere that explains how you add unit tests(?) here and/ or assist in a PR, I am happy to file one (more leaning into the direction of what the REST API is doing). Thanks.

Edit About the internals of the core function: We both now that there have been valid, open and unmerged patches on track for more than a decade. Even if they had undisputed comments and perfect users stories, they often enough just are a lost cause. I am pretty sure we both got some open as well. I do not invest a second into changing core since years and I do not see core behavior as a reason to do something or not.

[danielbachhuber]

The situation that got me into this:

Thanks for sharing this detail!

If you can point me somewhere that explains how you add unit tests(?) here and/ or assist in a PR, I am happy to file one (more leaning into the direction of what the REST API is doing). Thanks.

Sure, here is some guidance on our pull request best practices.

[ernilambar]

We already ask for confirmation if reassign parameter is missing.

if ( ! $reassign ) {
  WP_CLI::confirm( '--reassign parameter not passed. All associated posts will be deleted. Proceed?', $assoc_args );
}

If we update this to check existence of reassign and also validity of new user, this would not be a breaking change I guess.