Problem/Motivation
When Updating from 7.41 to 7.42 I always review the updates by hand, staging each hunk... and I must say that this hunk (see comment 1 below) stands out and looks a LOT like a hack, containing a very lengthy url-encoded (non-human readable) string.
The comment for this string is simply:
Make it just a little bit harder to pass the link part of the test.
Even though this might be innocuous, this hunk is confusing and caused me do a double take.
Proposed resolution
Improve the documentation to be clear about exactly what this long encoded string is for, in the comment directly preceding the string.
Patch in #9 proposes the following text:
// This long URL makes it just a little bit harder to pass the link part
// of the test with a mix of English words and a repeating series of
// random percent-encoded Chinese characters.
Remaining tasks
Improve verbiage.
Write patch.
Review patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2671344-improve-dblog-link-test-documentation-d8-14.patch | 3.5 KB | jwilson3 |
#9 | 2671344-improve-dblog-link-test-documentation-d8-9.patch | 3.5 KB | jwilson3 |
Comments
Comment #2
jwilson3The hunk in question:
Comment #3
cilefen CreditAttribution: cilefen as a volunteer commentedIt is not encrypted code, it is an encoded string.
Comment #4
cilefen CreditAttribution: cilefen as a volunteer commentedIt is test code. Why is it suspicious?
Comment #5
jwilson3Why is it suspicious?
Because it's not a human readable string, and the comment doesn't explain what the content is, requiring someone to have to push it through a decoder to actually see.
Comment #6
jwilson3It appears that the decoded text is actually semi-random Chinese characters:
Which translates to:
Comment #7
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHa, I remember checking what this translated to before committing it to Drupal 7 also - just to make sure nothing too evil snuck in there :)
I agree a better code comment would be a good idea, but this should be done in Drupal 8 first since that's where it came from.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt's in core/modules/dblog/src/Tests/DbLogTest.php for Drupal 8.
Comment #9
jwilson3How about the following text?
Comment #10
jwilson3Comment #11
cilefen CreditAttribution: cilefen as a volunteer commentedThat sounds better! When you make the next patch it is necessary to wrap the lines closer to the 80-column limit.
As an example, "of" can fit on the first line of the comment, and so on.
Comment #12
jwilson3@Cilefen. Is that your personal suggestion, or a requirement? I originally had it closer to the 80 col limit, but since it couldn't get onto 2 lines anyway, I opted for sending the "of" down to line two and formatting the paragraph for a better overall shape instead of having a very short third line.
Comment #13
cilefen CreditAttribution: cilefen as a volunteer commentedIt is a requirement. From https://www.drupal.org/coding-standards/docs:
There are exceptions but they do not apply in this case.
Comment #14
jwilson3Fixed. Thanks!
Comment #16
dagmarLooks good, thanks!
Comment #17
alexpottCommitted faccd21 and pushed to 8.1.x and 8.2.x. Thanks!
Comment #20
alexpottI think as per the new backport policy the Drupal 7 backport should be a new issue.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYes, but this should stay open until the other issue is created.
Comment #22
dagmarHere is the backport. #2718179: Better documentation for suspicious encoded string in dblog tests
Comment #23
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks!