Removing t() from asserts in simpletests in comment module

#1: remove-t-from-Comment-asserts-1742602-1.patch queued for re-testing.

Rerolling this. I also reviewed from the bottom up through CommentNodeAccessTest and found a couple things to fix:

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -224,7 +224,7 @@ abstract class CommentTestBase extends WebTestBase {
-    $this->assertTrue(TRUE, t($message)); // Display status message.
+    $this->assertTrue(TRUE, format_string('@message', array('@message' => $message))); // Display status message.

Let's move that inline comment up above while we're at it. (Also, wow, what a... special line of code.)

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -255,10 +255,10 @@ abstract class CommentTestBase extends WebTestBase {
-      $this->assertText(t('The update has been performed.'), t('Operation "' . $operation . '" was performed on comment.'));
+      $this->assertText(t('The update has been performed.'), t('Operation "@operation" was performed on comment.', array('@operation' => $operation)));

This one should be switched to format_string() as well.

-1 days to next Drupal core point release.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAnonymousTest.phpundefined
@@ -77,41 +77,41 @@ class CommentAnonymousTest extends CommentTestBase {
-    $this->assertText(t('E-mail field is required.'), t('E-mail required.')); // Name should have 'Anonymous' for value by default.
...
+    $this->assertText(t('E-mail field is required.'), 'E-mail required.'); // Name should have 'Anonymous' for value by default.

Another goofy trailing comment.

I reviewed the whole patch and didn't find any mistakes other than the one in #6. Rebased, resolved a few merge conflicts, and then cleaned up the above. Interdiff is after the rebase.

@xjm When I reviewed this patch the other day, I did not focus on the stray comments. If I see them again, should I correct those as well? Or is this effort just strictly t() and format_string() focused?

I did not see any errors the other day (other than problems due to a rebase) and I do not see any issues with any of your changes. Let's wait for this to come back green before RTBC.

This is RBTC. As noted in #8, I checked all of the message changes and they are correct.

Yeah, I just moved them because they were annoying me and the interdiff was small. :)

Putting in @jhodgon's box for whenever.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -224,7 +224,8 @@ abstract class CommentTestBase extends WebTestBase {
   function setCommentSettings($name, $value, $message) {
     variable_set($name . '_article', $value);
-    $this->assertTrue(TRUE, t($message)); // Display status message.
+    // Display status message.
+    $this->assertTrue(TRUE, format_string('@message', array('@message' => $message)));

Okay, so two things on this assertion:

1. It actually shouldn't be formatting the message; that's on the caller. (When rolling the patch for the RDF module, I discovered that the message was actually getting translated twice!)
2. It should be a $this->pass() anyway.

That's not really in scope for this issue, so I'm going to rip out this change and it can be handled in a followup.

Followup is here: #1798066: Clean up CommentTestBase::setCommentSettings()

Attached patch simply removes the change to that method, so this should still be RTBC.

OK, committed to 8.x, thanks!

I gather that comment.module is done for d8 and is ready for D7 except for the followup that is a separate issue now?

Dang it I cannot seem to remember to unassign these issues. Sorry for the traffic.

While reviewing the D8 Tests for Comment module, I found that we missed a few of t() assert corrections. Attached is a patch that cleans up those three (plus moving a stray in-line comment at end of line).

I can probably take care of committing this tomorrow. Thanks!

Thanks! Committed that extra patch to 8.x; back to 7.x now.

Backported #15 to D7.

Thanks @lazysoundsystem. I am glad that this is ready to RTBC.

If you have a chance, can you review some of the open sub-issue patches from #500866: [META] remove t() from assert message? Most of the open D8 patches were created by me and hence I cannot review/RTBC. Once those sub-issue patches get committed to D8, more patches like this one can be rolled for D7. Thanks in advance for your help.

Passing to Jennifer.

Thanks! Patch in #19 is committed to 7.x.

Given that #19 said it was a port of the patch in #15, and there was an earlier patch in #12 that was also committed to 8.x previously, can someone please review comment.test in D7 and verify that all the t() assert messages from #12 have also been taken care of? If so, set this to "fixed". If not, the patch in #12 now needs a backport. Thanks!

#19 is a backport of #12 and #15. I misspoke in my post in #19. Still, it should be checked, per jhodgdon's request.

#19: comment-1742602-19.patch queued for re-testing.

#19 was committed and didn't need to be retested. My mistake. Sorry. It still needs to be checked as mentioned in #23.

Tagging as Novice.

Diffstat from #19 (7.x backport): 1 files changed, 186 insertions, 184 deletions.
Diffstat from #12 (first 8.x patch): 13 files changed, 181 insertions, 180 deletions.
Diffstat from #15 (followup 8.x patch): 3 files changed, 4 insertions, 4 deletions.

I also grepped and skimmed through modules/comment/comment.test in 7.x and didn't see any assertion messages wrapped in t(). This is done :)

Thanks Cottser!