This is a sub-task of #500866: [META] remove t() from assert message focused on the Translation module.

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

There are approximately 24 changes needed in a 1 test files. This issue includes conversion of all format_patch() occurances as well.

CommentFileSizeAuthor
#10 1798376-10-t-translation.patch12.61 KBdcam
#6 1798376-3-t-translation.patch13.14 KBlars toomre
#6 interdiff-1798376-1-3.txt1.27 KBlars toomre
#1 1798376-1-t-translation.patch12.26 KBlars toomre

Comments

lars toomre’s picture

lars toomre commented
Title: Remove t() from test asserts in Translation mdoule » Remove t() from test asserts in Translation module
Status: Active » Needs review
StatusFileSize
new 1798376-1-t-translation.patch12.26 KB

Here is an initial untested patch for this issue. This patch includes format_string() conversions as well.

Status: Needs review » Needs work

The last submitted patch, 1798376-1-t-translation.patch, failed testing.

lars toomre’s picture

lars toomre commented

This error is caused by the problem noted in #1798382: Random Test failures in Entity translation tests.

lars toomre’s picture

lars toomre commented
Status: Needs work » Needs review

#1: 1798376-1-t-translation.patch queued for re-testing.

dcam’s picture

dcam commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

#1 no longer applies due to changes in TranslationTest.php.

lars toomre’s picture

lars toomre commented
Component: translation.module » documentation
Status: Needs work » Needs review
StatusFileSize
new interdiff-1798376-1-3.txt1.27 KB
new 1798376-3-t-translation.patch13.14 KB

Here is a locally untested re-roll of the patch from #1.

While doing the re-roll, I discovered two other uses of t() for assert messages that needed to be converted to format_string(). Hence, I made those conversions as well and am including an interdiff as well.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

I've tested #6 and the patch looks good to me. I didn't find any additional t()'s around assert messages.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Component: documentation » translation.module
Assigned: Unassigned » jhodgdon

fixing component, and assigning to me to do final review/commit.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! This is committed to 8.x and ready for backport (I assume?)

dcam’s picture

dcam commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1798376-10-t-translation.patch12.61 KB

Backported #6 to D7.

dcam’s picture

dcam commented

#10: 1798376-10-t-translation.patch queued for re-testing.

dcam’s picture

dcam commented
Issue tags: +Novice

Tagging as Novice.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

#10: 1798376-10-t-translation.patch queued for re-testing.

lazysoundsystem’s picture

lazysoundsystem commented
Status: Needs review » Reviewed & tested by the community

This one also looks good. RTBC.

jhodgdon’s picture

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

Thanks! Committed to 7.x.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated number of counts for this issue.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

I think this is not fully resolved yet. I still see the following lines using t(). Should i create new issue or re-open this issue?

./translation.test:46:    $this->assertRaw(t('The content type %type has been updated.', array('%type' => 'Basic page')), 'Basic page content type has been updated.');
./translation.test:57:    $this->assertRaw(t('Language negotiation configuration saved.'), 'URL language detection enabled.');
./translation.test:99:    $this->assertRaw(t('A translation of %title in %language already exists', array('%title' => $node_title, '%language' => $languages['es']->name)), 'Message regarding attempted duplicate translation is displayed.');
./translation.test:118:    $this->assertRaw(t('Basic page %title has been updated.', array('%title' => $node_title)), 'Original node updated.');
./translation.test:122:    $this->assertRaw('' . t('outdated') . '', 'Translation marked as outdated.');
./translation.test:129:    $this->assertRaw(t('Basic page %title has been updated.', array('%title' => $node_translation_title)), 'Translated node updated.');
./translation.test:152:    $this->assertRaw(t('Translations of %title', array('%title' => $node->title)), 'Translation overview page available with only one language enabled.');
./translation.test:300:        $this->assertRaw(t('The language %language has been created and can now be used. More information is available on the help screen.', array('%language' => $languages[$language_code]->name, '@locale-help' => url('admin/help/locale'))), 'Language has been created.');
./translation.test:311:      $this->assertRaw(t('Configuration saved.'), 'Language successfully enabled.');
./translation.test:337:    $this->assertRaw(t('Basic page %title has been created.', array('%title' => $title)), 'Basic page created.');
./translation.test:373:    $this->assertRaw(t('Basic page %title has been created.', array('%title' => $title)), 'Translation created.');
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Those t() statements are wrapping Drupal UI text, which still should be translated. This issue was about removing t() from test assertion messages, which should not be translated. For instance, in that last line, "Translation created" is the assertion message, and it is (correctly) not translated, while the assertRaw is looking for the text "Basic page (title) has been created" in the UI, so that needs to be translated.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

@jhodgdon, makes sense now. Thanks for the clarification.