Introduce filter for users_have_content within delete action of users.php

[garrett-eclipse]

Hello,

I'd like to request an enhancement on the delete action for users.
Idea - ​https://wordpress.org/ideas/topic/introduce-filter-for-users_have_content-within-delete-action-of-usersphp?replies=1#post-30346

The addition of a filter for $users_have_content in the users.php
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-admin/users.php#L214

Proposed filter;

<?php
/**
  * Filter the users_have_content, also known as the users have content flag, before use.
  *
  * @since ###
  *
  * @param boolean $users_have_content The flag for users have content.
  */
$users_have_content = apply_filters( 'users_have_content', $users_have_content, $userids ) );

Allows for plugins to introduce reassign capabilities when their user relational data isn't handled through the posts_author or link_owner database checks.
Example: Co Authors Plus saves additional author information in taxonomy terms. If a co-author is removed currently no reassignment can be made. With the filter the userids can be checked for coauthor relationships so as to enable the reassign user ability.

Thank you

[lukecavanagh]

Seems like a useful fix for users who are using Co-Authors Plus plugin on a live site.

[garrett-eclipse]

Initial Concept

[garrett-eclipse]

Thanks @lukecavanagh

I've put together an initial patch for this but will require review and unit tests.

I placed the filter prior to the WordPress checks to avoid plugins suppressing them and possibly losing users content. If needed another filter can be added after to support suppression but I fear that may have negative consequences.

If you could review and help me with next steps I'd appreciate the help.

Cheers

[birgire]

In 36860.diff​ I noticed a typo with an extra closing ).

[garrett-eclipse]

Remove extra bracket

[garrett-eclipse]

Thanks @birgire, Uploaded fix.

[garrett-eclipse]

Wrap SQL checks in conditional to avoid unnecessary database calls

[garrett-eclipse]

Thanks for the feedback today @SergeyBiryukov I've updated the patch to wrap the mysql calls in a conditional so if the plugin supplies 'true' to the filter then we can avoid those additional database calls.

[birgire]

Few more suggestions for 36860.3.diff​

• Add Default false. for the inline documentation of the $users_have_content input parameter.
• Add missing @param for the $userids input parameter.
• Code styling of $users_have_content in the filter's description.
• Add "" around users have flags in the filter's description, to make it more readable.

So the suggestion is to change:

/**
 * Filter to setup $users_have_content, also known as the users have content flag, before WordPress default checks.
 *
 * @since ###
 *
 * @param boolean $users_have_content The flag for users have content.
 */

to

/**
 * Filter to setup `$users_have_content`, also known as the "users have content" flag, before WordPress default checks.
 *
 * @since ###
 *
 * @param boolean $users_have_content The flag for users have content. Default false.
 * @param int[]   $userids            Array of IDs for users being deleted.
 */

[garrett-eclipse]

Updated Docblock

[garrett-eclipse]

Thanks @birgire I greatly appreciate the points, added 36860.4.diff to address your notes

[audrasjb]

Hello,

5.0.3 is going to be released in a couple of weeks. We are currently sorting the remaining tickets in the milestone.

The ticket adds a new filter and still needs some testing and unit-tests. Since 5.0.3 is going to be a "normal" minor with only self-contained patches and regression/bug fixes. Let's address this ticket in 5.1 which is scheduled for February.

[garrett-eclipse]

Fix minor CS 'tabs vs spacing' and applied 5.2.0

[garrett-eclipse]

I've updated the patch in preparation of 5.2.0. I don't believe it warrants unit tests, but @birgire would you mind giving it a final test and we can mark for commit. Thanks

[birgire]

@garrett-eclipse I will have a look and test it.

The following question comes to mind:

If we have some some custom user content, that's only added via some plugin and not stored in the posts or links table, how would one handle here the actual reassignment/removal from the db?

I think it would be nice to have a simple example to better understand how one could approach this problem, e.g. using this filter and how to reassign/delete the data.

[garrett-eclipse]

Replying to birgire:

The following question comes to mind:

If we have some some custom user content, that's only added via some plugin and not stored in the posts or links table, how would one handle here the actual reassignment/removal from the db?

I think it would be nice to have a simple example to better understand how one could approach this problem, e.g. using this filter and how to reassign/delete the data.

Thanks @birgire I appreciate you testing.

As to your question on how a plugin would handle any removal/reassignment;
This is already available and being used throughout many plugins as the delete_user and deleted_user actions are available and used to remove and re-assign data.
For a specific example Co-Authors Plus already handles delete and reassign using the delete_user_action here;
​https://github.com/Automattic/Co-Authors-Plus/blob/4e1b2d4c80bf28169bef5d8a1c77e95489acf042/co-authors-plus.php#L943-L988

This new filter is to correct a limitation of WordPress where it won't allow plugins to enable the re-assign screen when they have custom data but the user doesn't have any standard WP data.

In many cases plugins already have the delete/reassign logic in place and would just be able to use the new filter to ensure full coverage on delete so if their plugin has user data that's available for re-assignment.

