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.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | comment-1742602-19.patch | 61.88 KB | dcam |
#15 | 1742602-15-t-comment.patch | 2.89 KB | Lars Toomre |
#12 | comment-1742602-12.patch | 65.33 KB | xjm |
#12 | interdiff.txt | 710 bytes | xjm |
#7 | comment-1742602-7.patch | 65.71 KB | xjm |
Comments
Comment #1
lazysoundsystem CreditAttribution: lazysoundsystem commentedHere is the patch.
Comment #2
lazysoundsystem CreditAttribution: lazysoundsystem commentedSetting to 'needs review'
Comment #3
lazysoundsystem CreditAttribution: lazysoundsystem commentedI 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.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commented#1: remove-t-from-Comment-asserts-1742602-1.patch queued for re-testing.
Comment #6
xjmRerolling this. I also reviewed from the bottom up through CommentNodeAccessTest and found a couple things to fix:
Let's move that inline comment up above while we're at it. (Also, wow, what a... special line of code.)
This one should be switched to
format_string()
as well.-1 days to next Drupal core point release.
Comment #7
xjmAnother 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.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedThis is RBTC. As noted in #8, I checked all of the message changes and they are correct.
Comment #10
xjmYeah, I just moved them because they were annoying me and the interdiff was small. :)
Putting in @jhodgon's box for whenever.
Comment #11
xjmOkay, so two things on this assertion:
$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.
Comment #12
xjmFollowup is here: #1798066: Clean up CommentTestBase::setCommentSettings()
Attached patch simply removes the change to that method, so this should still be RTBC.
Comment #13
jhodgdonOK, 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?
Comment #14
jhodgdonDang it I cannot seem to remember to unassign these issues. Sorry for the traffic.
Comment #15
Lars Toomre CreditAttribution: Lars Toomre commentedWhile 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).
Comment #16
lazysoundsystem CreditAttribution: lazysoundsystem commentedThanks @Lars Toomre - these are all good. And I have the D7 backport waiting for when these are in.
Comment #17
jhodgdonI can probably take care of committing this tomorrow. Thanks!
Comment #18
jhodgdonThanks! Committed that extra patch to 8.x; back to 7.x now.
Comment #19
dcam CreditAttribution: dcam commentedBackported #15 to D7.
Comment #20
lazysoundsystem CreditAttribution: lazysoundsystem commentedI've looked through all the changes and they're all appropriate. RTBC.
Comment #21
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.
Comment #22
webchickPassing to Jennifer.
Comment #23
jhodgdonThanks! 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!
Comment #24
dcam CreditAttribution: dcam commented#19 is a backport of #12 and #15. I misspoke in my post in #19. Still, it should be checked, per jhodgdon's request.
Comment #25
dcam CreditAttribution: dcam commented#19: comment-1742602-19.patch queued for re-testing.
Comment #27
dcam CreditAttribution: dcam commented#19 was committed and didn't need to be retested. My mistake. Sorry. It still needs to be checked as mentioned in #23.
Comment #28
dcam CreditAttribution: dcam commentedComment #29
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #30
star-szrDiffstat 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 :)
Comment #31
jhodgdonThanks Cottser!