WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 5 months ago

Last modified 3 months ago

#36860 closed enhancement (fixed)

Introduce filter for users_have_content within delete action of users.php

Reported by: garrett-eclipse Owned by: garrett-eclipse
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.6
Component: Users Keywords: has-patch needs-testing has-dev-note
Focuses: administration Cc:

Description (last modified by 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

Attachments (8)

36860.diff (933 bytes) - added by garrett-eclipse 11 months ago.
Initial Concept
36860-2.diff (931 bytes) - added by garrett-eclipse 11 months ago.
Remove extra bracket
36860.3.diff (1.3 KB) - added by garrett-eclipse 11 months ago.
Wrap SQL checks in conditional to avoid unnecessary database calls
36860.4.diff (1.4 KB) - added by garrett-eclipse 11 months ago.
Updated Docblock
36860.5.diff (1.4 KB) - added by garrett-eclipse 5 months ago.
Fix minor CS 'tabs vs spacing' and applied 5.2.0
46421-6.diff (1.6 KB) - added by birgire 5 months ago.
36860.6.diff (1.5 KB) - added by xkon 5 months ago.
adding extra "users_have_additional_content" filter
36860.7.diff (1.5 KB) - added by garrett-eclipse 5 months ago.
Patch to cleanup the code and remove need of extra variable, but kept the contextual filter name

Download all attachments as: .zip

Change History (35)

#1 @lukecavanagh
3 years ago

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

@garrett-eclipse
11 months ago

Initial Concept

#2 @garrett-eclipse
11 months ago

  • Keywords has-patch 2nd-opinion needs-testing needs-unit-tests added
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

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

#3 @birgire
11 months ago

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

@garrett-eclipse
11 months ago

Remove extra bracket

#4 @garrett-eclipse
11 months ago

  • Keywords 2nd-opinion removed

Thanks @birgire, Uploaded fix.

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


11 months ago

#6 @garrett-eclipse
11 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 4.9.9

@garrett-eclipse
11 months ago

Wrap SQL checks in conditional to avoid unnecessary database calls

#7 @garrett-eclipse
11 months ago

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.

#8 @birgire
11 months ago

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
11 months ago

Updated Docblock

#9 @garrett-eclipse
11 months ago

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

#10 @pento
11 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#11 @pento
8 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#12 @pento
8 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#13 @audrasjb
8 months ago

  • Milestone changed from 5.0.3 to 5.1

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.

#14 @pento
7 months ago

  • Milestone changed from 5.1 to 5.2

@garrett-eclipse
5 months ago

Fix minor CS 'tabs vs spacing' and applied 5.2.0

#15 @garrett-eclipse
5 months ago

  • Keywords needs-unit-tests removed

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

#16 follow-up: @birgire
5 months ago

@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.

Last edited 5 months ago by birgire (previous) (diff)

#17 in reply to: ↑ 16 @garrett-eclipse
5 months ago

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.

#18 follow-up: @birgire
5 months ago

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?

Last edited 5 months ago by birgire (previous) (diff)

@birgire
5 months ago

#19 @birgire
5 months ago

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

#20 in reply to: ↑ 18 @garrett-eclipse
5 months ago

  • Keywords 2nd-opinion dev-feedback added

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.

#21 @birgire
5 months ago

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
5 months ago

adding extra "users_have_additional_content" filter

#22 @xkon
5 months ago

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
5 months ago

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

#23 @garrett-eclipse
5 months ago

  • Keywords commit added; 2nd-opinion dev-feedback removed

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.

#24 @SergeyBiryukov
5 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 months ago

#26 @garrett-eclipse
5 months ago

  • Keywords needs-dev-note added

#27 @desrosj
3 months ago

  • Keywords has-dev-note added; commit needs-dev-note removed
Note: See TracTickets for help on using tickets.