As per http://drupal.org/simpletest-tutorial-drupal7#t , I scanned the tests in the comment module and removed t()s from assert messages, or changed them to format_string() where appropriate.

Patch follows.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lazysoundsystem’s picture

Here is the patch.

lazysoundsystem’s picture

Status: Active » Needs review

Setting to 'needs review'

lazysoundsystem’s picture

I started this before reading through #500866: [META] remove t() from assert message, where they've been trying to solve this since 2009. Best to wait for confirmation there before putting any more work into this.

Lars Toomre’s picture

Status: Needs review » Needs work

The last submitted patch, remove-t-from-Comment-asserts-1742602-1.patch, failed testing.

xjm’s picture

Assigned: Unassigned » xjm
Issue tags: +Needs backport to D7

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
65.71 KB
+++ 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.

Lars Toomre’s picture

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

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

Assigned: xjm » jhodgdon

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

Putting in @jhodgon's box for whenever.

xjm’s picture

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

xjm’s picture

FileSize
710 bytes
65.33 KB

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

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

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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?

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

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

Lars Toomre’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
FileSize
2.89 KB

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

lazysoundsystem’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Lars Toomre - these are all good. And I have the D7 backport waiting for when these are in.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

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

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

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

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
61.88 KB

Backported #15 to D7.

lazysoundsystem’s picture

Status: Needs review » Reviewed & tested by the community

I've looked through all the changes and they're all appropriate. RTBC.

Lars Toomre’s picture

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.

webchick’s picture

Assigned: Unassigned » jhodgdon

Passing to Jennifer.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

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!

dcam’s picture

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

dcam’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D7

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

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, comment-1742602-19.patch, failed testing.

dcam’s picture

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

dcam’s picture

Status: Needs work » Patch (to be ported)
dcam’s picture

Issue tags: +Novice

Tagging as Novice.

star-szr’s picture

Status: Patch (to be ported) » Fixed
Issue tags: -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 :)

jhodgdon’s picture

Thanks Cottser!

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