Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
As per http://drupal.org/simpletest-tutorial-drupal7#t , I scanned the tests in the aggregator module and removed t()s from assert messages, or changed them to format_string() where appropriate.
Patch follows.
Comment | File | Size | Author |
---|---|---|---|
#17 | remove_t.patch | 1.9 KB | droplet |
#10 | remove_t_from_aggregator_asserts_D7-1741328-10.patch | 21.49 KB | lazysoundsystem |
#8 | remove_t_from_aggregator_asserts_D7-1741328-8.patch | 21.3 KB | lazysoundsystem |
#1 | remove-t-from-Aggregator-asserts-1741328-1.patch | 25.39 KB | lazysoundsystem |
Comments
Comment #1
lazysoundsystem CreditAttribution: lazysoundsystem commentedHere's the patch.
Comment #2
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 #3
Lars Toomre CreditAttribution: Lars Toomre commented#1: remove-t-from-Aggregator-asserts-1741328-1.patch queued for re-testing.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedI have reviewed all of the changes in this patch and they are appropriate. This will remove a number of t() calls wrapping assert text strings. Patch is also green so RTBC.
Comment #5
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #6
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #7
xjmWe're backporting these. :)
Comment #8
lazysoundsystem CreditAttribution: lazysoundsystem commentedAnd here's the backport.
Comment #9
lazysoundsystem CreditAttribution: lazysoundsystem commentedHang on, found some ones I missed. I'll submit another patch.
Comment #10
lazysoundsystem CreditAttribution: lazysoundsystem commentedThis one is ready for review.
Comment #11
amonteroPatch applies cleanly.
Grepped for remaining t()'s on assertions and found none left.
Tentatively setting it as RTBC.
Comment #12
webchickTagging, and moving to Jennifer's plate.
Comment #13
jhodgdonRE #10 - do the additional changes you found for 7.x need to be done in Drupal 8.x too? If so, we should go back and do them in 8.x first...
So, can someone please look through the 8.x tests and verify that all of the t() asserts have been removed? If so, set this back to RTBC and I'll get it committed. If not, set this issue to 8.x and we'll need a patch there first. Thanks!
Comment #14
jhodgdonOK, I did the 8.x review myself, and it appears to be clean. So, I committed the above patch to 7.x, since it looks good too. Thanks all!
Comment #15
lazysoundsystem CreditAttribution: lazysoundsystem commentedSorry, @jhodgdon, I had this on my list and was just about to get to it...
Thanks for beating me to it. And all your work checking all these rather repetitive patches.
Comment #17
droplet CreditAttribution: droplet commentedDuring D8 translation, I found the t() used in tests.
** Let me know if it preferred to start a new issue for this fix. Thanks :)
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedwell, a new issue would be better, but since we have it..
Comment #19
lazysoundsystem CreditAttribution: lazysoundsystem commentedAccording to the style guide - https://drupal.org/simpletest-tutorial-drupal7#t, the first parameter here *should* have a t() call.
For assertText(), it's the second parameter (not present here) that gets output on the test result page, that we're removing them from.
Comment #20
jhodgdonYeah, the first change in this patch is incorrect. The other changes are out of scope for this issue, which is about assert messages, not about test modules. We can start another issue called "Do not use t() in modules that are set up for use in tests unless t() is being tested" if you want...