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.

Comments

njbarrett created an issue. See original summary.

njbarrett’s picture

Status: Active » Needs review
StatusFileSize
new525 bytes

And here is the patch.

greggles’s picture

Thanks 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?

njbarrett’s picture

@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?

greggles’s picture

Title: Properly detect whitespace in email_registration_cleanup_username » Optionally remove whitespace in email_registration_cleanup_username
Status: Needs review » Needs work

Right 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.

njbarrett’s picture

@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?

greggles’s picture

Title: Optionally remove whitespace in email_registration_cleanup_username » Remove all whitespace in email_registration_cleanup_username
Status: Needs work » Needs review

I 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?

njbarrett’s picture

@greggles Yep. Sounds good.

njbarrett’s picture

No further feedback, shall we commit this?

greggles’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Yep, makes sense to me.

Thanks!

gngn’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new512 bytes

Here is the ported patch for 8.x-1.x-dev (very similiar to #2).

juliencarnot’s picture

Not 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?

gngn’s picture

I 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.

juliencarnot’s picture

Thanks 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.

  • andypost committed 85910b0 on 8.x-1.x authored by gngn
    Issue #2546480 by njbarrett, gngn: Remove all whitespace in...
andypost’s picture

Status: Needs review » Fixed

Thanx fixed in 8.x and pushed, should appear in 8.x-1.0-rc2

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.