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.

Files: 
CommentFileSizeAuthor
#30 interdiff.txt658 bytesdcam
#30 1797452-30-assert-m.patch65.77 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es).
[ View ]
#17 assert-m-1797452-17.patch65.76 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,379 pass(es).
[ View ]
#13 t-assert-M-1797452-13.patch70.75 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,895 pass(es).
[ View ]
#13 interdiff-1797452-9-13.txt839 bytesLars Toomre
#11 t-assert-M-1797452-11.patch70.75 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php.
[ View ]
#11 interdiff-1797452-9-11.txt838 bytesLars Toomre
#9 t-assert-M-1797452-9.patch73.54 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,695 pass(es).
[ View ]
#7 t-assert-M-1797452-7.patch72 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php.
[ View ]
#4 t-assert-M-1797452-4.patch70.74 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php.
[ View ]
#1 t-assert-M-1797452-1.patch70.71 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new70.71 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php.
[ View ]

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.

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

Correcting the issue title...

Status:Needs review» Needs work
StatusFileSize
new70.74 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php.
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new72 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new73.54 KB
PASSED: [[SimpleTest]]: [MySQL] 41,695 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new838 bytes
new70.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new839 bytes
new70.75 KB
PASSED: [[SimpleTest]]: [MySQL] 41,895 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thanks!

Assigned:Lars Toomre» jhodgdon

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new65.76 KB
PASSED: [[SimpleTest]]: [MySQL] 40,379 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

Issue tags:+needs backport to D7

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

Issue tags:+Novice

Tagging as Novice.

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

Status:Needs review» Reviewed & tested by the community

Hi,
#17 seems good :)
Thanks

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?

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.

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

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new65.77 KB
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es).
[ View ]
new658 bytes

Made the change per #27-29.

Status:Needs review» Reviewed & tested by the community

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

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

#30 no longer applies.

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

Sorry. Ignore me and #32.

Assigned:Unassigned» jhodgdon

assigning to me for final review/commit

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.