Problem/Motivation

My use case is that my usernames and email addresses match exactly - they have been imported from a previous system where users logged in with their email address and had no username. This works for almost all users, except where an email address contains the + character. In those cases my users are unable to edit their profiles because drupal throws a validation error on save:
The username contains an illegal character.

Proposed resolution

Add + character to the regex.

Remaining tasks

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#193 2157963-3-193.patch3.53 KBalexpott
#188 2157963-3-188.patch3.58 KBalexpott
#188 183-188-interdiff.txt1.75 KBalexpott
#183 2157963-3-183.patch2.71 KBalexpott
#180 2157963-3-180.patch1.8 KBalexpott
#173 innerdiff-2157963-173.txt2.95 KBbiguzis
#173 2157963-173.patch30.5 KBbiguzis
#164 interdiff-2157963-164.txt1.57 KBlokapujya
#164 2157963-164.patch30.55 KBlokapujya
#161 interdiff-2157963-161.txt804 byteslokapujya
#161 2157963-161.patch30.63 KBlokapujya
#155 interdiff-2157963-155.txt715 byteslokapujya
#155 2157963-155.patch29.85 KBlokapujya
#152 2157963-152.patch29.15 KBlokapujya
#152 interdiff-2157963-152.txt3.67 KBlokapujya
#149 2157963-149.patch27.87 KBlokapujya
#149 interdiff-2157963-149.txt4.9 KBlokapujya
#4 2157963-username_allow_character-D7_0.patch608 bytesthemic8
#3 drupal-username_allow_+_character-D7_0.patch608 bytesthemic8
drupal-username_allow_+_character.patch904 bytesjenlampton
#16 drupal-username_allow_+_character-rerolled.patch857 byteskpv
drupal-username_allow_+_character-D7.patch608 bytesjenlampton
#106 username-with-apostrophes-2157963-106.patch14.25 KBNikolay Shapovalov
#106 interdiff.102-106.txt800 bytesNikolay Shapovalov
#92 interdiff-2157963-87-92.txt3.22 KBsubhojit777
#92 allow_symbol_in-2157963-92.patch11.05 KBsubhojit777
#87 allow_symbol_in-2157963-87.patch10.6 KBNikolay Shapovalov
#81 2157963-78.patch10.5 KBlokapujya
#70 name_with_symbol.png153.79 KBdimaro
#65 allow_symbol_in-2157963-65.patch10.32 KBdimaro
#65 interdiff-2157963-63-65.txt1.09 KBdimaro
#63 allow_symbol_in-2157963-63.patch10.42 KBdimaro
#63 interdiff-2157963-58-63.txt1000 bytesdimaro
#58 21579363-Allow-plus-symbol-in-username-58.patch9.74 KBNoe_
#55 Drupal_user_Tests_UserSearchTest-24-4.html_.txt5.6 KBthtas
#51 2157963-Allow-plus-symbol-in-usernames-50.patch9.1 KBthtas
#49 2157963-46-redo.patch8.62 KBlokapujya
#48 choose_characters.txt1.13 KBlokapujya
#46 interdiff-2157963-46.txt1012 byteslokapujya
#46 2157963-46.patch3.32 KBlokapujya
#44 2157963-44.patch7.2 KBlokapujya
#42 2157963.42.patch6.63 KBalexpott
#42 40-42-interdiff.txt825 bytesalexpott
#40 2157963.39.patch6.05 KBalexpott
#38 2157963.38.patch62.71 KBalexpott
#38 34-38-interdiff.txt931 bytesalexpott
#34 2157963.34.patch5.74 KBalexpott
#34 33-34-interdiff.txt2.49 KBalexpott
#33 2157963.33.patch5.27 KBalexpott
#33 25-33-interdiff.txt4.34 KBalexpott
#30 interdiff-2159762-30.txt714 byteslokapujya
#30 2157963-30.patch3.19 KBlokapujya
#25 2157963.25-test-only.patch2.49 KBalexpott
#20 2157963-20-username_allow_+_character-test-only.patch988 bytesbenjf
#19 interdiff.txt868 bytesbenjf
#19 2157963-19-username_allow_+_character.patch1.8 KBbenjf
#102 interdiff_92.txt3.17 KBNikolay Shapovalov
#102 allow_symbol_in-2157963-102.patch14.22 KBNikolay Shapovalov
#109 interdiff.102-109.txt7.24 KBNikolay Shapovalov
#109 allow_symbol_in-2157963-109.patch21.19 KBNikolay Shapovalov
#117 2157963-117.patch21.88 KBlokapujya
#117 interdiff-2157963-117.txt1.56 KBlokapujya
#120 allow_symbol_in-2157963-120.patch25.54 KBbiguzis
#121 interdiff.117-120.txt3.19 KBbiguzis
#123 allow_symbol_in-2157963-122.patch25.54 KBbiguzis
#126 allow_symbol_in-2157963-126.patch25.07 KBbiguzis
#135 2157963-134.patch26.97 KBlokapujya
#135 interdiff-2157963-134.txt1.88 KBlokapujya
#138 2157963-138.patch27.26 KBlokapujya
#138 2157963-138.patch27.26 KBlokapujya
#143 2157963-143.patch26.38 KBlokapujya
#146 interdiff-2157963-143.txt3.84 KBbiguzis
#146 2157963-146.patch29.14 KBbiguzis
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sloshed Cause’s picture

