Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
26.75 KB

See the LanguageUrlRewritingTest::checkUrl() docs for that change. This patch includes multiple format_string() replacements.

Lars Toomre’s picture

I have gone through and reviewed all of the proposed message changes in this patch and they are accurate (including the suble last one!) With the test bot green, this is RTBC!

Regarding the last change, we really need to open up a follow-up issue to add @param directives to the method LanguageUrlRewritingTest::checkUrl(). At first glance, it is not at all intuitative that a test should have two messages as parameters two and three.

Thinking about this for a moment, I am going to open up a local branch to record some of these slightly off topic issues that we encounter in cleaning up t() assert messages. That then can be turned into an issue of its own afterwards.

Lars Toomre’s picture

Status: Needs review » Reviewed & tested by the community

I created #1797940: Fix docs for helper method in language tests for follow-ups to the remove t() from assert messages. The first patch there in #1797940-1: Fix docs for helper method in language tests addresses the missing documentation mentioned in #2.

This patch is RTBC!

Lars Toomre’s picture

I applied the patch in #1 and also went through looking at each assert message in the Tests for this module. I can confirm that no t() conversions were missed. Hence, this is RTBC!

webchick’s picture

Assigned: Unassigned » jhodgdon
jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed to 8.x.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
22.08 KB

Backported #1 to D7.

#1797364: Remove t() from assertion messages in tests for the locale module will also affect t() functions in Locale. Because of that, I only backported the changes from D8's language module tests. I didn't alter anything else.

izus’s picture

#7: 1797360-7-language-t.patch queued for re-testing.

izus’s picture

Status: Needs review » Reviewed & tested by the community

hi,
the patch seems good
Thanks

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again - committed to 7.x.

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