Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#24 | simpletest-1672764-24.patch | 1.76 KB | iflista |
#22 | simpletest-1672764-22.patch | 710 bytes | iflista |
#20 | simpletest-1672764-20.patch | 13.45 KB | iflista |
#18 | simpletest-1672764-18.patch | 1.75 KB | iflista |
#9 | 1672764-followup.patch | 816 bytes | xjm |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #1
jhodgdonThis 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?
Comment #2
tstoecklerRTBC++
Comment #3
jhodgdonThanks! I also just left messages for the two simpletest maintainers in IRC about this issue to see if they want to comment.
Comment #4
chx CreditAttribution: chx commentedLooks good. Do we want namespace anchors already? Or fix 'em later?
Comment #5
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x.
Comment #6
xjmComment #7
tim.plunkettSee attached.
Comment #8
chx CreditAttribution: chx commentedComment #9
xjm@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.Comment #10
tim.plunkettSeems like a good improvement.
Comment #11
boombatower CreditAttribution: boombatower commentedSeems like good clarification to docs.
Comment #12
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #13
tim.plunkettMoving back
Comment #14
jhodgdonLooks 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
Comment #15
jhodgdonHm. I wonder who committed the fix? :)
Comment #16
tim.plunkett@jhodgdon you did, guess Dries didn't push?
http://drupalcode.org/project/drupal.git/commit/586d48e
Comment #17
xjmNote that #1541958: Split setUp() into specific sub-methods is against 8.x again now.
Comment #18
iflista CreditAttribution: iflista commentedChanged comments according to previous patches.
Needs review.
Comment #19
jhodgdonThanks! I think the text in the patch looks good, but this line needs to be wrapped to 80 character maximum:
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.
Comment #20
iflista CreditAttribution: iflista commentedFixed few more comments longer than 80 character.
Comment #21
jhodgdonPlease 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!
Comment #22
iflista CreditAttribution: iflista commentedDone.
Comment #24
iflista CreditAttribution: iflista commentedSecond try.
Comment #25
iflista CreditAttribution: iflista commentedUps. Needs review! :)
Comment #26
jhodgdonLooks good! I'll get this committed shortly.
Comment #27
jhodgdonOh, I can't commit this until
#1541958: Split setUp() into specific sub-methods
is done.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedThat 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
Comment #29.0
(not verified) CreditAttribution: commentedUpdated issue summary.