user_name / username checks in bp_core_validate_user_signup

[anthony.bartoli]

bp_core_validate_user_signup() in bp-members-functions.php seems to have technically unnecessary rules when validating the username:

$errors->add( 'user_name',  __( 'Username must be at least 4 characters', 'buddypress' ) );

$errors->add( 'user_name', __( 'Sorry, usernames may not contain the character "_"!', 'buddypress' ) );

$errors->add( 'user_name', __( 'Sorry, usernames must have letters too!', 'buddypress' ) );

Because WP_Error only allows you to remove *all* errors (WP_Error->remove()) for a given code, using the same code ("user_name") for these checks is especially problematic if I wanted to get rid of one condition(s) while keeping other(s).

For example, I think many BP admins would not want community members using all numbers for a username but the current 4-character length requirement seems particularly subjective.

If these checks were to be removed, is there an automated suite of tests that could be ran to ensure that other BP functions would not break?

If no such tests exist, perhaps a safe fix would be to use a unique WP_Error code so that they can be removed individually with WP_Error->remove() by hooking onto the bp_core_validate_user_signup filter.

As of now, to eliminate one requirement while keeping the others, a developer would have no choice but to hack core BP, or remove all the username requirements (WP_Error->remove('user_name') or manually parse, filter, and rebuild $result['errors'].

So this is not a bug per se, but I think these username conditions could be re-written in a more flexible manner - assuming they are even required.

I'd be more than happy to write a patch if my observations are correct. I am somewhat new to BP and WP development, so I look forward to hearing your feedback on this.

[boonebgorges]

Thanks for the ticket, anthony.bartoli. A couple of thoughts:

Because WP_Error only allows you to remove *all* errors (WP_Error->remove()) for a given code, using the same code ("user_name") for these checks is especially problematic if I wanted to get rid of one condition(s) while keeping other(s).

I'd never realized this before about WP_Error, but you are quite right. The funny part is that when we display the errors, we only display the first one ([0]) ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-members/bp-members-screens.php#L112. Giving them different error handles seems like a good idea, but it's likely to cause some backward compatibility headaches. I have definitely written plugins that reach into this error object and unset various things, and if they were no longer grouped under 'user_name', those mods would break. It may still be worth doing (since it's fairly broken at the moment), but it's something to consider.

As for the requirements themselves: Most of them come from the days of WPMU (BP used to require it). MU had (and MS still has) fairly restrictive requirements on user_login. See extended discussion at #5185 and ​https://core.trac.wordpress.org/ticket/17904. I would be open to considering tearing these checks out altogether.

If these checks were to be removed, is there an automated suite of tests that could be ran to ensure that other BP functions would not break?

We have a suite of automated tests for BuddyPress. See ​https://buddypress.trac.wordpress.org/browser/trunk/tests/phpunit/testcases. It's likely that we don't have sufficient coverage to guarantee that nothing would break if we allowed for various kinds of usernames. We'd need, for example, to make sure that our URL parsing logic works correctly for usernames that are made up only of numbers.

If you'd like to have a shot at researching and implementing a suggested solution, I'd be happy to work through it with you.

[DJPaul]

As for the requirements themselves.. I would be open to considering tearing these checks out altogether.

I think someone would first need to put together a table of how non-MS and MS handle all the edge cases, so we can figure out what we want to do. I can't visualise what the inconsistencies might be if we let non-MS and MS handle things differently.

I have definitely written plugins that reach into this error object and unset various things, and if they were no longer grouped under 'user_name', those mods would break.

How are you parsing the appropriate error to unset? If the "key" is the same, are you matching on the (translatable) strings?

[boonebgorges]

How are you parsing the appropriate error to unset? If the "key" is the same, are you matching on the (translatable) strings?

Yes, I have matched the strings in the past. It's very fragile, so I'm not super super worried about breaking it.

I think someone would first need to put together a table of how non-MS and MS handle all the edge cases, so we can figure out what we want to do

Yes, this would be helpful, as we'll have to assess what effect they'll have on BP functionality. But my general preference would be for BP to do as little validation in this area as possible - we should not be in the business of adding extra restrictions to user_login.

[johnjamesjacoby]

I think someone would first need to put together a table of how non-MS and MS handle all the edge cases, so we can figure out what we want to do. I can't visualise what the inconsistencies might be if we let non-MS and MS handle things differently.

I know @netweb is super good at investigation like this, otherwise I don't mind getting more familiar with this. I know there are a few core tickets talking about this, and whether to merging the differences.

[anthony.bartoli]

Thanks everyone for your feedback.

How about adding an additional check for a plugin's implementation similar to how we hand off validation to MS? Like this:

if ( function_exists( 'wpmu_validate_user_signup' ) ) {
		
$result = wpmu_validate_user_signup( $user_name, $user_email );

// Or allow developers to do their own validation...
} elseif ( function_exists( 'bp_plugin_validate_user_signup' ) )  {
		
$result = bp_plugin_validate_user_signup( $user_name, $user_email ); //implement this in plugins/bp-custom.php

//Otherwise, perform our default validation
else{
		//current BP checks
}

Adding the above logic would preserve BP validation while allowing developers implement their own validation.

[boonebgorges]

Thanks for the suggestion, anthony.bartoli. The function_exists() pattern is not really one that we use much in BP; among other things, it's subject to the problem of what to do when more than one plugin defines the function. A more sustainable solution, if we want to allow plugin devs to bypass BP's validation, is to put the following at the beginning of bp_core_validate_user_signup():

$pre = apply_filters( 'bp_core_pre_validate_user_signup', false, $user_name, $user_email );
if ( false !== $pre ) {
    return $pre;
}

I'm not sure that this is the absolute best way to attack the problem. Plugin devs who use the filter would then be responsible for assembling a properly-formatted array of $results, not to mention the likelihood that bypassing some of BP/WP's core validation checks would result in breakage somewhere in the system. That being said, I think it's fine to introduce a pre-filter like this for the more intrepid devs out there, as long as no one else has an objection.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/