Posted by Lars Toomre on September 25, 2012 at 5:46am
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | coding standards |
Issue Summary
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.
Comments
#1
Here is an initial untested patch for this issue.
#2
This looks good on visual spot check, and the bot approves, so yay! Thanks, Lars.
#3
Tagging with coding standards so Jennifer can get to this if she has time.
#4
Might as well make that explicit.
#5
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?
#6
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.
#7
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.
#8
Backported #1 to D7.
#9
The last submitted patch, database-1794012-8.patch, failed testing.
#10
I mistakenly deleted a couple of t() functions that should have been changed to format_string().
#11
#10: database-1794012-10.patch queued for re-testing.
#12
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.
#13
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.
#14
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.
#15
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. :)
#16
Ok, thanks. I'll check them and tag any that haven't been tagged already.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.