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

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 database sub-system tests for the above. There are approximately 350 changes needed.

CommentFileSizeAuthor
#10 database-1794012-10.patch104.12 KBdcam
#8 database-1794012-8.patch104.09 KBdcam
#1 Remove-t-1794012-1.patch114.8 KBlars toomre

Comments

lars toomre’s picture

lars toomre commented
Status: Active » Needs review
StatusFileSize
new Remove-t-1794012-1.patch114.8 KB

Here is an initial untested patch for this issue.

Crell’s picture

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

This looks good on visual spot check, and the bot approves, so yay! Thanks, Lars.

webchick’s picture

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

Tagging with coding standards so Jennifer can get to this if she has time.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: lars toomre » jhodgdon

Might as well make that explicit.

jhodgdon’s picture

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

Wow. Committed to 8.x. Should this be backported to 7.x too? And are there issues for the other tests, since this one just covers the database system tests?

lars toomre’s picture

lars toomre commented

Thanks @jhodgdon. I think this was my first core commit.

The main issue #500866: [META] remove t() from assert message is stalled. Correction for the comment modules needs to be rerolled in #1742602: Removing t() from asserts in simpletests in comment module. To my knowledge, there are no other active patches about removing t() from assert messages. You might want to check with @xjm though.

I can continue to roll patches like this one. However, as I asked in #500866-237: [META] remove t() from assert message, I would like some guidance on number of changes in one patch. On Monday, when locally fixing tests for Common, there were about 235 changes. Do you want that as one big group or as smaller sub-sets? Previously, @xjm was worried that large patches like this one might conflict with other active patches.

Let me know. Thanks.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

We now have a tag called "avoid commit conflicts" for issues
http://drupal.org/node/1207020#avoid-commit-conflicts
and my mandate is to avoid conflicts with those issues when I commit. Other than that, patches about the size of the one that was on this issue are fine, and you don't need to worry about the conflicts (I'll check when I do the commit). So far I haven't had any complaints about big commits messing up other patches since we introduced this tag.

Anyway, based on that base issue (which should be mentioned in this issue summary by the way!), I guess this needs a backport to D7.

dcam’s picture

dcam commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new database-1794012-8.patch104.09 KB

Backported #1 to D7.

Status: Needs review » Needs work

The last submitted patch, database-1794012-8.patch, failed testing.

dcam’s picture

dcam commented
Status: Needs work » Needs review
StatusFileSize
new database-1794012-10.patch104.12 KB

I mistakenly deleted a couple of t() functions that should have been changed to format_string().

dcam’s picture

dcam commented

#10: database-1794012-10.patch queued for re-testing.

Crell’s picture

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

I was in shock that this still applied, until I realized it's the backport patch so the code isn't really changing. :-) Bot's good with it, looks good on visual read through.

jhodgdon’s picture

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

Whew, 345 lines changed! Thank goodness for diff --color-words... makes the scanning much more readable.

Thanks for the patch & review. Committed to 7.x.

dcam’s picture

dcam commented

Sweet. We've finally gotten one of these committed for 7.x and closed out. Thanks for the review, Crell. =)

jhodgdon, do you think these issues are good candidates for being tagged as "Novice" issues so we can maybe get some more eyes on them? They seem easy to review, although maybe a bit tedious.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Many of them are already tagged Novice. Yes, definitely good novice material! We don't require Novice projects to be extremely quick, just straightforward and doable. :)

dcam’s picture

dcam commented

Ok, thanks. I'll check them and tag any that haven't been tagged already.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.