Cleanup for user_validate_name() + tests

I don't pretend to understand that strange regexp, but it seems that there is no need to validate the server part because the authentication scheme don't use that anymore (it was used for Drupal Distributed Authentication, which is deprecated in favor of OpenID).

So feel free to drop that part.

One remark about the patch: the strlen() there should be a drupal_strlen(), because we are in an unicode context.

and various weirdo UTF8 chars... or even invalid UTF8 characters for that matter.

Patch is goods so far.

I'm wondering if we could incorporate allowing apostrophes into this cleanup, since it's currently just an omission. Patch and discussion here: http://drupal.org/node/165226

I don't understand at all why we are not using ENT_NOQUOTES in check_plain()?

It looks like this sort of things should not even be required. I guess some part of the code may call check_plain() two times, and it fails because that function is not reentrant.

Works for me. Will leave it at needs review for a second set of eyes but I think this is ready. Side note, patch isn't rolled from root of the Drupal directory, it's easier to apply that way.

@MadHarold, the patch is perfectly rolled from the root directory, there is nothing wrong about it.

Testing the patch in #8:

Applied the patch to todays dev tarball - applies cleanly.

created a number of accounts - those with or without apostrophe are created correctly. Those with quotation marks (") give an error of an illegal character.

All that is left is for a "policy decision" - either patch is comment or comment comment 8 should go in.

Moved to RTBC as I am the second pair of eyes as requested by Catch.

Well, there are some XSS vectors that succeed due to injected quotes closing an img path or attribute, so I'm not that comfortable with this. Clearly check_plain() should not be called multiple time on the same text - but that's a separate issue.

Would this analysis also apply for the original (otherwise almost identical) patch in comment 6 that uses ENT_COMPAT instead of ENT_NOQUOTES?

Or in clear words; check_plain should not be modified.

Of course there is. Investigate the causes of double encoding and fix them.

For instance: blog_link escapes the username

Then theme_links escapes the title and attributes again (in l()):

The patch in #22 can go in this way. Making check_plain() reentrant will require a new issue.

Committed to CVS HEAD. Thanks.

Some tests are failing. Please keep that issue open on 7.x so we can figured out why.

Bumping to critical by convention (tests are failing...).

Looks good, works good too.

Ok, let's consider inclusion in the Drupal 6.x branch.

Why can't we just trim() username? I realize this is already in the 7.x version; I didn't get a chance to review that patch before it was committed. It also shouldn't be a tab character or any other whitespace. trim() takes care of that.

Patch in comment 34 applies cleanly, works as expected and is a port of what is in HEAD so setting to RTBC

As for comment 35, I would guess the proper place to trim would be upon input - before the validation function.

I have re-ported the patch from comment 30 - without any functional changes that were introduced to the port to drupal 6 in comment 34.

Not supporting a single quote in a username is a bug, not a lacking feature.

We have two patches here - comment 34 without the changes allowing apostrophes in usernames, and comment 39 with the changes allowing apostrophes.

Can somebody please review (and maybe help in the other issue (#276174: Do not check_plain() usernames more than once) too so that I can find out exactly where the other checkplain takes place)?

Once both of these patches are RTBC, the maintainer can decide if he wished to allow or not allow this bug fixing functionality.

Patch in comment 39 still works, but with an offset of 14 lines in hunk 3. Should I reroll?

(and is some kind soul willing to review?)

updated patch from comment 39 - I just removed hunk three as all that did was change a string to inform that apostrophes are allowed.

In this patch, apostrophes are still allowed, but that is now "by stealth" so as not to break string freeze.

(This will need to be applied at the same time as #276174: Do not check_plain() usernames more than once to fix the theming errors for usernames in blog.module.)

Patch in comment 47 still applies, but with an offset of 2 lines.

For those subscribing - it would be helpful if the patch was reviewed and set to rtbc if its working as expected.

"How can I set it to rtbc"

you just choose the option. At that stage, the maintainer of the Drupal 6 branch can then agree or disagree with the change. til then it is nothing and will not be committed even if the maintainer likes it.

I have also been using this patch for a long time too and think it is ready, but since I rolled it, made some changes etc... it would not be good for me toset it to rtbc.

fwiw, the last version does not break any strings - it just adds the feature "silently" and this will be compatible with Drupal 7 since that already has the patch.

Great, thanks for all the testing, committed the patch with one small change. Left the initial strlen() intact (with the code style changes applied). The version in the patch did not allow for username "0" (the number zero) which is otherwise allowed before the patch. I'd suggest porting this change back to D7 as not to break backwards compatibility with D6 userbases.

Okay, I've asked around, and the general consensus seems to be that "0" should be a valid username. That being the case, here's a patch against D7 that implements the same check as D6 has. I'll deal with the impact of this in #845472 (for both D7 and D6).

Here is what D7 does at the moment:

Enter nothing in the Username field and you get 'Password field is required.'.

The only way to get the 'You must enter a username.' string (from user_validate_name() ) is to enter a 0 in the Username field.

The forms layer is already validating that a string has been entered. user_validate_name() is attempting to repeat this logic and screwing it up.

The 'required' check should just be removed from user_validate_name().

'0' is a perfectly valid string. We shouldn't let the failings of PHP dictate what is and what isn't a valid username.

See also http://drupal.org/node/845472#comment-3276796

It seems that user_validate_name() is tested directly, instead of through the form.

Updated patch fixes the empty check in user_validate_name() and sets the username form item to validated so that user_validate_name() can report the error the way it wants.

heh, that's a giant hack :)

If it's just for getting around the default #required error message, then postpone this change on #742344: Allow forms to set custom validation error messages on required fields.

Actually, #required should already handle exactly the case for that empty username, so I'm not sure why this code contains custom validation for the very same empty condition at all.

But again, if it's just to have a fancier validation error message, then aforementioned issue is required for this patch.

Powered by Dreditor.

Reading through the entire issue, now I understand where you're coming from.

1. The follow-up fix that was already applied to D6 isn't contained in http://api.drupal.org/api/function/user_validate_name/7 yet: Drupal 7 does not allow a username of "0".
2. That quick follow-up fix for D6 should have used drupal_strlen() instead of strlen().
3. I see the point of user_validate_name() being invoked on its own outside Form API. Thus, we want to keep that first condition + error message.
4. Regardless of what you do (and #validate => TRUE won't fly ;), that very error message won't be output for the user registration form. That's only possible via #742344: Allow forms to set custom validation error messages on required fields, which, however, requires to duplicate this form validation error message elsewhere.