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.
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:
- The
$this->assertTrue(TRUE)
should just be $this->pass(). - A variable should not be stuck directly in a
t()
like that. - In general,
t()
on assertions that use variables should be replaced with format_string(). Reference: http://drupal.org/simpletest-tutorial-drupal7#t - But actually, we shouldn't be formatting the assertion message at all. That's on the caller.
- 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
- Use
$this->pass()
instead of$this->assertTrue(TRUE)
. - Remove the
t()
from the$message
parameter in the assertion. - Move the inline comment above the line.
- Check the usage of
setCommentSetting()
throughout core and check whether any callers need to useformat_string()
on their message strings (e.g., there are variables to output in the message). If so, add it. If not, remove any use oft()
on plain-text messages for that parameter.
Remaining tasks
- Create patch.
Comment | File | Size | Author |
---|---|---|---|
#19 | 1798066-19-comment-t.patch | 8.26 KB | dcam |
#12 | 1798066-clean-commenttestbase-12.patch | 9.93 KB | boztek |
#11 | 1798066-clean-commenttestbase-11.patch | 9.93 KB | boztek |
#9 | 1798066-comment-cleanup.9.patch | 8.53 KB | larowlan |
#3 | 1798066-clean-commenttestbase-3.patch | 8.61 KB | boztek |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #1
larowlanTagging
Comment #2
boztek CreditAttribution: boztek commentedImplemented proposed solution.
Comment #3
boztek CreditAttribution: boztek commentedSwitched array index from double to single quote.
Comment #4
larowlanLooks good to me.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedThese 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!
Comment #6
Dries CreditAttribution: Dries commented#3: 1798066-clean-commenttestbase-3.patch queued for re-testing.
Comment #7
Dries CreditAttribution: Dries commentedAsked for a re-test as I don't think this patch applies. Might require a quick reroll.
Comment #9
larowlanComment #10
xjmSo there are two more to clean up I think:
Comment #11
boztek CreditAttribution: boztek commentedCan't see anymore; that should be all of them.
Comment #12
boztek CreditAttribution: boztek commentedSpoke too soon - should be '@level' => $level
Comment #13
xjmThanks @boztek. This looks ready now.
Comment #14
Dries CreditAttribution: Dries commentedLooks goot. Committed to 8.x.
Comment #15
tim.plunkettThis wasn't pushed.
Dries, please
git pull --rebase
before you push this, thanks!Comment #16
webchickCommitted *and pushed* to 8.x. :D Thanks.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedIt looks like some of this made it into Drupal 7 (probably via other issues), but not all of it.
Comment #19
dcam CreditAttribution: dcam commentedBackported #12 to D7.
Comment #19.0
dcam CreditAttribution: dcam commentedUpdated issue summary.
Comment #19.1
xjmRemoving myself from the author field to unfollow the issue. --xjm
Comment #20
parthipanramesh CreditAttribution: parthipanramesh commentedSeems to work fine. Thank you.
Comment #21
parthipanramesh CreditAttribution: parthipanramesh commentedComment #22
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/25875f9