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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jwilson3 created an issue. See original summary.

jwilson3’s picture

The hunk in question:

diff --git a/modules/dblog/dblog.test b/modules/dblog/dblog.test
index bf409c9..03308af 100644
--- a/modules/dblog/dblog.test
+++ b/modules/dblog/dblog.test
@@ -119,13 +119,16 @@ class DBLogTestCase extends DrupalWebTestCase {
   private function generateLogEntries($count, $type = 'custom', $severity = WATCHDOG_NOTICE) {
     global $base_root;
 
+    // Make it just a little bit harder to pass the link part of the test.
+    $link = urldecode('/content/xo%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A-lake-isabelle');
+
     // Prepare the fields to be logged
     $log = array(
       'type'        => $type,
       'message'     => 'Log entry added to test the dblog row limit.',
       'variables'   => array(),
       'severity'    => $severity,
-      'link'        => NULL,
+      'link'        => $link,
       'user'        => $this->big_user,
       'uid'         => isset($this->big_user->uid) ? $this->big_user->uid : 0,
       'request_uri' => $base_root . request_uri(),
cilefen’s picture

Title: Better documentation for suspicious encrypted code in dblog.test » Better documentation for suspicious encoded string in dblog.test

It is not encrypted code, it is an encoded string.

cilefen’s picture

It is test code. Why is it suspicious?

jwilson3’s picture

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

jwilson3’s picture

It appears that the decoded text is actually semi-random Chinese characters:

xo酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊酱凉拌素鸡照烧鸡黄玫瑰-科州的小九寨沟绝美高山湖泊-lake-isabelle

Which translates to:

xo sauce chicken salad Chicken photo yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of Chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of Chicken salad sauce chicken yellow roses - Branch state small beautiful mountain lake in Jiuzhaigou photo chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful sauce chicken salad according chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Jiuzhaigou absolutely Colorado US alpine lakes butter chicken salad chicken photo yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Colorado mountain lake in Jiuzhaigou beautiful photo of chicken salad sauce chicken yellow roses - a small Jiuzhaigou beautiful Colorado mountain lake -lake -isabelle
David_Rothstein’s picture

Version: 7.42 » 8.0.x-dev
Issue tags: +Needs backport to D7

Ha, 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.

David_Rothstein’s picture

Title: Better documentation for suspicious encoded string in dblog.test » Better documentation for suspicious encoded string in dblog tests

It's in core/modules/dblog/src/Tests/DbLogTest.php for Drupal 8.

jwilson3’s picture

How about 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.
jwilson3’s picture

Status: Active » Needs review
cilefen’s picture

That sounds better! When you make the next patch it is necessary to wrap the lines closer to the 80-column limit.

+++ b/core/modules/dblog/src/Tests/DbLogTest.php
@@ -153,7 +153,9 @@ private function verifyCron($row_limit) {
+    // 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

As an example, "of" can fit on the first line of the comment, and so on.

jwilson3’s picture

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

cilefen’s picture

@Cilefen. Is that your personal suggestion, or a requirement?

It is a requirement. From https://www.drupal.org/coding-standards/docs:

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over...

There are exceptions but they do not apply in this case.

jwilson3’s picture

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed faccd21 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 395cb8b on 8.2.x
    Issue #2671344 by jwilson3: Better documentation for suspicious encoded...

  • alexpott committed faccd21 on 8.1.x
    Issue #2671344 by jwilson3: Better documentation for suspicious encoded...
alexpott’s picture

I think as per the new backport policy the Drupal 7 backport should be a new issue.

David_Rothstein’s picture

Status: Fixed » Patch (to be ported)

Yes, but this should stay open until the other issue is created.

dagmar’s picture

Status: Patch (to be ported) » Fixed
David_Rothstein’s picture

Issue tags: -Needs backport to D7

Thanks!

Status: Fixed » Closed (fixed)

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