Download & Extend
Issues

Improve documentation of randomString() and randomName()

Posted by xjm on July 5, 2012 at 4:20pm
Project:Drupal core
Version:7.x-dev
Component:documentation
Category:task
Priority:normal
Assigned:iflista
Status:closed (fixed)
Issue tags:needs backport to D7

Issue Summary

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.
AttachmentSizeStatusTest resultOperations
randomstring-docs.patch1.81 KBIdlePASSED: [[SimpleTest]]: [MySQL] 37,057 pass(es).View details
Login or register to post comments

Comments

#1

Posted by jhodgdon on July 5, 2012 at 4:36pm
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?

#2

Posted by tstoeckler on July 6, 2012 at 12:17am

RTBC++

#3

Posted by jhodgdon on July 6, 2012 at 2:48pm

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

#4

Posted by chx on July 7, 2012 at 6:37am

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

#5

Posted by Dries on July 7, 2012 at 1:27pm
Version:8.x-dev» 7.x-dev

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

#6

Posted by xjm on July 7, 2012 at 3:00pm
Status:reviewed & tested by the community» patch (to be ported)

#7

Posted by tim.plunkett on July 7, 2012 at 9:51pm
Status:patch (to be ported)» needs review

See attached.

AttachmentSizeStatusTest resultOperations
drupal-1672764-7.patch1.72 KBIdlePASSED: [[SimpleTest]]: [MySQL] 39,251 pass(es).View details

#8

Posted by chx on July 7, 2012 at 11:28pm
Status:needs review» reviewed & tested by the community

#9

Posted by xjm on July 8, 2012 at 11:20am
Version:7.x-dev» 8.x-dev
Status:reviewed & tested by the community» needs review

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

AttachmentSizeStatusTest resultOperations
1672764-followup.patch816 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 37,262 pass(es).View details

#10

Posted by tim.plunkett on July 9, 2012 at 2:03pm
Status:needs review» reviewed & tested by the community

Seems like a good improvement.

#11

Posted by boombatower on July 9, 2012 at 6:17pm

Seems like good clarification to docs.

#12

Posted by Dries on July 9, 2012 at 6:31pm
Status:reviewed & tested by the community» fixed

Committed to 8.x. Thanks.

#13

Posted by tim.plunkett on July 9, 2012 at 6:37pm
Version:8.x-dev» 7.x-dev
Status:fixed» patch (to be ported)

Moving back

#14

Posted by jhodgdon on July 9, 2012 at 6:51pm

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

#15

Posted by jhodgdon on July 9, 2012 at 6:51pm

Hm. I wonder who committed the fix? :)

#16

Posted by tim.plunkett on July 9, 2012 at 6:54pm

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

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

#17

Posted by xjm on July 10, 2012 at 1:25am

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

#18

Posted by iflista on July 11, 2012 at 6:10pm
Assigned to:Anonymous» iflista
Status:patch (to be ported)» needs review

Changed comments according to previous patches.
Needs review.

AttachmentSizeStatusTest resultOperations
simpletest-1672764-18.patch1.75 KBIdlePASSED: [[SimpleTest]]: [MySQL] 39,265 pass(es).View details

#19

Posted by jhodgdon on July 12, 2012 at 2:07pm
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.

#20

Posted by iflista on July 12, 2012 at 2:34pm
Status:needs work» needs review

Fixed few more comments longer than 80 character.

AttachmentSizeStatusTest resultOperations
simpletest-1672764-20.patch13.45 KBIdlePASSED: [[SimpleTest]]: [MySQL] 39,252 pass(es).View details

#21

Posted by jhodgdon on July 12, 2012 at 10:51pm
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!

#22

Posted by iflista on July 13, 2012 at 4:35pm
Status:needs work» needs review

Done.

AttachmentSizeStatusTest resultOperations
simpletest-1672764-22.patch710 bytesIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest-1672764-22.patch. Unable to apply patch. See the log in the details link for more information.View details

#23

Posted by System Message on July 13, 2012 at 4:43pm
Status:needs review» needs work

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

#24

Posted by iflista on July 13, 2012 at 8:33pm

Second try.

AttachmentSizeStatusTest resultOperations
simpletest-1672764-24.patch1.76 KBIdlePASSED: [[SimpleTest]]: [MySQL] 39,259 pass(es).View details

#25

Posted by iflista on July 13, 2012 at 8:34pm
Status:needs work» needs review

Ups. Needs review! :)

#26

Posted by jhodgdon on July 16, 2012 at 3:43pm
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.

#27

Posted by jhodgdon on July 16, 2012 at 4:33pm

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

#28

Posted by David_Rothstein on July 30, 2012 at 2:54am
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

#29

Posted by System Message on August 13, 2012 at 3:01am
Status:fixed» closed (fixed)

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

nobody click here