The function email_registration_cleanup_username is not properly replacing whitespace with underscores in the given $name parameter. It should use the proper regex pattern for whitespace detection.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | email_registration-remove-whitespace-2546480-12-d8.patch | 512 bytes | gngn |
| #2 | properly_detect-2546480-2.patch | 525 bytes | njbarrett |
Comments
Comment #2
njbarrett commentedAnd here is the patch.
Comment #3
gregglesThanks for the issue and patch.
I wonder if it's important to clean up whitespace, though? Drupal core allows spaces in usernames, so it seems OK to let it pass through, right?
Comment #4
njbarrett commented@greggles That might be the case, but I guess most people see a username as a single word without whitespace. I'm not sure it's good for backward compatability to change how this works now, as people using the module may be expecting it to replace whitespace with an underscore. Perhaps in a 2.0 release it could be an configurable option?
Comment #5
gregglesRight now it's possible to hook into this process to add site-specific filtering, so that feels like optional enough for me. I have a general preference not to add a ton of checkboxes to the admin UI to cover each possible site-specific feature that people want. Maybe we can see if anyone else is interested in this feature as a way to judge whether it's worth adding the option?
Updating title and status to reflect the conversation.
Comment #6
njbarrett commented@greggles So are we assuming the original intention of replacing whitespace with an underscore was broken, and so people would expect the default behaviour of the module to NOT replace whitespace?
Comment #7
gregglesI see what you're saying now. Thanks for clarifying.
So...what I see is that the old way would remove instances of 2 or more literal space characters and the new way removes all instances of white space characters http://3v4l.org/9QuIs
I think the original goal was probably more legibility focused and I can't fully think of a reason for it, honestly. I can imagine why multiple spaces should get replaced with a single space.
This patch does change behavior of two space characters - I'm not sure how much that matters. I'd say this is probably ready to commit. Let's see if there's any feedback in the next few days?
Comment #8
njbarrett commented@greggles Yep. Sounds good.
Comment #9
njbarrett commentedNo further feedback, shall we commit this?
Comment #11
gregglesYep, makes sense to me.
Thanks!
Comment #12
gngn commentedHere is the ported patch for 8.x-1.x-dev (very similiar to #2).
Comment #13
juliencarnot commentedNot sure I'm understanding everything here, sorry if I'm off topic... Is the purpose of this patch to prevent a user creating an account with firstname.lastname@gmail.com to change her username from "firstname.lastname" to "Firstname Lastname" when completing her registration?
Given that this module allows to no longer use the username as login (one could get why special characters should be avoided then) and that the username will be essentially displayed on author meta and user views, I don't really see why email_registration should add more constraints than what core already defines...
I have some users who chose to have a space in their username ("Firstname Lastname"). What would happen to them if this patch is applied?
Comment #14
gngn commentedI think the patched function email_registration_cleanup_username() is only called in the registration process.
Now it confirms to the comment "Convert any other series of spaces to a single underscore".
So, no, I think your users could change their username afterwards.
Comment #15
juliencarnot commentedThanks for this info. So I understand the patch would only affect a user registering with an email adress such as "much.more unusual"@example.com (see https://en.wikipedia.org/wiki/Email_address#Valid_email_addresses) or users registering with an invalid email address if e-mail validation is not enforced, and not the username chosen when confirming registration and setting a password.
Comment #17
andypostThanx fixed in 8.x and pushed, should appear in 8.x-1.0-rc2