This is a sub-task of #500866: [META] remove t() from assert message focused on the P-S include files from the system module sub-systems (excluding system sub-system itself).

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 Pager, Path, Queue and Session sub-system tests for the above. There are approximately 84 changes needed spread out across 7 test files. This issue includes conversion of all format_patch() occurances as well.

CommentFileSizeAuthor
#9 1797914-9-P-S-assert-t.patch28.51 KBdcam
#4 t-assert-P-S-1797914-4.patch6.64 KBdcam
#1 t-assert-P-S-1797914-1.patch30.92 KBlars toomre

Comments

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new t-assert-P-S-1797914-1.patch30.92 KB

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

As in other of my patches for this initiative, this patch also includes some conversion of double quoted assert message text strings to single quoted strings to conform with Drupal's best practices.

dcam’s picture

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

#1 looks good. I didn't find any additional t()'s around assert messages.

jhodgdon’s picture

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

Amazingly, this patch still applies. Committed to 8.x.

I noticed when doing my final review that some of the $group parameters still have t() on them. Maybe we should follow-up patch to remove those? Thanks!

dcam’s picture

dcam commented
Status: Needs work » Needs review
StatusFileSize
new t-assert-P-S-1797914-4.patch6.64 KB

I removed the additional t()'s from around the group parameters where I found them.

dcam’s picture

dcam commented
Issue tags: +Novice

Tagging as Novice.

izus’s picture

izus commented

#4: t-assert-P-S-1797914-4.patch queued for re-testing.

izus’s picture

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

HI,
#4 looks good for me
Thanks

jhodgdon’s picture

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

Thanks! Committed #4 to 8.x. We now need to port a combination of #1/#4 to 7.x.

dcam’s picture

dcam commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1797914-9-P-S-assert-t.patch28.51 KB

Here's a backport of #1 and #4.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 1797914-9-P-S-assert-t.patch, failed testing.

dcam’s picture

dcam commented
Status: Needs work » Needs review

#9: 1797914-9-P-S-assert-t.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Novice

#9: 1797914-9-P-S-assert-t.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

bump! We only have a couple of these left. Can someone review this patch?

lazysoundsystem’s picture

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

This one applies cleanly, and passes all the tests.

jhodgdon’s picture

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

Thanks all! Committed to 7.x.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added change counts for this issue.