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.
Part of meta #500866: [META] remove t() from assert message.
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff.txt | 688 bytes | dcam |
#24 | 1797200-24-t-node.patch | 60.56 KB | dcam |
#22 | 1797200-22-t-node.patch | 60.03 KB | dcam |
#19 | 1797200-19-t-node.patch | 60.03 KB | dcam |
#17 | 1797200-17-t-node.patch | 59.68 KB | dcam |
Comments
Comment #1
xjmComment #2
xjmJust the simple string messages. You can use
git diff --color-words
with the patch applied locally for easier review.Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedI have reviewed each of the straight t() removals from the assert messages in the #2 patch. All are correct and appropriate. I will leave checking for completeness to the follow-up patch correcting t() to format_string() conversions.
With the bot being green, this is RTBC!
Comment #4
webchickComment #5
jhodgdonCommitted the patch in #2 to 8.x. Leaving this issue open for follow-ups for t() -> format_string().
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an untested patch that completes the removal of t() from the node test assert messages or, if appropriate, the conversion of t() to format_string().
With this patch applied, I do not observe any more case of t() being used in node tests using grep. This patch includes the final 27 conversions in this module.
Comment #7
dcam CreditAttribution: dcam commentedI tested #6 and found two more t()'s around assert messages in NodeRevisionsTest.php on lines 87 and 99.
I could alter the patch myself, but then then we'll have to wait for someone else to test it. @Lars Toomre, if you can make the changes then I'll be sure to come back and re-test. Then we can get this committed.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a patch that includes the two items noted in #7.
Comment #9
dcam CreditAttribution: dcam commentedI've verified that #8 removes the two t()'s found in #7. I can't find any others that were missed. The patch looks good to me. Marking as RTBC.
Comment #10
webchickTum te tum...
Comment #11
jhodgdonThanks, that one's in! Looks like we need to port #2 + #8.
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @jhodgdon for the review and commit. It is so nice to see the node Tests now complete for D8. Thanks!!
Comment #13
dcam CreditAttribution: dcam commentedBackported #2 and #8 to D7.
Comment #15
dcam CreditAttribution: dcam commented#13: 1797200-13-t-node.patch queued for re-testing.
Comment #17
dcam CreditAttribution: dcam commentedI'm not sure why #13 wasn't applying, but I re-backported #2 and #8 to try and get a good patch this time.
Comment #19
dcam CreditAttribution: dcam commentedLet's try this again. I rolled this one on a different system. Maybe I did something to node.test on that other laptop that's causing #13 and #17 to not apply.
Backport of #2 and #8 to D7.
Comment #20
dcam CreditAttribution: dcam commented#19: 1797200-19-t-node.patch queued for re-testing.
Comment #22
dcam CreditAttribution: dcam commentedI rerolled the patch in #19.
Comment #24
dcam CreditAttribution: dcam commented#22 had the same problem described in #19. Sorry about that.
I rerolled #17 again and changed another unneeded t(). The post-reroll change is shown in interdiff.txt.
Comment #25
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #26
K.MacKenzie CreditAttribution: K.MacKenzie commentedComment #27
jhodgdonThanks! Assigning to me to commit unless someone else beats me to it.
Comment #28
jhodgdonThanks all! This one is committed to 7.x. Another one of these closed!