Problem/Motivation

  • Misuse of the method DrupalTestBase::randomString() is frequently responsible for random, hard-to-debug test failures, because it generates a list of random characters that includes special characters like #, *, /, \, ?, etc.
  • Developers often do not consider the implications of using this for values that are not user input or that have already been sanitized, nor do they necessarily know to look for DrupalTestBase::randomName() (which is the "safe" variant).

Proposed resolution

  • Improve the documentation to clarify when each method should be used.

Remaining tasks

  • Patch needs review.
CommentFileSizeAuthor
#24 simpletest-1672764-24.patch1.76 KBiflista
#22 simpletest-1672764-22.patch710 bytesiflista
#20 simpletest-1672764-20.patch13.45 KBiflista
#18 simpletest-1672764-18.patch1.75 KBiflista
#9 1672764-followup.patch816 bytesxjm
#7 drupal-1672764-7.patch1.72 KBtim.plunkett
randomstring-docs.patch1.81 KBxjm

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks reasonable to me, and the documentation is clear.... Maybe I'll wait for a second opinion (or a few days at RTBC) to commit though, in case the policy suggestion here needs tweaking?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

RTBC++

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks! I also just left messages for the two simpletest maintainers in IRC about this issue to see if they want to comment.

chx’s picture

chx commented

Looks good. Do we want namespace anchors already? Or fix 'em later?

dries’s picture

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

Committed to 8.x. Moving to 7.x.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Patch (to be ported)
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new drupal-1672764-7.patch1.72 KB

See attached.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1672764-followup.patch816 bytes

@sun's remarks in #1659682: Consider renaming randomString() and/or adding additional, context-appropriate helpers for random string generation made me realize my wording might inappropriately dissuade developers from using randomString() when they should. The attached is better, I think. Apologies for the extra work.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Seems like a good improvement.

boombatower’s picture

boombatower commented

Seems like good clarification to docs.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Moving back

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Looks good, committed to 8.x, ready to port original + follow-up to 7.x.

Note: the 7.x patch should probably wait on this "avoid commit conflicts" patch for committing:
#1541958: Split setUp() into specific sub-methods

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Hm. I wonder who committed the fix? :)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

@jhodgdon you did, guess Dries didn't push?

http://drupalcode.org/project/drupal.git/commit/586d48e

xjm’s picture

xjm
she/her
Primary language English
commented

Note that #1541958: Split setUp() into specific sub-methods is against 8.x again now.

iflista’s picture

iflista commented
Assigned: Unassigned » iflista
Status: Patch (to be ported) » Needs review
StatusFileSize
new simpletest-1672764-18.patch1.75 KB

Changed comments according to previous patches.
Needs review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks! I think the text in the patch looks good, but this line needs to be wrapped to 80 character maximum:

+   * machine or file names that have already been validated); instead, use DrupalWebTestCase::randomName().

Should be a quick re-roll... I still think we need to wait for #1541958: Split setUp() into specific sub-methods before this is committed also.

iflista’s picture

iflista commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-1672764-20.patch13.45 KB

Fixed few more comments longer than 80 character.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Please limit the patch on this issue to fixing just this one issue in 7.x that was already fixed in 8.x.

Other documentation cleanups are being addressed on:
#1310084: [meta] API documentation cleanup sprint
If you'd like to help with that other issue, that would be great! Instructions are at the top of that issue. Thanks!

iflista’s picture

iflista commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-1672764-22.patch710 bytes

Done.

Status: Needs review » Needs work

The last submitted patch, simpletest-1672764-22.patch, failed testing.

iflista’s picture

iflista commented
StatusFileSize
new simpletest-1672764-24.patch1.76 KB

Second try.

iflista’s picture

iflista commented
Status: Needs work » Needs review

Ups. Needs review! :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Improve documetation of randomString() and randomName() » Improve documentation of randomString() and randomName()
Status: Needs review » Reviewed & tested by the community

Looks good! I'll get this committed shortly.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Oh, I can't commit this until
#1541958: Split setUp() into specific sub-methods
is done.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

That patch is now in, and it looks like it didn't wind up conflicting (but thanks for waiting anyway).

So, I went ahead and committed this one to 7.x. Thanks! http://drupalcode.org/project/drupal.git/commit/1c2301b

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.