#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 )
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)
Change History (35)
#2
@
6 years 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
@
6 years ago
In 36860.diff I noticed a typo with an extra closing )
.
This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.
6 years ago
#7
@
6 years 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
@
6 years 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. */
#9
@
6 years ago
Thanks @birgire I greatly appreciate the points, added 36860.4.diff to address your notes
#13
@
6 years 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.
#15
@
6 years 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:
↓ 17
@
6 years 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.
#17
in reply to:
↑ 16
@
6 years 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:
↓ 20
@
6 years 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?
#19
@
6 years ago
ps: just adding the 46421-6.diff as an example if we want the filtering as bool|null
.
#20
in reply to:
↑ 18
@
6 years 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 thepost_author
andlink_owner
queries fornull
. 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
@
6 years 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; } }
#22
@
6 years 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 );
@
6 years ago
Patch to cleanup the code and remove need of extra variable, but kept the contextual filter name
#23
@
6 years 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.
Seems like a useful fix for users who are using Co-Authors Plus plugin on a live site.