Posted by xjm

Problem/Motivation

The setCommentSettings() method in the base comment test class looks like this:

function setCommentSettings($name, $value, $message) {
  variable_set($name . '_article', $value);
  $this->assertTrue(TRUE, t($message)); // Display status message.
}

There are several things wrong with this:

  1. The $this->assertTrue(TRUE) should just be $this->pass().
  2. A variable should not be stuck directly in a t() like that.
  3. In general, t() on assertions that use variables should be replaced with format_string(). Reference: http://drupal.org/simpletest-tutorial-drupal7#t
  4. But actually, we shouldn't be formatting the assertion message at all. That's on the caller.
  5. Also, that inline comment should not be hanging off the end of the line.

See: #1742602: Removing t() from asserts in simpletests in comment module and #1797506: Remove t() from assertion messages in tests for the rdf module

Proposed resolution

  1. Use $this->pass() instead of $this->assertTrue(TRUE).
  2. Remove the t() from the $message parameter in the assertion.
  3. Move the inline comment above the line.
  4. Check the usage of setCommentSetting() throughout core and check whether any callers need to use format_string() on their message strings (e.g., there are variables to output in the message). If so, add it. If not, remove any use of t() on plain-text messages for that parameter.

Remaining tasks

  • Create patch.
CommentFileSizeAuthor
#19 1798066-19-comment-t.patch8.26 KBdcam
#12 1798066-clean-commenttestbase-12.patch9.93 KBboztek
#11 1798066-clean-commenttestbase-11.patch9.93 KBboztek
#9 1798066-comment-cleanup.9.patch8.53 KBlarowlan
#3 1798066-clean-commenttestbase-3.patch8.61 KBboztek
#2 1798066-clean-commenttestbase-2.patch8.61 KBboztek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

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

Updated issue summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: +#pnx-sprint

Tagging

boztek’s picture

boztek CreditAttribution: boztek commented
Status: Active » Needs review
FileSize
1798066-clean-commenttestbase-2.patch8.61 KB

Implemented proposed solution.

boztek’s picture

boztek CreditAttribution: boztek commented
FileSize
1798066-clean-commenttestbase-3.patch8.61 KB

Switched array index from double to single quote.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented

These changes may conflict with #1742602: Removing t() from asserts in simpletests in comment module. However, both this and that issue should be combined when producing a patch to backport to D7.

I reviewed this patch as well and agree that all changes are appropriate and correct. This is good to go. +RTBC!

Dries’s picture

Dries CreditAttribution: Dries commented

#3: 1798066-clean-commenttestbase-3.patch queued for re-testing.

Dries’s picture

Dries CreditAttribution: Dries commented

Asked for a re-test as I don't think this patch applies. Might require a quick reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1798066-clean-commenttestbase-3.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
1798066-comment-cleanup.9.patch8.53 KB
xjm’s picture

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

So there are two more to clean up I think:

core/modules/comment/lib/Drupal/comment/Tests/CommentThreadingTest.php:    $this->setCommentSettings('comment_default_mode', COMMENT_MODE_THREADED, t('Comment paging changed.'));
core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php:    $this->setCommentSettings('comment_default_mode', COMMENT_MODE_THREADED, t('Comment paging changed.'));
boztek’s picture

boztek CreditAttribution: boztek commented
Status: Needs work » Needs review
FileSize
1798066-clean-commenttestbase-11.patch9.93 KB

Can't see anymore; that should be all of them.

boztek’s picture

boztek CreditAttribution: boztek commented
FileSize
1798066-clean-commenttestbase-12.patch9.93 KB 
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -199,7 +199,7 @@ function setCommentForm($enabled) {
+    $this->setCommentSettings('comment_anonymous', $level, format_string('Anonymous commenting set to level @level.', array('level' => $level)));

Spoke too soon - should be '@level' => $level

xjm’s picture

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

Thanks @boztek. This looks ready now.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Looks goot. Committed to 8.x.

tim.plunkett’s picture

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

This wasn't pushed.

Dries, please git pull --rebase before you push this, thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed *and pushed* to 8.x. :D Thanks.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)

It looks like some of this made it into Drupal 7 (probably via other issues), but not all of it.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Patch (to be ported) » Needs review
FileSize
1798066-19-comment-t.patch8.26 KB

Backported #12 to D7.

dcam’s picture

dcam CreditAttribution: dcam commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

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

Removing myself from the author field to unfollow the issue. --xjm

parthipanramesh’s picture

parthipanramesh CreditAttribution: parthipanramesh commented

Seems to work fine. Thank you.

parthipanramesh’s picture

parthipanramesh CreditAttribution: parthipanramesh commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

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

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/25875f9

Status: Fixed » Closed (fixed)

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