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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
70.71 KB

Here is an initial untested patch for this issue. This patch includes format_string() conversions as well.

Status: Needs review » Needs work

The last submitted patch, t-assert-M-1797452-1.patch, failed testing.

Lars Toomre’s picture

Title: Remove t() from asserts for G-L includes system tests » Remove t() from asserts for M includes system tests
Status: Needs work » Needs review

Correcting the issue title...

Lars Toomre’s picture

Status: Needs review » Needs work
FileSize
70.74 KB

Ah, an extra '(' does not help. Here is another untested version of that patch.

Lars Toomre’s picture

Status: Needs work » Needs review

I am getting tired and making silly mistakes.... LOL

Status: Needs review » Needs work

The last submitted patch, t-assert-M-1797452-4.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
72 KB

Another darn typo... Here is the third roll of this initial untest patch (no local testing environment).

Status: Needs review » Needs work

The last submitted patch, t-assert-M-1797452-7.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
73.54 KB

Sorry for the noise... small mistakes from being tired...

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7
+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.phpundefined
@@ -515,7 +515,7 @@ class RouterTest extends WebTestBase {
-    $this->assertTitle($asserted_title . ' | Drupal', t('Menu title is') . ': ' . $asserted_title, 'Menu');
+    $this->assertTitle($asserted_title . ' | Drupal', 'Menu title is: ' . $asserted_title, 'Menu');

I 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.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
838 bytes
70.75 KB

Here is an update for that format_string() issue. An interdiff is also attached.

Status: Needs review » Needs work

The last submitted patch, t-assert-M-1797452-11.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
839 bytes
70.75 KB

Here is an updated patch for the missing comma. Also included is an interdiff.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

Assigned: Lars Toomre » jhodgdon
jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed to 8.x, ready for backport now.

dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
65.76 KB

Backported #13 to D7.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, assert-m-1797452-17.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review

#17: assert-m-1797452-17.patch queued for re-testing.

dcam’s picture

Issue tags: +Needs backport to D7

#17: assert-m-1797452-17.patch queued for re-testing.

dcam’s picture

Issue tags: +Novice

Tagging as Novice.

izus’s picture

#17: assert-m-1797452-17.patch queued for re-testing.

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
#17 seems good :)
Thanks

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

This one line from the patch doesn't look right to me:

-    return $this->assert($link1['mlid'] == $link2['mlid'], $message ? $message : t('First link is identical to second link'));
+    return $this->assert($link1['mlid'] == $link2['mlid'], $message ?: 'First link is identical to second link');

Should probably check to see if a similar change was made in 8.x too?

dcam’s picture

Status: Needs work » Reviewed & tested by the community

The 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.

jhodgdon’s picture

well maybe it should be removed from 8.x too? I'm not familiar with that syntax. Is it correct?

dcam’s picture

Oh, 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.

dcam’s picture

Status: Reviewed & tested by the community » Needs work

Actually, 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.

jhodgdon’s picture

OK. 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.

dcam’s picture

Status: Needs work » Needs review
FileSize
65.77 KB
658 bytes

Made the change per #27-29.

izus’s picture

Status: Needs review » Reviewed & tested by the community

Hi,
i reviewed #30 and it seems good for me.
Thanks

dcam’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#30 no longer applies.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Sorry. Ignore me and #32.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

assigning to me for final review/commit

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks again everyone! Committed to 7.x.

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