ayg wrote:

*** Bug 12602 has been marked as a duplicate of this bug. ***

Just a minor correction, but @ should be made invalid for use, not completely invalid.

There are 3 levels:

• 'valid' controlled by User::isValidUserName();
• 'useable' controlled by User::isUsableName() and inherits from 'valid';
• 'creatable' controlled by User::isCreatableName() and inherits from 'useable' and therefore 'valid' to;

Currently:
A 'valid' username is one which does not contain characters such as / or other types of characters which could get broken or break something in the interface.

A 'useable' username is one which is 'valid' and also is not a blacklisted username (ie: For maintenance scripts).

A 'creatable' username is one which is 'useable' and therefore also 'valid' and also does not contain characters which should not be 'useable' however have been in prior use or have some 'useable' use under restricted circumstances and therefore are not put in the unusable category but still denied for creation.

The proposal would be to move the @ sign from it's current non-'createable' state to a non-'useable' state. A lot of interface methods test for 'valid' usernames, but still accept 'useable' usernames as input. If the @ sign were moved directly to non-'valid' then it would break the proposed upgrade to transwiki import by breaking ui parts where it should still be valid for checking things on users which were transwikied. For example, the API would no longer allow you to check the contributions of a transwikied user. In other words if someone imported a number of pages and you wanted to check out what imported contributions were made by Dantman@en.wikipedia.org the API would fail with a message "User name Dantman@en.wikipedia.org is not valid" instead of displaying the transwikied user's contributions.

So to restate, @ should be moved from it's current location in User::isCreatableName() into User::isUsableName(); NOT into User::isValidUserName();

mike.lifeguard+bugs wrote:

Severity -> normal as I don't think this is really an /enhancement/

mac.med02 wrote:

Possible patch, needs review

attachment user patch.patch ignored as obsolete

You're applying all creation restrictions to usable names. That also means changing $wgInvalidUsernameCharacters semantics. I don't feel that's the right way.

mac.med02 wrote:

Different

Gave something different a shot. Hopefully this should work.

Attached:

ayg wrote:

+ //Ensure that the username does not contain @ symbol
+ else if ( preg.match( '@', $name ) {
+ return false;
+ }
+
+

Did you test this? "preg.match" looks like a fatal error to me, plus you're missing a parenthesis, so that looks like a syntax error there as well. You should at *least* test your patches with php -l before submitting . . .

Also, you should be able to use strpos() here, preg_match() is overkill. And don't put whitespace between an if and elseif; it should be like

if ( foo ) {
...
} elseif ( bar ) {
// comment
...
}

And finally, you've got a whole bunch of trailing whitespace here, on three separate lines -- please avoid that.

• global $wgInvalidUsernameCharacters;

+ global $wgInvalidUsernameCharacters, $wgInvalidUsernameCharacters;

		return
			self::isUsableName( $name ) &&

+

			// Registration-time character blacklisting...
			!preg_match( '/[' . preg_quote( $wgInvalidUsernameCharacters, '/' ) . ']/', $name );

+

You don't seem to have changed anything in this part other than adding an unused global declaration, and adding trailing whitespace.

Other than that, this seems good, except that a) I don't know for sure if we actually want this anymore, I filed this bug based on something Brion said ages ago and his reasons might be obsoleted by SUL; b) if we do want it, we definitely need to work out a migration strategy before committing it.

sumanah wrote:

mac.med02@gmail.com, did you see Aryeh's review? Adding the "reviewed" keyword.