Repository navigation
Reassigning posts while deleting a user can result in lost posts if the target user does not exist #461
Description
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() . '.';
}
Activity
danielbachhuber commentedon Feb 11, 2024
Thanks for the report, @franz-josef-kaiser
Thinking out loud..
It's not so much that the data was lost though, right? The
post_authorsimply 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 commentedon Feb 11, 2024
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
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 commentedon Feb 12, 2024
Thanks for sharing this detail!
Sure, here is some guidance on our pull request best practices.
ernilambar commentedon Feb 26, 2024
We already ask for confirmation if reassign parameter is missing.
If we update this to check existence of reassign and also validity of new user, this would not be a breaking change I guess.