This is a sub-task of #500866: [META] remove t() from assert message focused on the M include files from the system module sub-systems.
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 Mail, Menu and Module sub-system tests for the above. There are approximately 155 changes needed spread out across 17 test files. This issue includes conversion of all format_patch() occurances as well.
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff.txt | 658 bytes | dcam |
#30 | 1797452-30-assert-m.patch | 65.77 KB | dcam |
#17 | assert-m-1797452-17.patch | 65.76 KB | dcam |
#13 | t-assert-M-1797452-13.patch | 70.75 KB | Lars Toomre |
#13 | interdiff-1797452-9-13.txt | 839 bytes | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an initial untested patch for this issue. This patch includes format_string() conversions as well.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedCorrecting the issue title...
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedAh, an extra '(' does not help. Here is another untested version of that patch.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedI am getting tired and making silly mistakes.... LOL
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedAnother darn typo... Here is the third roll of this initial untest patch (no local testing environment).
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedSorry for the noise... small mistakes from being tired...
Comment #10
xjmI think this one needs to be changed to a
format_string()
with placeholders instead of the concatenation.Everything else looked correct, so an interdiff will make it quicker to re-review.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an update for that format_string() issue. An interdiff is also attached.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedHere is an updated patch for the missing comma. Also included is an interdiff.
Comment #14
xjmThanks!
Comment #15
webchickComment #16
jhodgdonThanks! Committed to 8.x, ready for backport now.
Comment #17
dcam CreditAttribution: dcam commentedBackported #13 to D7.
Comment #19
dcam CreditAttribution: dcam commented#17: assert-m-1797452-17.patch queued for re-testing.
Comment #20
dcam CreditAttribution: dcam commented#17: assert-m-1797452-17.patch queued for re-testing.
Comment #21
dcam CreditAttribution: dcam commentedTagging as Novice.
Comment #22
izus CreditAttribution: izus commented#17: assert-m-1797452-17.patch queued for re-testing.
Comment #23
izus CreditAttribution: izus commentedHi,
#17 seems good :)
Thanks
Comment #24
jhodgdonThis one line from the patch doesn't look right to me:
Should probably check to see if a similar change was made in 8.x too?
Comment #25
dcam CreditAttribution: dcam commentedThe change was made in 8.x. You can see it on line 514 of #13 and in the 8.x code at line 66 of core/modules/system/lib/Drupal/system/Tests/TreeDataUnitTest.php.
Comment #26
jhodgdonwell maybe it should be removed from 8.x too? I'm not familiar with that syntax. Is it correct?
Comment #27
dcam CreditAttribution: dcam commentedOh, you're asking about the ternary operator syntax? Yes, it's correct. That's the shorthand way of writing it. From http://php.net/manual/en/language.operators.comparison.php:
"Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise."
A criticism of ternary operators is that they can make code hard to read. The shorthand can make it even a little more difficult if you don't understand what's going on, but it does make code a little easier to write since if you're just checking a variable like in this instance you don't have to type it out twice in a row. I don't know if there's a Drupal policy on its use.
Comment #28
dcam CreditAttribution: dcam commentedActually, since PHP 5.2.5 is listed as the official requirement for 7.x, I think it will need to be changed. I'm not sure what should be done about 8.x. The syntax should probably be changed back there as well so the versions match.
Comment #29
jhodgdonOK. Looks like it's pretty common in our code (I did a quick grep, eliminated vendor, and found about 300 uses in 8.x; some are regular expressions or JS, but many are PHP). Good to know about -- there's always something I don't know about in PHP. :)
But yes if it's 5.3+ we'd better not do it in 7.x. I grepped there and only found ?: in regular expressions in 7.x.
Comment #30
dcam CreditAttribution: dcam commentedMade the change per #27-29.
Comment #31
izus CreditAttribution: izus commentedHi,
i reviewed #30 and it seems good for me.
Thanks
Comment #32
dcam CreditAttribution: dcam commented#30 no longer applies.
Comment #33
dcam CreditAttribution: dcam commentedSorry. Ignore me and #32.
Comment #34
jhodgdonassigning to me for final review/commit
Comment #35
jhodgdonThanks again everyone! Committed to 7.x.