Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | database-1794012-10.patch | 104.12 KB | dcam |
#8 | database-1794012-8.patch | 104.09 KB | dcam |
#1 | Remove-t-1794012-1.patch | 114.8 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue.
Comment #2
Crell CreditAttribution: Crell commentedThis looks good on visual spot check, and the bot approves, so yay! Thanks, Lars.
Comment #3
webchickTagging with coding standards so Jennifer can get to this if she has time.
Comment #4
webchickMight as well make that explicit.
Comment #5
jhodgdonWow. 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?
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.
Comment #7
jhodgdonWe 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.
Comment #8
dcam CreditAttribution: dcam commentedBackported #1 to D7.
Comment #10
dcam CreditAttribution: dcam commentedI mistakenly deleted a couple of t() functions that should have been changed to format_string().
Comment #11
dcam CreditAttribution: dcam commented#10: database-1794012-10.patch queued for re-testing.
Comment #12
Crell CreditAttribution: Crell commentedI 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.
Comment #13
jhodgdonWhew, 345 lines changed! Thank goodness for diff --color-words... makes the scanning much more readable.
Thanks for the patch & review. Committed to 7.x.
Comment #14
dcam CreditAttribution: dcam commentedSweet. 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.
Comment #15
jhodgdonMany 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. :)
Comment #16
dcam CreditAttribution: dcam commentedOk, thanks. I'll check them and tag any that haven't been tagged already.
Comment #17.0
(not verified) CreditAttribution: commentedUpdated issue summary.