Remove t() from asserts for M includes system tests

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

Thanks!

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

Backported #13 to D7.

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

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

Tagging as Novice.

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

Hi,
#17 seems good :)
Thanks

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?

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.

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.

Made the change per #27-29.

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

#30 no longer applies.

Sorry. Ignore me and #32.

assigning to me for final review/commit

Thanks again everyone! Committed to 7.x.