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.

CommentFileSizeAuthor
#17 remove_t.patch1.9 KBdroplet
#10 remove_t_from_aggregator_asserts_D7-1741328-10.patch21.49 KBlazysoundsystem
#8 remove_t_from_aggregator_asserts_D7-1741328-8.patch21.3 KBlazysoundsystem
#1 remove-t-from-Aggregator-asserts-1741328-1.patch25.39 KBlazysoundsystem

Comments

lazysoundsystem’s picture

lazysoundsystem commented
Status: Active » Needs review
StatusFileSize
new remove-t-from-Aggregator-asserts-1741328-1.patch25.39 KB

Here's the patch.

lazysoundsystem’s picture

lazysoundsystem commented

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

lars toomre commented

#1: remove-t-from-Aggregator-asserts-1741328-1.patch queued for re-testing.

lars toomre’s picture

lars toomre commented
Title: Removing t() from asserts in simpletests » Remove t() from asserts messages in tests for aggregator module
Status: Needs review » Reviewed & tested by the community

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

dries’s picture

dries commented

Committed to 8.x. Thanks.

Tor Arne Thune’s picture

Tor Arne Thune commented
Status: Reviewed & tested by the community » Fixed
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

We're backporting these. :)

lazysoundsystem’s picture

lazysoundsystem commented
Version: 8.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new remove_t_from_aggregator_asserts_D7-1741328-8.patch21.3 KB

And here's the backport.

lazysoundsystem’s picture

lazysoundsystem commented
Assigned: Unassigned » lazysoundsystem
Status: Needs review » Needs work

Hang on, found some ones I missed. I'll submit another patch.

lazysoundsystem’s picture

lazysoundsystem commented
Assigned: lazysoundsystem » Unassigned
Status: Needs work » Needs review
StatusFileSize
new remove_t_from_aggregator_asserts_D7-1741328-10.patch21.49 KB

This one is ready for review.

amontero’s picture

amontero
Location Barcelona
commented
Status: Needs review » Reviewed & tested by the community

Patch applies cleanly.
Grepped for remaining t()'s on assertions and found none left.
Tentatively setting it as RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » jhodgdon
Issue tags: +Coding standards

Tagging, and moving to Jennifer's plate.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

RE #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!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Assigned: jhodgdon » Unassigned
Status: Needs review » Fixed

OK, 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!

lazysoundsystem’s picture

lazysoundsystem commented

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

Status: Fixed » Closed (fixed)

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

droplet’s picture

droplet commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7
StatusFileSize
new remove_t.patch1.9 KB

During D8 translation, I found the t() used in tests.

** Let me know if it preferred to start a new issue for this fix. Thanks :)

ParisLiakos’s picture

ParisLiakos commented
Version: 7.x-dev » 8.x-dev
Status: Closed (fixed) » Reviewed & tested by the community
Issue tags: -Needs backport to D7

well, a new issue would be better, but since we have it..

lazysoundsystem’s picture

lazysoundsystem commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7 
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorConfigurationTest.php
@@ -48,7 +48,7 @@ function testSettingsPage() {
-    $this->assertText(t('Dummy length setting'));

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

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Closed (fixed)

Yeah, 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...