Thanks for this. I agree that this should be added to allow usernames and emails to match providing it won't produce any unwanted side effects.

themic8’s picture

Assigned: Unassigned » themic8

I will test the Drupal 7 patch.

themic8’s picture

Assigned: themic8 » Unassigned
FileSize
608 bytes

The that has been provided works.

themic8’s picture

FileSize
608 bytes

Update file for testing.

themic8’s picture

Status: Active » Needs review

The last submitted patch, drupal-username_allow_+_character.patch, failed testing.

themic8’s picture

Status: Needs review » Reviewed & tested by the community

Tested Drupal 7 patch only.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2157963-username_allow_character-D7_0.patch, failed testing.

jenlampton’s picture

Version: 7.x-dev » 8.x-dev

I'm still using this D7 patch, but we'll need some work done on this in order for it to get in to D8.

dcam’s picture

Closing #1821974: Allow plus sign in user_validate_name to be able to use email as login as a duplicate. It's an older issue, but this one has a slightly more recent Drupal 8 patch than the other.

The last submitted patch, drupal-username_allow_+_character.patch, failed testing.

dcam’s picture

Issue tags: +Needs reroll, +Novice

The Drupal 8 patch in the original post needs to be rerolled.

droplet’s picture

kpv’s picture

Assigned: Unassigned » kpv

working on it

kpv’s picture

kpv’s picture

Status: Needs work » Needs review
star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

We should probably add/expand upon automated test coverage for this functionality. Thanks for the reroll/repatch @kpv!

benjf’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -Needs tests
FileSize
1.8 KB
868 bytes

updated patch to include a test

benjf’s picture

Adding a test-only patch.

star-szr’s picture

Assigned: kpv » Unassigned
Issue tags: +Needs backport to D7

Thanks @kpv and @benjf! This seems backportable to me.

Status: Needs review » Needs work

The last submitted patch, 20: 2157963-20-username_allow_+_character-test-only.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

Setting to Needs review since #20 was a tests-only patch and was supposed to fail.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

#19 looks good and has tests.

alexpott’s picture

This seems sane since more and more sites just use an email address as a username - also see #2014185: Check usernames that are email addresses more rigidly, only allow if matches email - a plus is a valid character an email address.

Let's see if the attached patch breaks anything.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2157963.25-test-only.patch, failed testing.

thijsvdanker’s picture

Assigned: Unassigned » thijsvdanker
Issue tags: +Amsterdam2014

I'm having a look as to why the test is failing.

thijsvdanker’s picture

Assigned: thijsvdanker » Unassigned

Test fails because of two reasons.

  1. ContactPersonalTest->testSendPersonalContactMessage() fails as it checks the dblog for a string. As the username has became longer he string has changed -> test fails.
  2. UrlAlterFunctionalTest->testUrlAlter() fails as the + is urlencoded into % in the "user/[username]" link.

Regarding 1. there are two ways to go:

  1. Fix the test
  2. Not update the default user name in drupalCreateUser().

The second url issue probably needs to be fixed either at test or at code level.

thijsvdanker’s picture

