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.
This is a sub-task of #500866: [META] remove t() from assert message focused on the System sub-system.
In D8 per http://drupal.org/simpletest-tutorial-drupal7#t, best practice is to remove t() from assert messages in tests. When necessary, t() should be replaced with format_string().
This issue is to correct the System sub-system tests for the above. There are approximately 127 changes needed (including format_string() changes) changing 22 Test files.
Comment | File | Size | Author |
---|---|---|---|
#14 | 1797926-14-t-assert-system.patch | 47.89 KB | dcam |
#9 | 1797926-9-t-assert-system.patch | 56.09 KB | Lars Toomre |
#4 | 1797926-4-t-assert-system.patch | 55.82 KB | Lars Toomre |
#1 | t-asset-system-1797926-1.patch | 55.75 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue. This patch includes format_string() conversions as well.
The format_string() conversions appear to be correct, but are worthy of a closer look in the review process. A couple of them confused me at first and I had to convert some concatenations to escaped strings.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedLooks like #1798756: Finish conversion of error_level to CMI added another t(). That will need to be removed.
Let's first see what reviewers of this patch have to say.
Comment #3
dcam CreditAttribution: dcam commented#2 no longer applies due to changes in InfoAlterTest.php.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a re-roll of the patch from #1. It also includes two additional fixes from the ErrorHandlerTest.php that were referenced in #2.
I did not go back and recheck that there were no other t() around assert messages. There might well have been some added since #1 was first rolled about three weeks ago. However, #1 was complete as of its creation time.
Thanks for trying to review this @dcam. Let me know what else you think needs to be changed.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedDrupal\file\Tests\UsageTest test failure looks like it is a random test failure.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commented#4: 1797926-4-t-assert-system.patch queued for re-testing.
Comment #8
dcam CreditAttribution: dcam commentedI tested #4. There's an addtional t() to eliminate in SiteMaintenanceTest.php, line 103.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the review @dcam. Here is an updated patch with that one additional change.
Comment #10
dcam CreditAttribution: dcam commented#9 looks good. I didn't find any additional t()'s around assert messages.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedThanks again for the review @dcam! I glad that we are getting the D8 patches completed.
Comment #12
webchickComment #13
jhodgdonThanks! Committed to 8.x.
Comment #14
dcam CreditAttribution: dcam commentedBackported #9 to D7.
I tried to stick to backporting only the changes made in #9. There are other t()'s around assert messages in system.test, but they may be handled by other issues with patches that are being backported. system.test will probably need to be checked for remaining t()'s after all system-test-related issues have been committed.
Comment #15
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #16
izus CreditAttribution: izus commented#14: 1797926-14-t-assert-system.patch queued for re-testing.
Comment #17
izus CreditAttribution: izus commentedHi,
#14 seems good.
Thanks
Comment #18
jhodgdonThanks! Committed to 7.x.
Comment #19.0
(not verified) CreditAttribution: commentedUpdated initial counts.