That being said I agree, I believe a nice example should be cooked up for the Codex to illustrate how to use the filter, and then we can reference the delete_user action and add another example there for the actual reassignment/removal.

If we're able to wrap up testing I feel we can make RC1 and then can continue work on the docs and codex once it's in trunk.

[birgire]

Thanks @garrett-eclipse for that information and it's informative to see how plugins are handling it.

One thing that might be confusing with the current patch 36860.5.diff​ is that if we use e.g.:

add_filter( 'users_have_content', '__return_false' );

The developer might think it's being overriden by the filter, but we still get the UI for reassigning/removing user data, even if the deleted user is post author or link owner.

Another approach, if we want to allow the filter to control the UI both ways, is to have the default filter value as null and only run the post_author and link_owner queries for null. That way the boolean value of the filter can be used to turn it on/off.

Do you see any reason to know that the user is being deleted from this UI, compared to e.g. other wp_delete_user() calls elsewhere?

[birgire]

ps: just adding the 46421-6.diff​ as an example if we want the filtering as bool|null.

[garrett-eclipse]

Replying to birgire:

Thanks @garrett-eclipse for that information and it's informative to see how plugins are handling it.

One thing that might be confusing with the current patch 36860.5.diff​ is that if we use e.g.:

add_filter( 'users_have_content', '__return_false' );

The developer might think it's being overriden by the filter, but we still get the UI for reassigning/removing user data, even if the deleted user is post author or link owner.

Another approach, if we want to allow the filter to control the UI both ways, is to have the default filter value as null and only run the post_author and link_owner queries for null. That way the boolean value of the filter can be used to turn it on/off.

Do you see any reason to know that the user is being deleted from this UI, compared to e.g. other wp_delete_user() calls elsewhere?

Thanks for the feedback @birgire I'd initially been going down the approach to allowing the plugin/customization to have full control but as I mentioned in my second comment I wanted to avoid plugins suppressing the WordPress checks as it can potentially lose users content.

Reference to comment#2:

I placed the filter prior to the WordPress checks to avoid plugins suppressing them and possibly losing users content. If needed another filter can be added after to support suppression but I fear that may have negative consequences.

Mainly I wanted to avoid unintentional data loss but am open to further discussion. The other idea above about another filter if suppression is desired could be avoided by using your null approach there.

Adding dev-feedback/2nd-opinion to get some more input so we can make a decision and hopefully still get this in by Thursday.

[birgire]

If the approach in 36860.5.diff​ is preferred, then I wonder if another filter name would be beneficial:

like users_have_custom_content, users_have_additional_content or something that describes that this is in addition to authored posts or owned links, instead of the general users_have_content name.

So if the filter's output is false then $users_have_content would be determined from the current post author or link owner checks.

If the filter's output is true then $users_have_content would be true and the current post author or link owner checks would be skipped.

Here's an example:

 */
$users_have_custom_content = (bool) apply_filters( 'users_have_custom_content', false, $userids ) );

$users_have_content = false;

if ( $users_have_custom_content ) {
	$users_have_content = true;			
} else {
	if ( $wpdb->get_var( "SELECT ID FROM {$wpdb->posts} WHERE post_author IN( " . implode( ',', $userids ) . ' ) LIMIT 1' ) ) {
		$users_have_content = true;
	} elseif ( $wpdb->get_var( "SELECT link_id FROM {$wpdb->links} WHERE link_owner IN( " . implode( ',', $userids ) . ' ) LIMIT 1' ) ) {
		$users_have_content = true;
	}
}

[xkon]

adding extra "users_have_additional_content" filter

[xkon]

I find @birgire 's comment easier to read in general as the different naming of the filter makes more sense and it also avoids the 2 extra db checks so that can't be bad as well :) .

Since I was checking this out 36860.6.diff​ is based on that while adding the users_have_additional_content name for descriptive purposes. I'm pretty sure this will come in handy to more authors as well.

And since I was at it a little "copy paste" to save some time as well:

add_filter(
	'users_have_additional_content',
	function( $flag, $userids ) {

		if ( ! $flag ) {
			error_log( print_r( $userids, true ) );

			$flag = true;
		}

		return $flag;
	},
	15,
	2
);

[garrett-eclipse]

Patch to cleanup the code and remove need of extra variable, but kept the contextual filter name

[garrett-eclipse]

Thanks @xkon & @birgire I greatly appreciate the testing and input. And whole-heartedly agree that differentiating the filter by naming it users_have_additional_content gives it the necessary context to properly define it's behaviour.

I also appreciate the additional patches and have put a final 36860.7.diff forward which removes the need for the additional variable and checks as it provides the same results/conditions and preserves the distinct and contextual filter name.

I've marked as commit, but please take a last test on it and I'll work on the docs once it's in the 5.2 beta.

[SergeyBiryukov]

In 44967:

Users: Introduce users_have_additional_content filter to indicate whether the users being deleted have additional content associated with them outside of the post_author and link_owner relationships.

Props garrett-eclipse, xkon, birgire.
Fixes #36860.

[desrosj]

Dev note: ​https://make.wordpress.org/core/2019/04/24/miscellaneous-developer-updates-in-5-2/