Last modified: 2014-09-23 23:42:12 UTC
It's been forbidden to create accounts with at-signs in them for some time, because this conflicts with interwiki syntax and will cause problems for SUL. More immediately, such users cannot be affected by Special:Userrights, which caused some confusion on enwiki earlier with the wider access that's been given. A maintenance script runnable from update.php needs to find and rename all such users (with suitable warning), and @ should be made entirely invalid in usernames instead of just invalid on creation.
*** 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();
Severity -> normal as I don't think this is really an /enhancement/
Created attachment 6619 [details] Possible patch, needs review
You're applying all creation restrictions to usable names. That also means changing $wgInvalidUsernameCharacters semantics. I don't feel that's the right way.
Created attachment 6625 [details] Different Gave something different a shot. Hopefully this should work.
- + //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.
mac.med02@gmail.com, did you see Aryeh's review? Adding the "reviewed" keyword.