I've created a separate issue here #2349527: testUrlAlter fails for usernames with a + for the second issue (urlAlter).

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
714 bytes

The full dblog text is shown in the hover text, so assertRaw() should work.

Status: Needs review » Needs work

The last submitted patch, 30: 2157963-30.patch, failed testing.

alexpott’s picture

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -526,7 +526,7 @@ protected function drupalCreateUser(array $permissions = array(), $name = NULL)
-    $edit['name']   = !empty($name) ? $name : $this->randomMachineName();
+    $edit['name']   = !empty($name) ? $name : $this->randomMachineName() . '+' . $this->randomMachineName();

+++ b/core/modules/user/src/Plugin/Validation/Constraint/UserNameConstraintValidator.php
@@ -33,7 +33,7 @@ public function validate($items, Constraint $constraint) {
-    if (preg_match('/[^\x{80}-\x{F7} a-z0-9@_.\'-]/i', $name)
+    if (preg_match('/[^\x{80}-\x{F7} a-z0-9@+_.\'-]/i', $name)

So I don't think making all the random test usernames over twice as long is a good idea - I just did this to test. The problem we have is that we never test usernames with spaces, @, +, . or other allowed characters. Which means we write poor tests like UrlAlterFunctionalTest.

We need to somehow generate random strings with only these allowed characters.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
5.27 KB

Patch attached ensures that usernames generated by web test base might contain all types of valid characters. We no longer need to fix ContactPersonalTest since username length is back to normal. I've included the patch in #2349527: testUrlAlter fails for usernames with a + since I don't think this should be a separate issue - it is a result of ensuring we have proper test coverage for this change so let's fix it here.

Below is a sample of the code simulating how the patch generates usernames and the possible output - it's going to be interesting to see if this introduces any random error.

$rand = new \Drupal\Component\Utility\Random();
for($i=0; $i<50; $i++) {
  $name = $rand->string(8, TRUE, array(
      '\Drupal\user\Plugin\Validation\Constraint\UserNameConstraintValidator',
      'validCharacters'
    ));
  var_dump($name);
}

Outputs...

string(8) "bKlf1LT0"
string(8) "JWwV0fcs"
string(8) "7UKvuhC."
string(8) "sPj7sOhM"
string(8) "6.aHhAKv"
string(8) "LBKJUIiK"
string(8) "@ueOt 8f"
string(8) "vcvD4Bh."
string(8) "zniT0@Qt"
string(8) "zfyUeZJA"
string(8) "XDrxgRb9"
string(8) ".1MQxm5u"
string(8) "fJHLQ5FL"
string(8) "8qMG GO-"
string(8) "e07Dl.M9"
string(8) "Z90ISf_E"
string(8) "8kYDG+yP"
string(8) "YR.Ip7m6"
string(8) "wBjbfj-Z"
string(8) "5LNM_VLd"
string(8) ".N+Id3NX"
string(8) "PcR f.8x"
string(8) "xN9_lyqb"
string(8) "7m+dgnGq"
string(8) "SG9SFt1L"
string(8) "6lR7zIK4"
string(8) "zbpjgO3 "
string(8) "1Wt.KHqO"
string(8) "_wmd0wNL"
string(8) "u3k+wGAC"
string(8) "GSTd9z97"
string(8) "DOrA8vF2"
string(8) "8Q@VE4kx"
string(8) "TSDVb9gq"
string(8) "ly6dMDFd"
string(8) "c41g-F5Z"
string(8) "m@rmJK1p"
string(8) "@0I'N2gv"
string(8) "Hm0cJZs9"
string(8) "hECdmUYG"
string(8) "9oZHsHQE"
string(8) "9B9tc25K"
string(8) "v269U+u'"
string(8) "yr@CUmQv"
string(8) "b nvO1_N"
string(8) "ktYyHO8R"
string(8) "u6g3GfI+"
string(8) "bliT2Cm_"
string(8) "Ox.7edML"
string(8) "c7vR8X1@"
alexpott’s picture

FileSize
2.49 KB
5.74 KB

Hmmm.... usernames starting or ending with a space or having double spaces will be problematic. We can leverage TestBaseTest::randomStringValidate().

The last submitted patch, 33: 2157963.33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 34: 2157963.34.patch, failed testing.

lokapujya’s picture

Should we have a testBase method like randomUsername() ?

alexpott’s picture

Status: Needs work » Needs review
FileSize
931 bytes
62.71 KB

So the last patch generates invalid email addresses :)

re #37 @lokapujya I don't think so - that is what drupalCreateUser is for.

Patch attached fixes this with the benefit that we now also start testing having + and . in email addresses before the @ sign.

Below are sample values generated whilst repeating Drupal\system\Tests\Datetime\DrupalDateTimeTest

Test summary
------------

string(8) "1Aiw48uf"
string(20) "1Aiw48uf@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "G0oYRGr."
string(20) "kZpRS0Ao@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "CrdGIzrJ"
string(20) "CrdGIzrJ@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "LTGh7COj"
string(20) "LTGh7COj@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "dHHZ hzU"
string(20) "hjuQGlp1@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "yU03c3x1"
string(20) "yU03c3x1@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "gwin1@Nt"
string(20) "wSWhCF9i@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "scskcadw"
string(20) "scskcadw@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "ht1@2CCO"
string(20) "h8fbIN20@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "3fYCT6@B"
string(20) "xaxkGENq@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "nr2BeC4T"
string(20) "nr2BeC4T@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "b+PEOMmr"
string(20) "b+PEOMmr@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "29v8R63V"
string(20) "29v8R63V@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "3bWw eLi"
string(20) "tnkwn8B0@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "jDCEaybC"
string(20) "jDCEaybC@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "YH8-GSuq"
string(20) "YH8-GSuq@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "Os.XsuvG"
string(20) "Os.XsuvG@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes
string(8) "Is.mYirQ"
string(20) "Is.mYirQ@example.com"
Drupal\system\Tests\Datetime\DrupalDateTimeTest               16 passes

Status: Needs review » Needs work

The last submitted patch, 38: 2157963.38.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

Forget to rebase

Status: Needs review » Needs work

The last submitted patch, 40: 2157963.39.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
825 bytes
6.63 KB

This will fix Drupal\user\Tests\UserPasswordResetTest - that was asserting on mail subject without mime encoding it.

The test fail in Drupal\config_translation\Tests\ConfigTranslationOverviewTest is not worrying since this means it has tried over 100 times to generate a valid username and has failed. We might need to look at how Random::string() chooses possible characters.

Status: Needs review » Needs work

The last submitted patch, 42: 2157963.42.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Amsterdam2014
FileSize
7.2 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 44: 2157963-44.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
1012 bytes

function used in #42 is deprecated.

Status: Needs review » Needs work

The last submitted patch, 46: 2157963-46.patch, failed testing.

lokapujya’s picture

FileSize
1.13 KB

regarding: choose valid characters -- attaching some local code I was messing around with.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
8.62 KB

Status: Needs review » Needs work

The last submitted patch, 49: 2157963-46-redo.patch, failed testing.

thtas’s picture

Status: Needs work » Needs review
FileSize
9.1 KB

I had a go at re-rolling this patch as it wasn't applying

Also changed the tests a bit to stop sanitizing the username and site in the testUserPasswordReset
This is because these values aren't santized in user_mail because "replacement is intended for an email message, not a web browser" and the matching against escaped characters was failing in some cases depending on the random string chosen for the test.

Status: Needs review » Needs work

The last submitted patch, 51: 2157963-Allow-plus-symbol-in-usernames-50.patch, failed testing.

thtas’s picture

Seems to be causing continued issues with other tests where there are username comparisons are in assertions.
Because usernames are generated randomly in tests, tests will only fail if there happens to be these these extra characters in the usernames, so the failure stats in the above patch are probably way higher.

Not sure the best way forward, maybe all the calls to assertText() should all be change to assertEscaped() if they are only checking a username?

lokapujya’s picture

Can we find out which characters are not passing? I ran the UserSearchTest test a few times locally but couldn't reproduce a failing test. Then, if it's reasonable that those characters would be escaped, then try the assertEscaped()?

thtas’s picture

Attached is the verbose output of an example where a test failed (locally) because of an encoded character

This is from line 101 of UserSearchTest

    $this->assertText($blocked_user->getUsername(), 'Blocked users are listed on the user search results for users with the "administer users" permission.');

It's failing because it's looking for the text so'MoCaY but the text in the source is so&#039;MoCaY

The last submitted patch, 51: 2157963-Allow-plus-symbol-in-usernames-50.patch, failed testing.

Noe_’s picture

Status: Needs work » Needs review
FileSize
9.74 KB

Rerolled from comment #51.
It should test ok now.

Status: Needs review » Needs work

The last submitted patch, 58: 21579363-Allow-plus-symbol-in-username-58.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 58: 21579363-Allow-plus-symbol-in-username-58.patch, failed testing.

lokapujya’s picture

Issue summary: View changes

Thanks for rerolling! I think we now need to fix the issue with assertions described in 53-55, possibly by using assertEscaped().

dimaro’s picture

Status: Needs work » Needs review
FileSize
1000 bytes
10.42 KB

I run the test to fail locally and all are correct except "UrlAlterFunctionalTest".

To resolve this error I used the function urldecode in assertUrlOutboundAlter function. (See interdiff)

Hope it works.

Status: Needs review » Needs work

The last submitted patch, 63: allow_symbol_in-2157963-63.patch, failed testing.

dimaro’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
10.32 KB

Update the issue.

I think the error in the test UrlAlterFunctionalTest is because the url is generated differently.

Once you use the same method to get the url, the error disappears, I think.

It would be correct? Any other ideas?

On the other hand the error in the test BulkFormTest not get replicate.

Hope it works!

Status: Needs review » Needs work

The last submitted patch, 65: allow_symbol_in-2157963-65.patch, failed testing.

Status: Needs work » Needs review
dimaro’s picture

In the function drupalCreateUser user name is generated randomly.
This may cause the attached patch is right or maybe not.
I tested locally and it works, but as you can see, on the first try the patch failed.

Needs work?

lokapujya’s picture

What happens if you make the test useso'MoC@Y instead of a random for the username on that test that failed?

dimaro’s picture

FileSize
153.79 KB

I tested with the user "so'MoC@Y" and the result can be seen in the attached image

Status: Needs review » Needs work

The last submitted patch, 65: allow_symbol_in-2157963-65.patch, failed testing.

lokapujya’s picture

The second failed test contained a single quote.

dimaro’s picture

The test failed is "Fastest.php" no "UrlAlterFunctionalTest.php" which was failing before, I think.

lokapujya’s picture

We need to make the test consistently pass. Seems like it still fails randomly?

dimaro: Are you still trying to get it to work?

thtas’s picture

It might be worth temporarily changing the random username generation code to always include a failure character (quote or w/e), just for the purpose of tracking down all the different failure cases. The last thing we want is for this to pass tests based on lucky random names...

jaydee1818’s picture

I have a use case where user accounts in Drupal are created via an API to an external app's usernames. This app allows the '%' symbol to be used in usernames; Drupal does not. Is it possible to have something in the UI that allows choosing of extra symbols that can be allowed in a username?

The last submitted patch, 65: allow_symbol_in-2157963-65.patch, failed testing.

lokapujya’s picture

Issue tags: +Needs reroll
lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.5 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 81: 2157963-78.patch, failed testing.

fuzzy76’s picture

@jaydee1818 I'm pretty sure that a) that should be a separate issue and b) the percentage-sign (%) would require a huge amount of testing and bugfixing to support since it has special meanings both in string-formatting / placeholders and in url encoding. There are a lot of potential side-effects / secondary bugs.

Nikolay Shapovalov’s picture

Assigned: Unassigned » Nikolay Shapovalov

Status: Needs work » Needs review

zniki.ru queued 81: 2157963-78.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 81: 2157963-78.patch, failed testing.

Nikolay Shapovalov’s picture

Nikolay Shapovalov’s picture

Assigned: Nikolay Shapovalov » Unassigned
kekkis’s picture

Status: Needs work » Needs review

Updated status to make the last patch testable.

Status: Needs review » Needs work

The last submitted patch, 87: allow_symbol_in-2157963-87.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Status: Needs work » Needs review
FileSize
11.05 KB
3.22 KB

I guess some tests are randomly failing, because I tested them on local and it passed.

Status: Needs review » Needs work

The last submitted patch, 92: allow_symbol_in-2157963-92.patch, failed testing.

subhojit777’s picture

These tests are passing in my local. Have tested them with 5.5.x and 5.6.x. Any idea why they are failing here.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Anonymous’s picture

After applying the patch, I tried UsersBlockTest locally. First time it gave me a failure, but a different one than for testbot (user 2 instead of user 1), the second and third time it was green.
I didn't look into the patch too much, but it sure does look random to me.

serundeputy’s picture

@pjonckiere @subhojit777
see #70 and #76; the tests randomly fail since the user names being tested are generated at random. sometimes tests have user names with a single quote and sometimes not.

The last submitted patch, 92: allow_symbol_in-2157963-92.patch, failed testing.

nlisgo’s picture

Assigned: Unassigned » nlisgo

Going to see if I can help out on this issue. The first thing I will want to address is ensuring that the test run is consistent.

lokapujya’s picture

Thank You nlisgo! That would be very helpful if the data that causes the failure was used every test run.

Nikolay Shapovalov’s picture

Use asserEscaped instead of assertText when searching for email or username in UserSearchTest.php.
Run UserSearchTest 100 times and 12801 (100%) test passes, 0 fails.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Nikolay Shapovalov’s picture

Status: Needs work » Needs review

Updated status to make the last patch testable.

Status: Needs review » Needs work

The last submitted patch, 102: allow_symbol_in-2157963-102.patch, failed testing.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review
FileSize
800 bytes
14.25 KB

I want check all tests with username having apostrophes.
Just for test purposes, my local tests is very very slow.
Please use #102.

Status: Needs review » Needs work

The last submitted patch, 106: username-with-apostrophes-2157963-106.patch, failed testing.

Nikolay Shapovalov’s picture

Assigned: Unassigned » Nikolay Shapovalov
Nikolay Shapovalov’s picture

There are some failing tests with apostrophes in a username

  • Drupal\file\Tests\FileTokenReplaceTest
  • Drupal\system\Tests\Menu\BreadcrumbTest
  • Drupal\system\Tests\System\AccessDeniedTest
  • Drupal\system\Tests\System\PageNotFoundTest
  • Drupal\system\Tests\Theme\FastTest

So FastTest falls because /user/autocomplete is removed.

BreadcrumbTest falls because UserController::userTitle(), title (username) of user edit page doesn't escaped, and breadcrumb doesn't escaped either.
But AssertBreadcrumbTrait::assertBreadcrumbParts is searching escaped breadcrumb. As I found, try to escape title isn't a good idea, so we should do something with breadcrumb.

Nikolay Shapovalov’s picture

Assigned: Nikolay Shapovalov » Unassigned
Status: Needs work » Needs review
Nikolay Shapovalov’s picture

Issue summary: View changes
Status: Needs review » Needs work

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 109: allow_symbol_in-2157963-109.patch, failed testing.

jenlampton’s picture

Patch in #4 still applies cleanly to 7.39

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 109: allow_symbol_in-2157963-109.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
21.88 KB
1.56 KB

Adding the single quote back until all the fails are gone. Fixing one fail.

Status: Needs review » Needs work

The last submitted patch, 117: 2157963-117.patch, failed testing.

The last submitted patch, 117: 2157963-117.patch, failed testing.

biguzis’s picture

Status: Needs work » Needs review
FileSize
25.54 KB

Fixed some fails. For some was expected user name with apostrophe, but output was with &#039; so I decode those values and i got equal values.

biguzis’s picture

FileSize
3.19 KB

Interdiff

Status: Needs review » Needs work

The last submitted patch, 120: allow_symbol_in-2157963-120.patch, failed testing.

biguzis’s picture

biguzis’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 123: allow_symbol_in-2157963-122.patch, failed testing.

biguzis’s picture

Status: Needs work » Needs review
FileSize
25.07 KB

once again missed correct file. :(
___
okey, some of tests now are passing and new test are failing. Right now failing tests are:
Drupal\search\Tests\SearchConfigSettingsFormTest
Drupal\system\Tests\Menu\BreadcrumbTest
Drupal\simpletest\Tests\SimpleTestTest
Drupal\system\Tests\Theme\FastTest
Drupal\tracker\Tests\TrackerTest
Drupal\user\Tests\UserCancelTest
Drupal\user\Tests\UserPictureTest
Drupal\user\Tests\Views\BulkFormAccessTest

Status: Needs review » Needs work

The last submitted patch, 126: allow_symbol_in-2157963-126.patch, failed testing.

The last submitted patch, 126: allow_symbol_in-2157963-126.patch, failed testing.

The last submitted patch, 120: allow_symbol_in-2157963-120.patch, failed testing.

The last submitted patch, 123: allow_symbol_in-2157963-122.patch, failed testing.

The last submitted patch, 126: allow_symbol_in-2157963-126.patch, failed testing.

Sir-Arturio’s picture

Assigned: Unassigned » Sir-Arturio

Working on this at DrupalCon Barcelona.

Sir-Arturio’s picture

Assigned: Sir-Arturio » Unassigned

Released for someone else.

lokapujya’s picture

Assigned: Unassigned » lokapujya

working on an update.

lokapujya’s picture

Assigned: lokapujya » Unassigned
Status: Needs work » Needs review
FileSize
26.97 KB
1.88 KB

Status: Needs review » Needs work

The last submitted patch, 135: 2157963-134.patch, failed testing.

The last submitted patch, 135: 2157963-134.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
27.26 KB
27.26 KB
<<<<<<< HEAD
    $result = $this->container->get('path_processor_manager')->processOutbound($original);
    return $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
=======
    $result = $this->container->get('url_generator')->generateFromPath(ltrim($original, '/'));
    $final = $this->container->get('url_generator')->generateFromPath(urldecode($final));
    $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
>>>>>>> 126

Keeping change in HEAD, although something might need to be changed later.

The last submitted patch, 138: 2157963-138.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 138: 2157963-138.patch, failed testing.

The last submitted patch, 138: 2157963-138.patch, failed testing.

The last submitted patch, 138: 2157963-138.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
26.38 KB

Status: Needs review » Needs work

The last submitted patch, 143: 2157963-143.patch, failed testing.

The last submitted patch, 143: 2157963-143.patch, failed testing.

biguzis’s picture

Status: Needs work » Needs review
FileSize
29.14 KB
3.84 KB

Fixed some tests

The last submitted patch, 146: 2157963-146.patch, failed testing.

The last submitted patch, 146: 2157963-146.patch, failed testing.

lokapujya’s picture

Status: Needs review » Needs work

The last submitted patch, 149: 2157963-149.patch, failed testing.

The last submitted patch, 149: 2157963-149.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
29.15 KB

Status: Needs review » Needs work

The last submitted patch, 152: 2157963-152.patch, failed testing.

The last submitted patch, 152: 2157963-152.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
29.85 KB
715 bytes

I'm not really sure that we really want to support some of these characters in usernames and whether I should continue patching tests like this.

Status: Needs review » Needs work

The last submitted patch, 155: 2157963-155.patch, failed testing.

The last submitted patch, 155: 2157963-155.patch, failed testing.

biguzis’s picture

As reporter said, there can be situations, when e-mails and usernames are the same and can be imported from some other place (for example using LDAP). And who knows is there some of these character or not, but if there is, then someone wont be able log in. Yes, these emails with apostrophe, plus sign and all other special characters looks weird and there is not much services, that allows some of them. Personally haven't seen any e-mail with apostrophe character.
Don't know if its worth to deal with apostrophe thing.

lokapujya’s picture

Issue summary: View changes
lokapujya’s picture

Issue summary: View changes

delete this comment. So, even if we support some characters, we might still not want to allow them by default.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
30.63 KB
804 bytes

not tested. But, why does the test need to run HTML:escape() here.

Status: Needs review » Needs work

The last submitted patch, 161: 2157963-161.patch, failed testing.

The last submitted patch, 161: 2157963-161.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
30.55 KB
1.57 KB

The first change at least is not outputted to a browser. The second one, not sure.

cilefen’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Task » Feature request
Issue tags: +Needs issue summary update

This issue needs a summary summary update to explain its exact goals. This issue isn't a Task. I contend this is a feature and therefore must be moved to 8.1.x because 8.0 is in RC. If someone wants to make the argument this is a bug, please do.

lokapujya’s picture

Agreed that it's a feature.

subhojit777’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
lokapujya’s picture

Right now the issue summary says allow + symbol. But the actual patch is allowing more characters, so adding back the tag.

Nikolay Shapovalov’s picture

This issue needs a summary summary update to explain its exact goals.

Right now the issue summary says allow + symbol. But the actual patch is allowing more characters, so adding back the tag.

Totally agree.

AaronBauman’s picture

Status: Needs review » Needs work

The last submitted patch, 164: 2157963-164.patch, failed testing.

dimaro’s picture

Issue tags: +Needs reroll
biguzis’s picture

biguzis’s picture

Status: Needs work » Needs review
dimaro’s picture

Issue tags: -Needs reroll
lokapujya’s picture

Apparently , Drupal already supports allows apostrophes. So if we have written some test cases that are failing due to apostrophes then they might be existing bugs.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 173: 2157963-173.patch, failed testing.

dimaro’s picture

Issue tags: +Needs reroll
alexpott’s picture

Category: Feature request » Task
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs reroll
FileSize
1.8 KB

So having + signs in a user name is not a feature. A best this is a task and for some sites they might consider this a bug.

I made a scoping mistake 2 years ago. I shouldn't have expanded this out to include testing things like apostrophe's and more randomness in the names. That should be in a separate issue. If I had kept the scope in check this would have made it into D8. alexpott-- My intentions to have better test coverage were right but my implementation in this issue was very wrong.

Here's a patch that does exactly what we need it to.

Berdir’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

We also have sites that hide the username and use the e-mail for that and I did run into the exact same problem when testing with my usual +something test email.

Patch looks good to me and agree with alex pot that this is a task. Also removing the now unnecessary remaining tasks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 180: 2157963-3-180.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

Rerolled and improved test coverage.

Berdir’s picture

Huh, what is that with the alias thing exactly, how is that related?

Maybe provide a test-only patch if that indeed fails without the fix?

alexpott’s picture

@Berdir since username aliases are common and we're now permitting + in user names which are url encoded it just seems prescient to have a user with a plus sign in the alias test. Nothing should be broken but it just ensures that no assumptions are being made.

Berdir’s picture

Status: Needs review » Needs work

I see. That test has nothing to do with url aliases, it's url altering :) It doesn't create/store aliases, it does runtime inbound/outbound processing.

Can we improve the comment there a bit to explain what's actually going to happen in the test?

jhodgdon’s picture

It might be a good idea to make sure this doesn't break anything with the User search plugin too. There are tests for this plugin in \Drupal\user\Tests\UserSearchTest.

alexpott’s picture

Ensured this is tested for UserSearch... no problems with the plus sign. assertText has a problem with single quotes - the dom manipulator converts all of these :( but as that is not in scope left it out.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! The revised comment looks good, and assuming the tests pass, I think this should be RTBC again.

lokapujya’s picture

Allowing the + simplifies manual testing of sites that automatically use the email as the username. RTBC++

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 188: 2157963-3-188.patch, failed testing.

jhodgdon’s picture

Issue tags: +Needs reroll

Hm. The Postgres failure on the last patch seems like an unrelated problem. But the patch apparently needs a reroll now. We should probably test it against SQLite and PostgreSQL...

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.53 KB
First, rewinding head to replay your work on top of it...
Applying: Fix it
Using index info to reconstruct a base tree...
M	core/modules/user/src/Plugin/Validation/Constraint/UserNameConstraintValidator.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/user/src/Plugin/Validation/Constraint/UserNameConstraintValidator.php
Applying: A little more test
Applying: Add more tests

Simple rebase fixed it.

  • catch committed c36ddb4 on 8.2.x
    Issue #2157963 by lokapujya, alexpott, biguzis, zniki.ru, dimaro, benjf...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

Wim Leers’s picture

What a wonderfully simple patch :)

chrowe’s picture

Issue tags: -Needs reroll

Status: Fixed » Closed (fixed)

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

jenlampton’s picture

Version: 8.2.x-dev » 7.x-dev
Status: Closed (fixed) » Active
Issue tags: +Needs backport to D7

Patch in #4 still applies cleanly to 7.50.

jenlampton’s picture

Status: Active » Reviewed & tested by the community

Not sure if the D7 patch needs additional tests or not, but setting to RTBC for maintainers to decide.

naveenvalecha’s picture

Version: 7.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Closed (fixed)

@jenlampton

I've created the d7 issue https://www.drupal.org/node/2790923 so that issue credits gets preserved here and there as well. Please do the followup there