[META] remove t() from assert message

I completely agree...and have meant to start this issue for a while.

+1

Do we even want to remove it from button text.

I understand buttons are translated, but if we are assuming English then what is the point of translating the test. The only exception may be in the locale tests and such, but that makes sense.

Thoughts?

Right, so remove all meta data and strings not related to the code being tested.

For example, the following would be the new convention:

$this->assertTrue(TRUE, 'Text explaining the result');

$edit = array(...);
$this->drupalPost('some/url', $edit, t('Button text'));

public static function getInfo() {
  return array(
    'name' => 'Test name',
    'description' => 'Test description',
    'group' => 'Test group',
  );
}

Go for it. I thought we agreed on this ages ago?

Yes. Webchick and I had discussed this before a couple months ago. Using t() in tests only makes sense if you need the placeholder/checkplain replacement or you are looking for a specific button that could be translated, like the drupalPost() $submit parameter.

Working on this.

Need to see if there are any other invalid t() instances. Plus I want to see what bot thinks.

Used to following script.

foreach (file_scan_directory('modules', '/\.test$/') as $file) {
  $contents = file_get_contents($file->filepath);

  // Run twice to ensure that second parameters get picked up as well and
  // be safe with 4 run throughs.
  for ($i = 0; $i < 4; $i++) {
    $contents = preg_replace('/(assert\w+\(.*?,.*?)t\(([\'|"].*?[\'|"])\)/', '\1\2', $contents);
  }

  // Clean getInfo() methods.
  $contents = preg_replace('/(getInfo\(\).*?)t\((.*?)\)(.*?)t\((.*?)\)(.*?)t\((.*?)\)/s', '\1\2\3\4\5\6', $contents);

  file_put_contents($file->filepath, $contents);

  echo 'Cleaned: ' . $file->filepath . "\n";
}

following output:

Cleaned: modules/openid/openid.test
Cleaned: modules/tracker/tracker.test
Cleaned: modules/book/book.test
Cleaned: modules/blogapi/blogapi.test
Cleaned: modules/blog/blog.test
Cleaned: modules/syslog/syslog.test
Cleaned: modules/path/path.test
Cleaned: modules/node/node.test
Cleaned: modules/filter/filter.test
Cleaned: modules/system/system.test
Cleaned: modules/php/php.test
Cleaned: modules/menu/menu.test
Cleaned: modules/field/modules/field_sql_storage/field_sql_storage.test
Cleaned: modules/field/modules/text/text.test
Cleaned: modules/field/field.test
Cleaned: modules/locale/locale.test
Cleaned: modules/forum/forum.test
Cleaned: modules/statistics/statistics.test
Cleaned: modules/upload/upload.test
Cleaned: modules/simpletest/tests/form.test
Cleaned: modules/simpletest/tests/session.test
Cleaned: modules/simpletest/tests/registry.test
Cleaned: modules/simpletest/tests/module.test
Cleaned: modules/simpletest/tests/unicode.test
Cleaned: modules/simpletest/tests/theme.test
Cleaned: modules/simpletest/tests/bootstrap.test
Cleaned: modules/simpletest/tests/menu.test
Cleaned: modules/simpletest/tests/error.test
Cleaned: modules/simpletest/tests/filetransfer.test
Cleaned: modules/simpletest/tests/database_test.test
Cleaned: modules/simpletest/tests/file.test
Cleaned: modules/simpletest/tests/common.test
Cleaned: modules/simpletest/tests/xmlrpc.test
Cleaned: modules/simpletest/tests/schema.test
Cleaned: modules/simpletest/tests/image.test
Cleaned: modules/simpletest/tests/actions.test
Cleaned: modules/simpletest/tests/cache.test
Cleaned: modules/simpletest/tests/graph.test
Cleaned: modules/simpletest/tests/batch.test
Cleaned: modules/simpletest/simpletest.test
Cleaned: modules/dblog/dblog.test
Cleaned: modules/aggregator/aggregator.test
Cleaned: modules/user/user.test
Cleaned: modules/profile/profile.test
Cleaned: modules/contact/contact.test
Cleaned: modules/taxonomy/taxonomy.test
Cleaned: modules/search/search.test
Cleaned: modules/poll/poll.test
Cleaned: modules/trigger/trigger.test
Cleaned: modules/translation/translation.test
Cleaned: modules/help/help.test
Cleaned: modules/comment/comment.test
Cleaned: modules/block/block.test

This script fixes two issues:

foreach (file_scan_directory('modules', '/\.test$/') as $file) {
  $contents = file_get_contents($file->filepath);

  // Run twice to ensure that second parameters get picked up as well and
  // be safe with 4 run throughs.
  for ($i = 0; $i < 4; $i++) {
    $contents = preg_replace('/(assert\w+\(.*?,.*?) t\(([\'|"].*?[\'|"])\)/', '\1\2', $contents);
  }

  // Clean getInfo() methods.
  $contents = preg_replace('/(getInfo\(\).*?)t\(([\'|"].*?[\'|"])\)(.*?)t\(([\'|"].*?[\'|"])\)(.*?)t\(([\'|"].*?[\'|"])\)/s', '\1\2\3\4\5\6', $contents);

  file_put_contents($file->filepath, $contents);

  echo 'Cleaned: ' . $file->filepath . "\n";
}

Focus on getInfo() to get a good patch.

foreach (file_scan_directory('modules', '/\.test$/') as $file) {
  $contents = file_get_contents($file->filepath);

  // Run twice to ensure that second parameters get picked up as well and
  // be safe with 4 run throughs.
//  for ($i = 0; $i < 4; $i++) {
//    $contents = preg_replace('/(assert\w+\(.*?,.*? )t\(([\'|"].*?[\'|"])\)/', '\1\2', $contents);
//  }

  // Clean getInfo() methods, allow for some to be not t()'ed.
  $contents = preg_replace('/(getInfo\(\).*?name.*?)t\(([\'|"].*?[\'|"])\)(.*?})/s', '\1\2\3', $contents);
  $contents = preg_replace('/(getInfo\(\).*?description.*?)t\(([\'|"].*?[\'|"])\)(.*?})/s', '\1\2\3', $contents);
  $contents = preg_replace('/(getInfo\(\).*?group.*?)t\(([\'|"].*?[\'|"])\)(.*?})/s', '\1\2\3', $contents);

  file_put_contents($file->filepath, $contents);

  $last = exec('php -l ' . $file->filepath);
  if (substr($last, 0, 2) != 'No') {
    echo '  Clean failed: ' . $file->filepath . "\n";
  }
  else {
    echo 'Cleaned: ' . $file->filepath . "\n";
  }
}

Managed to upload old patch.

Patch applied cleanly and I couldn't find any instances of t() in core getInfo() functions.

oh man i'm so excited about this. finally a mega patch i can get behind.

This makes sense. I'm just pondering when is the most logical time to commit this.

I support this patch as well.

I would really like to see this get in since its ready to go, rather then rotting and having to be re-rolled repeatedly.

After this initial patch, the secondary chunk needs to be rolled so I think we should try and get this in.

The first patch creates minimal disturbance for patches since it only touches the getInfo() stuff which most patches aren't touching.

I'd say do it sooner than later. It's going to be a hassle either way so no sense having it looming over us for a few weeks.

I'm +1 on committing that right now. As far as I know, we don't have a lot of patches affecting tests right now.

I'll wait til I get confirmation that this will get committed when I post. This is the exact reason I wanted to get on with it...so I don't have to re-roll endlessly.

Talked with webchick in IRC, she said it would be alright.

Guess it should be bumped up as it was before.

Committed to HEAD. Docs need updating.

Also, this issue has a much larger change: cleaning up the assertions as described above. Not sure what the thoughts are on when to commit that.

It is much larger patch and I don't want to start rolling it until we are ready to commit. Seems like there is concern that it isn't worth committing it if it screws up patches and slows development. I am ok with postponing as long as it gets in before D7 release. Not sure if something like this can get in after freeze?

One advantage for sooner is when people start upgrading the standard will be set.

I will work on updating docs.

field.test got left out - probably because of the double spacing in front of the => which failed regexp replace ?
Being bold and setting to RTBC.

Looks good...fixed formatting error as well.

bleh, nevermind, field.test had been fixed in the committed patch, forget about #31.
Leaving to 'needs work', where it was before I started being a dumba**.

Hehe.

+

Sometimes I've used assert messages like t('Link check for %url is disabled.', array('%url' => $documentation_url)). How should we change this? Maybe a good idea to change to strtr('Link check for %url is disabled.', array('%url' => $documentation_url)) - not sure if this keeps the string out of the POTX eyes...

This has been postponed til after freeze.

Reverting status to active.

Additionally cross-linking #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

I will attempt this using the pgp parser shortly.

Based on drupal_web_test_case.php I have found the following assertion pattern:

Format: message

assertCacheExists
assertCacheRemoved


Format: message, group

pass
fail
error


Format: p, message, group

assert
assert*


Format: p1, p2, message, group

assertEqual
assertNotEqual
assertIdentical
assertNotIdentical
assertLink
assertLinkByHref
assertFieldByXPath
assertNoFieldByXPath
assertFieldByName
assertNoFieldByName
assertFieldById
assertNoFieldById
assertOptionSelected
assertNoOptionSelected
assertMail

assertPathMatch
assertNormalized
assertNoNormalized
assertSessionIds
assertSameLink
assertFilePermissions
assertDirectoryPermissions
assertFileHookCalled

Format: p1, p2, p3, p4, message, group

assertError


Ignore:

assertFieldSettings
assertNodeAccess
assertFailedLogin
assertTableCount
assertModules
assertLogMessage
assertFieldValues
assertCommentOrder
assertNothing
assertModuleList
assertCacheExists
assertSessionCookie
assertSessionEmpty
assertFileUnchanged
assertDifferentFile
assertSameFile
assertFileHooksCalled
assertTags
assertToolkitOperationsCalled
assertErrorMessage
assertNoErrorMessage
assertUrlOutboundAlter
assertUrlInboundAlter
assertPaths
assertReversePaths
assertComponents
assertWeights


Special:

assertAssertion

I have a coder_upgrade routine written and will be testing shortly. I will then review and provide patch.
development.module:

// $Id$

/**
 * Implements hook_upgrade_info().
 */
function development_upgrade_info() {
drupal_set_time_limit(0);
  $upgrade = array(
    'title' => t('SimpleTest remove t() from assert message, test getInfo(), etc (parser routines)'),
    'link' => 'http://drupal.org/node/500866',
  );
  return array('development' => $upgrade);
}

development.upgrade:

/**
 * Implements hook_upgrade_file_alter().
 *
 * Upgrades calls to class methods using grammar parser.
 *
 * @param PGPReader $reader
 *   The object containing the grammar statements of the file to convert.
 */
function development_upgrade_file_alter(&$reader) {
  $items = &$reader->getFunctionCalls();
  foreach ($items as &$item) {
    if (!isset($item) || !is_object($item) || !is_a($item, 'PGPFunctionCall') || $item->type != T_FUNCTION_CALL) {
      // The reference could have been changed in another routine so that it
      // no longer refers to an ojbect.
      continue;
    }

    if (is_a($item->name, 'PGPOperand') && $item->name->findNode('value') == '$this') {
      $name = substr($item->name->toString(), 7); // Strip off '$this->'.
      development_convert_asserts($item, $reader, $name);
    }
  }
}

/**
* Implements hook_upgrade_call_alter().
*/
function development_convert_asserts(&$item, &$reader, &$name) {
  $ignore = array(
    'assertFieldSettings',
    'assertNodeAccess',
    'assertFailedLogin',
    'assertTableCount',
    'assertModules',
    'assertLogMessage',
    'assertFieldValues',
    'assertCommentOrder',
    'assertNothing',
    'assertModuleList',
    'assertCacheExists',
    'assertSessionCookie',
    'assertSessionEmpty',
    'assertFileUnchanged',
    'assertDifferentFile',
    'assertSameFile',
    'assertFileHooksCalled',
    'assertTags',
    'assertToolkitOperationsCalled',
    'assertErrorMessage',
    'assertNoErrorMessage',
    'assertUrlOutboundAlter',
    'assertUrlInboundAlter',
    'assertPaths',
    'assertReversePaths',
    'assertComponents',
    'assertWeights',
  );

  $zero_single = array(
    'assertCacheExists',
    'assertCacheRemoved',
    'assertAssertion',
  );

  $zero_normal = array(
    'pass',
    'fail',
    'error',
  );

  // All assert* not otherwise accounted for: $one.

  $two = array(
    'assertEqual',
    'assertNotEqual',
    'assertIdentical',
    'assertNotIdentical',
    'assertLink',
    'assertLinkByHref',
    'assertFieldByXPath',
    'assertNoFieldByXPath',
    'assertFieldByName',
    'assertNoFieldByName',
    'assertFieldById',
    'assertNoFieldById',
    'assertOptionSelected',
    'assertNoOptionSelected',
    'assertMail',

    // None DrupalTestCase assertions.
    'assertPathMatch',
    'assertNormalized',
    'assertNoNormalized',
    'assertSessionIds',
    'assertSameLink',
    'assertFilePermissions',
    'assertDirectoryPermissions',
    'assertFileHookCalled',
  );

  $four = array(
    'assertError',
  );

  // Function name.
//  $name = $item->name['value'];

  // Halt processing if call is to be ignored.
  if (in_array($name, $ignore)) {
    return;
  }

  // Determine the assertion format.
  if (in_array($name, $zero_single)) {
    // Format: message.
    $params = array(0);
  }
  elseif (in_array($name, $zero_normal)) {
    // Format: message, group.
    $params = array(0, 1);
  }
  elseif (in_array($name, $two)) {
    // Format: p1, p2, message, group.
    $params = array(2, 3);
  }
  elseif (in_array($name, $four)) {
    // Format: p1, p2, p3, p4, message, group.
    $params = array(4, 5);
  }
  elseif (strpos($name, 'assert') === 0) {
    // Format: p1, message, group.
    $params = array(1, 2);
  }

  // Check if the function call is a recognized assertion.
  if (!empty($params)) {
    // Cycle through the parameter indexes to check, based on the assertion
    // format, and run the message extraction process.
    foreach ($params as $param) {
      development_extract_message($item, $param);
    }
  }
}

/**
* Check parameter at index for a wrapping t() function and remove if present.
*
* @param $item
*   PGP function call object.
* @param $index
*   Index of the parameter to check.
*/
function development_extract_message($item, $index) {
  if ($parameter = $item->getParameter($index)) {
    $operand = $parameter->getElement();
    if (is_a($operand, 'PGPFunctionCall') && is_array($operand->name) && $operand->name['value'] == 't') {
      if ($operand->parameters->count() > 1) {
        $operand->name['value'] = 'strtr';
      }
      else {
        $message = $operand->getParameter();
        $item->setParameter($index, $message);
      }
    }
  }
}

Attempting to find all the other "custom" assertions defined outside DWTC. Any that deviate from the default case will need to have special cases added to script.

grep -nR --exclude-dir=CVS "n assert" .
/modules/menu/menu.test:576:    // Change default parent item to Navigation menu, so we can assert more                                                                         
./modules/file/tests/file.test:144:  function assertFileExists($file, $message = NULL) {
./modules/file/tests/file.test:152:  function assertFileEntryExists($file, $message = NULL) {
./modules/file/tests/file.test:162:  function assertFileNotExists($file, $message = NULL) {
./modules/file/tests/file.test:170:  function assertFileEntryNotExists($file, $message) {
./modules/file/tests/file.test:179:  function assertFileIsPermanent($file, $message = NULL) {
./modules/file/tests/file.test:560:  function assertPathMatch($expected_path, $actual_path, $message) {
./modules/field_ui/field_ui.test:155:  function assertFieldSettings($bundle, $field_name, $string = 'dummy test string', $entity_type = 'node') {
./modules/node/node.test:777:  function assertNodeAccess($ops, $node, $account) {
./modules/filter/filter.test:1090:  function assertNormalized($haystack, $needle, $message = '', $group = 'Other') {
./modules/filter/filter.test:1114:  function assertNoNormalized($haystack, $needle, $message = '', $group = 'Other') {
./modules/user/user.test:297:  function assertFailedLogin($account, $flood_trigger = NULL) {
./modules/system/system.test:26:  function assertTableCount($base_table, $count = TRUE) {
./modules/system/system.test:43:  function assertModules(array $modules, $enabled) {
./modules/system/system.test:77:  function assertLogMessage($type, $message, $variables = array(), $severity = WATCHDOG_NOTICE, $link = '') {
./modules/dblog/dblog.test:545:  protected function assertLogMessage($log_message, $message) {
./modules/field/tests/field.test:60:  function assertFieldValues($entity, $field_name, $langcode, $expected_values, $column = 'value') {
./modules/comment/comment.test:757:  function assertCommentOrder(array $comments, array $expected_order) {
./modules/simpletest/simpletest.test:119:    // Call an assert function specific to that class.                                                                                  
./modules/simpletest/simpletest.test:131:  function assertNothing() {
./modules/simpletest/simpletest.test:177:   * Assert that an assertion with the specified values is displayed                                                                    
./modules/simpletest/simpletest.test:187:  function assertAssertion($message, $type, $status, $file, $function) {
./modules/simpletest/tests/module.test:78:  protected function assertModuleList(Array $expected_values, $condition) {
./modules/simpletest/tests/cache.test:43:  protected function assertCacheExists($message, $var = NULL, $cid = NULL, $bin = NULL) {
./modules/simpletest/tests/cache.test:67:  function assertCacheRemoved($message, $cid = NULL, $bin = NULL) {
./modules/simpletest/tests/session.test:204:  function assertSessionCookie($sent) {
./modules/simpletest/tests/session.test:216:  function assertSessionEmpty($empty) {
./modules/simpletest/tests/session.test:373:  protected function assertSessionIds($sid, $ssid, $assertion_text) {
./modules/simpletest/tests/menu.test:481:  protected function assertSameLink($link1, $link2, $message = '') {
./modules/simpletest/tests/file.test:60:  function assertFileUnchanged($before, $after) {
./modules/simpletest/tests/file.test:78:  function assertDifferentFile($file1, $file2) {
./modules/simpletest/tests/file.test:91:  function assertSameFile($file1, $file2) {
./modules/simpletest/tests/file.test:106:  function assertFilePermissions($filepath, $expected_mode, $message = NULL) {
./modules/simpletest/tests/file.test:128:  function assertDirectoryPermissions($directory, $expected_mode, $message = NULL) {
./modules/simpletest/tests/file.test:223:  function assertFileHooksCalled($expected) {
./modules/simpletest/tests/file.test:256:  function assertFileHookCalled($hook, $expected_count = 1, $message = NULL) {
./modules/simpletest/tests/batch.test:151:  function assertBatchMessages($texts, $message) {
./modules/simpletest/tests/common.test:480:  function assertTags($tags) {
./modules/simpletest/tests/common.test:1622:  function assertError($error, $group, $function, $file, $message = NULL) {
./modules/simpletest/tests/image.test:46:  function assertToolkitOperationsCalled(array $expected) {
./modules/simpletest/tests/error.test:107:  function assertErrorMessage(array $error) {
./modules/simpletest/tests/error.test:115:  function assertNoErrorMessage(array $error) {
./modules/simpletest/tests/path.test:222:  protected function assertUrlOutboundAlter($original, $final) {
./modules/simpletest/tests/path.test:241:  protected function assertUrlInboundAlter($original, $final) {
./modules/simpletest/tests/graph.test:112:  function assertPaths($graph, $expected_paths) {
./modules/simpletest/tests/graph.test:130:  function assertReversePaths($graph, $expected_reverse_paths) {
./modules/simpletest/tests/graph.test:147:  function assertComponents($graph, $expected_components) {
./modules/simpletest/tests/graph.test:168:  function assertWeights($graph, $expected_orders) {

Updated #42 based on grep in #43.

/me crosses fingers

Fixed what appears to be pgp issue with "instanceof".

PGP bugs fixed...lets see how this works.

I'll file issues for pgp bugs, but also core issue from this: #724788: Ugly code sytle, against guidelines in user.test.

PGP issues:
#724796: Block statements without braces whose body is a block statement
#724800: Expressions inside curly braces

and old issues for reference:
#722602: Issue with parethesis
#722596: Doesn't parse instanceof properly

PGP issues have been fixed.

3274 added, 3271 removed!

Only remaining annoyance is:

-    if ($contact !== TRUE) { // If true then attempting to find error message.
+    if ($contact !== TRUE) {
+ // If true then attempting to find error message.

Which isn't syntax obviously.

A lot of syntax fixes and poor spacing (like 3 spaces!?), dot issues, and failsauce like:

-      'description' => 'Perform various tests with cron trigger.' ,

Currently investigating some encoding issues with common.test.

Summed comment formating issues: #726054: Standardize/Correct comment formating, it seems to also add an extra trailing space with certain statements, but solotandem tells me he has that one fixed.

Otherwise this is great to see it fixing all the formating oddballs.

Trailing white space issue fixed, other two instances of bad comment formatting fixed, manually reviewed (scanned). Overally it looks good, and bot likes it.

Since trailing whitespace is removed by pgp in latest version it screws up an assertEqual().

Confirm that patch passes all tests localy

+++ modules/comment/comment.test	25 Feb 2010 23:10:50 -0000
@@ -768,7 +768,7 @@
-    return $this->assertIdentical($expected_cids, $result_order, t('Comment order: expected @expected, returned @returned.', array('@expected' => implode(',', $expected_cids), '@returned' => implode(',', $result_order))));
+    return $this->assertIdentical($expected_cids, $result_order, strtr('Comment order: expected @expected, returned @returned.', array('@expected' => implode(',', $expected_cids), '@returned' => implode(',', $result_order))));

@@ -828,7 +828,7 @@
-      $this->assertIdentical($expected_page, $returned_page, t('Flat mode, @new replies: expected page @expected, returned page @returned.', array('@new' => $new_replies, '@expected' => $expected_page, '@returned' => $returned_page)));
+      $this->assertIdentical($expected_page, $returned_page, strtr('Flat mode, @new replies: expected page @expected, returned page @returned.', array('@new' => $new_replies, '@expected' => $expected_page, '@returned' => $returned_page)));

So what about stuff like that? That's not the same.

Powered by Dreditor.

How not?

$new_replies = 'NEW!';
$expected_page = 'NEW!';
$returned_page = 'NEW!';
echo strtr('Flat mode, @new replies: expected page @expected, returned page @returned.', 
  array('@new' => $new_replies, '@expected' => $expected_page, '@returned' => $returned_page));

Prints: Flat mode, NEW! replies: expected page NEW!, returned page NEW!.

@boombatower t() is doing check_plain() for @ and drupal_placeholder for %, I think that you need some function to make it (st() parsed by potx too)

Having the italics in the output message is really of little to no importance in the simpletest assertion messages. What do you mean by the last bit, "st() parsed by potx too"? Is strtr parsed by potx?

I mean http://api.drupal.org/api/function/st/7
Strings passed to st() are extracted to be translated too!

Agree about placeholders, also I think about check_plain() - it's required because variables mostly randomName() so could lead to breaking output HTML

strtr() is just a PHP function, so it's not parsed by potx.

However, as a test author, that's really annoying to have to cludge together this weird strtr() string just to avoid calling t().

These should ideally be switched to:

$this->assertIdentical($expected_page, $returned_page, "Flat mode, $new_replies replies: expected page $expected_page, returned page $returned_page");

I'm not sure if that's possible with PHP Grammar Parser (which I refuse to call PGP because that's just an encryption type :P).

It's not only about breaking your layout. It's about outputting HTML.

$string = '<script type="text/javascript">alert("BAM!");</script>';
$this->assertEqual($expected, $returned, "Proper output: $string");

Two options:

a) Don't remove t() for messages using placeholders.

b) Introduce a t()-replacement helper function for testing. t() stands for "translate". s() for "string". Optionally make t() invoke s(). Might be helpful in a lot of other places.

Meh. Not crazy on "yet another silly one-or-two letter function name for dealing with strings", especially during API freeze, so b) is out for me.

a) makes sense, but then we create yet more inconsistency where "sometimes" you use t() around assertions and other times you don't.

....which makes me wonder why we bother doing all this in the first place. ;P If we just used t() everywhere, and adjusted our potx extractors to not parse .test files, we wouldn't have to teach our test writers/reviewers any special tricks.

If we just used t() everywhere, and adjusted our potx extractors to not parse .test files

I originally wanted to suggest this, too. But then I realized that a single test run will add 1,423,598,172 stupid strings to {locale_source} on your site. In turn, this would mean that you cannot run tests on any site where data matters. Bad.

I really don't see the need to for test to be dumping such data as HTML and what not. My instinct was to do "$var stuff" or $var . " stuff", but felt the strtr was a closer match. I can see about changing, but I want agreement on it. Personally I am fine with dropping strtr, but keeping t() around is just bad. In all tests I write (in contrib and what not) I no longer use t() and create string using normal appending process. Creating yet another oddness is just not a good idea.

Thinking about it more, I'm actually not sure #67 is that big of a deal, really. As a test author it's your problem to call check_plain(), check_markup(), etc. accordingly in places where you're outputting dangerous strings. Else we could try making the $message variable passed into assert() check_plain()ed and see how much stuff breaks.

In terms of what to replace strtr() with, "Blah dee blah $variable" seems the most natural to me.

Same way we have http://api.drupal.org/api/function/_dblog_format_message/7 which put a log of garbage into {locale_source}

I think it's a good idea to have some default implementation for printing function with escaping, so t() could use it and st() too.

"for $bar baz" is good for reading but fail in security!

Broken tags.

After reading the comments over again, I think we should stick to "Text $var" instead of something else. These are tests the messages are purely for debug purposes and shouldn't be outputting HTML or the like, and if they do? so what? these are developer tools and messages...if we can't trust the developer there isn't much we can prevent them from doing.

I'll revisit the script and see if I can get these merged into a string. I believe I started on it, but stopped due to comments. If I can get a confirmation then I'll go ahead and update the script and role a patch.

This was confirmed by webchick and Dries so I'll begin updating the script.

Need to figure out why script crashes on filter.test and error.test, but I went ahead and created a place where I can work on code. Aside from the crash the code does what is required.

http://github.com/boombatower/drupal-dev/tree/500866

I updated the code in the github repository and attached a new patch.

The upgrade routine was not dealing with 3 test files (error, form, and filter) that had calls to t() where the $args replacement parameter was a variable as opposed to an inline array. For example, from error.test:

t('Found %type in error page.', $error_pdo_exception)

All other calls to t() (to be changed by this patch) have an inline array of replacement values, like:

t("Found '!message' in error page.", array('!message' => $error_details))

To handle the first case, the upgrade routine searches for the most recent assignment to the replacement variable using the Grammar Parser API. From this statement object, the array keys can be extracted.

There are 2 cases of changes in the attached patch:
1) with an inline array of replacement values, each key in the t() message string is replaced by the corresponding value in the replacement array.
2) with a variable assigned elsewhere, each key in the t() message string is replaced by the corresponding key in the variable, e.g. $variable[$key]. To see an example of this, search for error.test in the patch file.

Change status.

Oops. CVS diff from inside the modules dir.

Curious.

hmm

solotandem updated script for API changes and we tweaked it to removed trailing or starting . '' or '' .

Arg, the patch is missing quite a few.

Previous patches involved silly mistakes....this one looks good.

Patch is obviously large, so not likely to go over in entirety with fine tooth comb. But a manual review of the 2 test cases indicates all looks well.

I also review the first 1000 lines and spot check a lot more.

Up-to-date version of the code used to generate patch can be found at http://github.com/boombatower/drupal-dev/tree/500866.

More accurate title...we can do followup for getInfo and what not if we choose to.

So now we need commiters to apply this patch and re-roll all patches that contains tests with t()

cross post....

also note simply apply them and run teh script.

Ok. This is something we agreed to do in Drupalcon SF. And although it's incredibly, stupidly late, committing it is necessary so that translators don't end up with oodles and oodles of useless extra strings to translate.

Committed to HEAD. Make sure this is reflected in the SimpleTest documentation, and the module upgrade guide (along with how devs can modify their own tests using the script you have here).

PLEASE REVERT.

This patch removed HTML encoding in *a lot* of places (ie. calls to check_plain()). t() automatically does that for you, reverting to string concatenation feels really wrong.

What we need here is a non-translating version of t(). Let's call it nt(). It does what t() does in terms of placeholder replacement and encoding, but (1) doesn't call locale() and (2) doesn't get extracted by potx.

This would have reduced the size and complexity of that patch by a ton, and is really useful for other stuff (like development shops, like us, that sometimes develop custom modules using non-English strings in code, don't want to lose placeholder replacement and want to keep track of strings that might be translated one day).

Agreed with Damien. And, hm. The more I've tried to not use t() in assertion messages in code recently, the more I was in the need for such a replacement t() function. However, the more I thought about a replacement t() function, the more often I asked myself whether that replacement t() function wouldn't be the same as t() for >99% of all cases, since t() was and still is designed to be very performant for the English-only use-case already. Additionally, proper usage of t() is very well known already.

Originally, I concluded above:

If we just used t() everywhere, and adjusted our potx extractors to not parse .test files

I originally wanted to suggest this, too. But then I realized that a single test run will add 1,423,598,172 stupid strings to {locale_source} on your site. In turn, this would mean that you cannot run tests on any site where data matters. Bad.

But if that - i.e., the addition of yet unknown locale source strings in the rare case of running tests on a production database of a Drupal site having multiple languages enabled -- if that is really the only issue with using t() in tests and making potx skip .test files, then I think we could surely tweak t() resp. locale() to never pollute {locale_source} when running tests. Similar to how we've tweaked other bare essential core functions elsewhere for testing.

Ok, at any rate, this still needs additional discussion. Reverted.

If we run tests on dev server how it's possible to sync strings with production?

making potx skip .test files

Also test modules and probably this helps #779204: Move simpletest/tests/*.test files into system/tests/*.test

never pollute {locale_source} when running tests

This applies only to DB of site that performs tests

It starts here. I'm sure changing the correct t() to nt() would not be hard with coder upgrade.

Yes, the scenario I'm describing is only possible by running DrupalUnitTestCases, or alternatively, within the context of something along the lines of #666956: Provide DrupalCloneTestCase for testing pre-configured drupal sites

That's the same as:

function nt($string, array $args = array(), array $options = array()) {
  $options['langcode'] = 'en';
  return t($string, $args, $options);
}

No?

No, that's not. We need to skip the whole custom string logic, because it doesn't apply here. Duplicating the (tiny) placeholder replacement logic feels better then bloating t() with new corner-cases.

at the risk of bikeshedding would p() for placeholder be better than nt() for not-translated? agree that we need another function for doing check_plain()ed replacement.

nt() or p(), I can buy either.

Not p(). Has no correlation to the existing t() function and we already use st() for a "special" t().

nt() makes sense to me since "not translated" is (almost) the opposite of "translate" (if you don't count verb tense)

#101: Also note that a much improved version of that patch (I'll comment on issue as well) is maintained in contrib SimpleTest 7.x-2.x and Examiner.com makes heavy use of it.

Another thought....why do we have HTML in assertion messages? Just as Damien argued in IRC we should be consistent with PHP and since assertion messages mirror (and in some cases are a shell for) exceptions/errors and PHP uses plain text messages we should as well. As such encoding HTML entities should be a non-issue, but if we really want to be sure...why not just throw a check_plain() in $this->assert()...I find the whole t('...', array(...)) very bulky compared to 'foo ' . $bar . 'sf' especially when these strings are simply debug information for tests.

If we really really have to go with a t() variant then my vote is on p()...simply because it seems arbitrary and one less character to type over and over, but obviously not a big deal in the end. Considering we never plan to translate these strings the use-case Damien mentioned in IRC about just switch nt|p() to t() if you want to end up translating a site and what not...really doesn't apply in our case so I'd much prefer raw string concatenation.

Either way I am quite annoyed that this all comes out after solotandem and myself wrote up the script (after receiving no further discussion and agreement) instead of before...big waste of time if we don't end up using it (which I'm also against as stated above).

k, seriously, screw this. we'll deal with it in D8.

Why don't we start by removing all the t() calls that don't do any string replacement?

I used the following commands to generate a patch:

git checkout 7.x
perl -pi -e "s|\\\$this->assert(.*), t\\((['\"])(.*)(['\"])\\)\\);|\\\$this->assert\\1, \\2\\3\\4);|" $(egrep -rl '\$this->assert.*, t\(.*\)\);' modules)
git diff --no-prefix > ~/Patches/d7/remove-unnecessary-t-from-assert.patch

The comment about an alternative t() function is just pie in the sky...this should have gone in as it was patched...but after so much crap and people who seemingly wanted to derail this I was exhausted as happens a lot of with core.

If we can move past that stuff...we can re-run the script and commit this.

Patch no longer applies. Re-rolled.

Trying again without the "--no-prefix" option.

Please just rerun the script or don't bother. No need for this issue to drag into multiple stages...lets do it or don't.

All tests passed this time. It appears that patches created with the "--no-prefix" option are now rejected by the testbot.

#88: 500866-remove-t.patch queued for re-testing.

Patch in #88 no longer applies. Could boombatower either re-run the script or explain to a dummy like me how to re-run it?

Install this module (http://github.com/boombatower/drupal-dev) on your d7 site, install coder_upgrade (usually best to use dev) from (http://drupal.org/project/coder) and then run an "upgrade" on the entire core with the routine defined in the "development.upgrade" file. From the .module file "SimpleTest remove t() from assert message, test getInfo(), etc (parser routines)" should be displayed in coder upgrade interface.

Somewhere along the line the Needs Documentation tag that was put on by webchick in #29 got lost.

The part that needs documentation is the removal of t() from the return values of getInfo(), which was committed in #29. Searching all the SimpleTest documentation on drupal.org reveals only one brief mention, in a tutorial, about what to put in getInfo(). Because this function is not inherited from DrupalWebTestCase or one of its parents, and is not defined by an interface, documentation of the function signature or return values doesn't show up on api.drupal.org (can't search for it there ...) or on drupal.org. I just fixed one handbook page that had the wrong signature and still used t() in its example, but an example of getInfo() is no substitute for proper documentation. I also submitted a patch to the examples module because some of the example tests were still using t().

After following boombatower's instructions, I consistently get the following error:

PHP Fatal error: Call to undefined method PGPOperand::getKey() in sites/all/modules/drupal-dev/development.upgrade on line 180

The getKey() method is defined for the PGPArray class, but not for the PGPList class from which the PGPOperand class is derived.

Apparently boombatower's module is not synchronized with the grammar_parser module.

(trying earlier versions of the grammar_parser codebase...)

Okay, if I checkout an earlier version of grammar_parser and then patch coder_upgrade.module to match, it runs for a slightly longer period of time before crash-and-burning.

Can boombatower please specify which commit number he is using from grammar_parser and coder projects?

I'll take a look and see if I can't update it, obviously it's been sometime :)

Let's try this patch.

The upgrade code (in github repository mentioned in #119, and last run in #88) needed to be tweaked for one test class, testTaxonomyUpgrade. If this passes, then I will update the repository code.

Same patch except in -p1 format.

This is definitely 8.x now. We'll just have to live with 7.x being in an inconsistent state. :\

The patch in #125 fails on one assertion. It would have passed except for a bug in the existing test code for testTaxonomyUpgrade. On line 159, the t() call wants a replacement for key = '%old_vid' and this key is not present in the $args array at that point in the code.

See #1097686: UpgradePathTaxonomyTestCase: replacement argument for "%old_vid" not set.

Didn't intend to change version (page loads with 7.x in post new comment settings).

#125: 500866-remove-t_5.patch queued for re-testing.

Since the offending line does not exist in 8.x, setting the issue version to 8.x should allow the test to pass.

Re-rolled against fresh 8.x checkout.

Fixed.

Trying again...

Found a bunch more in drupal_web_test_case.php

And again...

And again...

And again...

#142: simpletest_remove_t-500866-142.patch queued for re-testing.

#142: simpletest_remove_t-500866-142.patch queued for re-testing.

Re-rolled because of changes in core.

Apologies for the noise, but there is no way I can test this locally.

I must say I appreciate your persistence, once this passes I hope we can get it committed quickly.

Not likely, given the size of the queue. But it's easier to maintain than to start over from scratch.

EDIT: Just found a bunch more -- I had been searching for " t(" and not "(t(".

@pillarsdotnet, When you write, "Just found a bunch more -- I had been searching for ' t(' and not '(t('," does this mean you are editing/creating the patch manually? Or, are you using the automated routine described in #119 and other prior comments?

I'm editing manually. The automated routine is broken and boombatower hasn't had time to fix it yet.

(clickety...)

Only 28 more files to go.

I updated the code in https://github.com/boombatower/drupal-dev.git back in March when I determined what was causing the failure in the taxonomy test case. (But I failed to indicate so.) Give it a try and save yourself all the tedious manual labor. Let me know if you encounter any issues.

A new patch created with the automated routine.

Okay, I'll use that as a starting point, but it misses a bunch of 'em.

Here's a partial; still 27 files to edit.

Note: Time in this issue is much better spent editing the automator script so it catches what your grepping does. In this way, time wouldn't need to be spent constantly chasing 8.x, but could instead the script could just be run by core committers at a time when committing this is deemed least invasive to other things going on.

@pillarsdotnet, I believe you are mistaken as to the changes to be made by the patch. Based on a comparison of your patch in #163 and mine in #161, you are removing all calls to t() in the tests. That is not the intention. Continuing to do so will continue to guarantee test failures.

Please review #5 which shows some examples and is agreed to as the desired approach by several comments thereafter.

The patch in #161 is the one to commit and, unless someone can prove otherwise, is RTBC. This patch was prepared using the methodology described in the earlier comments on this issue.

@webchick, Please commit the patch from #161.

Committing this or not is the domain of Dries; this isn't a D7 patch. Please don't mark your own patches RTBC. :)

A new patch created with the automated routine, without changes to indention of '//' style comments.

Looks good.

Rather then continually postponing this or derailing this with pie in the sky stuff, I would appreciate it if we could get this through so we don't needlessly miss another release.

Agree - let's get this in (before it gets stale), and document the new standard, so we can point to it for contrib and new tests.

Also, I guess I should point out that no D8 clean-up patches / features are going to make it in at all, while the number of critical bugs exceeds 15 between D7 & D8. It's currently at 21.

Hopefully we can get some of those moved to RTBC soon.

Why shouldn't this be backported to D7?

We can all appreciate the desire to resolve critical bugs in a feature-stable code environment.

Yet, somehow a cleanup to tests would seem bifurcated from critical bugs dealing with core. In other words, what is the chance of bug in the test environment being flagged as critical? How can changes of this nature to the test environment (which is arguably distinct from the features provided by core) overlap with with and affect resolution of feature bugs?

So, again, why should committing this patch wait on critical feature bugs that have no bearing on it, nor will its commitment affect the resolution of those critical bugs (except possibly to remove a t() or two from any updated tests)?

Because this patch doesn't actually fix anything. It's a syntactical nicety that's adding consistency to .test files, and that's great, and totally on the table for 8.x (unfortunately we can't introduce it in 7.x anymore, because it would represent an API change of sorts, so we'll have to live with the inconsistency in D7).

But clean-up patches like this require re-rolls of other, more pressing issues in the queue, like major/critical bug fixes for D7 (because they must be applied to D8 first), which is actively being used in production right now. To prevent the problem we had in D7 where we got to code freeze and had 100+ critical issues to deal with and extended the release date of D7 by a year+, Dries imposed a rule regarding the number of critical bugs during his keynote at DrupaCon Chicago. If it gets > 15, then the only thing that gets committed are bug fixes until that number goes down. It's an incentive to get people to focus on the important stuff before the clean-up stuff/feature stuff, or at the very least not screw over people who are actively working on the important stuff.

See #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 for a large discussion on this issue, particularly most of catch's posts.

@#174 -- The proper issue to discuss such topics is #1050616: Figure out backport workflow from Drupal 8 to Drupal 7.

And most patches to core should be accompanied by changes to tests, although many of them don't.

We need to discuss:

-    $this->assertRaw(t('The category %title has been added.', array('%title' => $category_1['title'])), t('The category %title has been added.', array('%title' => $category_1['title'])));
+    $this->assertRaw(t('The category %title has been added.', array('%title' => $category_1['title'])), 'The category ' . $category_1['title'] . ' has been added.');

I'm not at all convinced that's a good change, could create confusion for developers.

However I don't see why the patch in general couldn't be applied to D7, it's not an API change, just applying a pattern where one no longer existed - does not enforce that contrib modules do this.

Two very closely related issues:

- #1191614: Make t() formatter available as its own function: would imply to replace t() with format_string() in test assertion messages.

- #1213510: A modern t(): might imply to merely append a method to t() calls in test assertion messages.

Switching 'needs change notification' to 'best practices change'.

I'm not totally up to speed on how Drupal calculates which strings should be translated for pot-file generation, and am only coming to this thread because I was just asked to change a core patch to remove t() from some assertion messages. I'm looking to see if this is an official best practice now or what.

Without an issue summary its hard to tell if the idea *is* going to be implemented, and if I really *should* remove t() from my message or not.

Also, I haven't read all 179 comments on this issue, so not sure if this idea has been suggested and discarded already, or if its just an uninformed idea, but... throwing caution to the wind... could it make sense to just exclude *.test from the list of file types to search for translation strings? This would allow us to ignore all strings in test files, including those instances where t() is still being used for placeholder replacement.

Tagging per #180.

If the test files themselves should be as t() free as possible, should not the primary API drupal_web_test_case.php also follow suit? The most recent patch doesn't remove simple strings from this file. Here are some examples:

Assertion message categories:

t('Browser')
t('E-mail')
t('Other')
t('Role')

Various simple assertion messages themselves:

t('Log out')
t('Email:')
t('Parsed page successfully.')
t('Verbose message')
t('The test did not complete due to a fatal error.')
t("The test cannot be executed because it has not been set up properly.")

Various t-based string substitutions. From what I can gather looking at the most recent patch, it looks like these need to be simplified as well. E.g.:

t('User created with name %name and pass %pass', array('%name' => $edit['name'], '%pass' => $edit['pass']))
t('Failed to set field @name to @value', array('@name' => $name, '@value' => $value)

What makes this uber-confusing is the possible need to leave *some* of the simple t() strings in place in the tests files. Consider the $submit argument of the DrupalTestCase::drupalPost() method, which according to the current API, should be wrapped in t(). Hoping someone more involved on the thread can shed some light here.

Can someone remind me why we want to do this again? Originally it was because we didn't want translators to have to translate these strings, which is absolutely true. However, if you do a search on localize.drupal.org for a string like 'Empty message correctly rejected during parsing' you'll see these strings already aren't coming up for translators. So this problem has already been worked around, even before Drupal 7's release.

Switching from t() to some other string mechanism means developers have to break their good habits of always wrapping user-facing input in t() in order to write tests, and have to learn a different way of handling dynamic strings like t('Created the @foo type', array('@foo' => $foo)) than they do in 99.9% of the rest of their code.

Tests already have enough barriers. Why do we want to add more?

Well, there's really no reason to add t() to the assertion message string. That's just needless overhead.

Output from test themes and modules is a different case and I'm on the fence about that. However, sun's point has been that it helps us test only the things being tested by decoupling what's being tested from all the other implicit stuff (translation in this case).

Edit: The example catch cites in #177 should probably use format_string() rather than a concatenation.

Edit: jhodgdon and I recently documented the current practice at http://drupal.org/simpletest-tutorial-drupal7#t.

It just 30k needless semi-expensive function calls.

We have format_string() now - looks like a perfect match for those assert messages.

Only drag is the func name feels too verbose - I'd rename to s(), personnally.
s() - don't translate,
t() - translate,
easy :-)

Thanks. "Performance" seems like a reasonable reason, as does using format_string(). The issue summary could really use an update.

I'll update the handbook page to remove the bit about load for translators, since you've indicated that's already fixed.

+1 for renaming format_string to s() in D8. :-)

While renaming format_string() to s() seems a good idea to me, we'd need to move on with this either way. Replacing 'format_string' by 's' will be a quick and straight forward one anyway so we can discuss that one in parallel since that is a different issue to this one.

But in the meanwhile tons of tests are being written using lots of t() for Drupal 8 and this patch would become bigger and bigger.
On top of that, has anyone noticed that Drupal 8 unit tests are failing if you run them locally from a site with Locale module enabled? (yes, because of translating assert messages).

xjm suggested limiting the scope to plain strings only, with a follow up for complex strings: #1681890: Use format_string() instead of t() for assertion message text

xjm also asked me to post the bash script I wrote because it was faster than trying to figure out how to fix the one above to only find the plain strings:

for file in `grep -rl 'assert[A-Za-z]*(.* t([^)]*));$' core/modules/`
do
  # No need to check, we only have files fitting the criteria from grep.
  mv $file $file.tmp
  sed  's#\(assert[A-Za-z]*(.* \)t(\([^)]*)\));#\1\2;#g' $file.tmp > $file
  rm $file.tmp
  echo Fixed $file
done

I know it's tricksy with the end paren bits, but it IS just a one time script I wasn't planning to share. :)

I found the getInfo() bits with: grep -rA4 "function getInfo(." Projects/core/modules/*/lib/Drupal/*/Tests/ | grep "'name' => t("
since there were only 6, I just manually fixed 'em.

Let's see what the bot has to say...

Great start, indeed :)
I noticed that the space before t() was also removed, so that there is no space between parameters.
Example:
$this->assertRaw($base_theme_project_link,'Link to the Update test base theme project appears.');

Those failures are from incorrectly removing t() when it should be switched to format_string().

Which makes me think, would it be easier to do #1681890: Use format_string() instead of t() for assertion message text first?

Solotandem is in irc and can be asked for details on changing the script. Better then chasing all the special cases and formatting issues from the shell script since reg ex is not suited to this task.

@Tor Arne Thune and @timplunkett You're absolutely right, I thought I had corrected for those. Let me be sure I got the right patch and if not fix it.

@boomatower I don't understand how regex via preg_replace w/ PCRE limitations is better than regex via bash/sed. The point here was we don't want all the special cases for this patch. Only straight ahead t() on the last parameter of an assertion. I think the php script will still be useful for #1681890: Use format_string() instead of t() for assertion message text.

The script used by solotandem and I is not a bunch of regex...it uses a parser to parse/change/writeout code. Use whatever you like (just suggested the parser since you seem to be having issues with regex hitting the case correctly) as those of us who have worked on this are long burned out from repeated shedding.

I've fixed the script.

However, unless this blocks something, then I'd highly recommend to defer the commit until after feature freeze. Committing this patch will cause quite some hazard.

These are what I was taking about in #195, they should be format_string.

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterCrudTest.phpundefined
@@ -88,17 +88,17 @@ class FilterCrudTest extends WebTestBase {
-    $this->assertEqual($db_format->format, $format->format, t('Database: Proper format id for text format %format.', $t_args));
-    $this->assertEqual($db_format->name, $format->name, t('Database: Proper title for text format %format.', $t_args));
-    $this->assertEqual($db_format->cache, $format->cache, t('Database: Proper cache indicator for text format %format.', $t_args));
-    $this->assertEqual($db_format->weight, $format->weight, t('Database: Proper weight for text format %format.', $t_args));
+    $this->assertEqual($db_format->format, $format->format, 'Database: Proper format id for text format %format.', $t_args);
+    $this->assertEqual($db_format->name, $format->name, 'Database: Proper title for text format %format.', $t_args);
+    $this->assertEqual($db_format->cache, $format->cache, 'Database: Proper cache indicator for text format %format.', $t_args);
+    $this->assertEqual($db_format->weight, $format->weight, 'Database: Proper weight for text format %format.', $t_args);
 
     // Verify filter_format_load().
     $filter_format = filter_format_load($format->format);
-    $this->assertEqual($filter_format->format, $format->format, t('filter_format_load: Proper format id for text format %format.', $t_args));
-    $this->assertEqual($filter_format->name, $format->name, t('filter_format_load: Proper title for text format %format.', $t_args));
-    $this->assertEqual($filter_format->cache, $format->cache, t('filter_format_load: Proper cache indicator for text format %format.', $t_args));
-    $this->assertEqual($filter_format->weight, $format->weight, t('filter_format_load: Proper weight for text format %format.', $t_args));
+    $this->assertEqual($filter_format->format, $format->format, 'filter_format_load: Proper format id for text format %format.', $t_args);
+    $this->assertEqual($filter_format->name, $format->name, 'filter_format_load: Proper title for text format %format.', $t_args);
+    $this->assertEqual($filter_format->cache, $format->cache, 'filter_format_load: Proper cache indicator for text format %format.', $t_args);
+    $this->assertEqual($filter_format->weight, $format->weight, 'filter_format_load: Proper weight for text format %format.', $t_args);

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCommentLanguageTest.phpundefined
@@ -101,7 +101,7 @@ class LocaleCommentLanguageTest extends WebTestBase {
-        $this->assertEqual($comment->langcode, $langcode, t('The comment posted with content language %langcode and belonging to the node with language %node_language has language %comment_language', $args));
+        $this->assertEqual($comment->langcode, $langcode, 'The comment posted with content language %langcode and belonging to the node with language %node_language has language %comment_language', $args);

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.phpundefined
@@ -89,12 +89,12 @@ class LocaleJavascriptTranslation extends WebTestBase {
-      $this->assertTrue(isset($source_strings[$str]), t("Found source string: %source", $args));
+      $this->assertTrue(isset($source_strings[$str]), "Found source string: %source", $args);
 
       // Make sure that the proper context was matched.
-      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : t("Context for %source is blank", $args));
+      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : "Context for %source is blank", $args);

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/ProgrammaticTest.phpundefined
@@ -84,7 +84,7 @@ class ProgrammaticTest extends WebTestBase {
-    $this->assertTrue($valid_input == $valid_form, t('Input values: %values<br/>Validation handler errors: %errors', $args));
+    $this->assertTrue($valid_input == $valid_form, 'Input values: %values<br/>Validation handler errors: %errors', $args);

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.phpundefined
@@ -93,15 +93,15 @@ class ErrorHandlerTest extends WebTestBase {
-    $this->assertText($error_pdo_exception['%type'], t('Found %type in error page.', $error_pdo_exception));
-    $this->assertText($error_pdo_exception['!message'], t('Found !message in error page.', $error_pdo_exception));
+    $this->assertText($error_pdo_exception['%type'], 'Found %type in error page.', $error_pdo_exception);
+    $this->assertText($error_pdo_exception['!message'], 'Found !message in error page.', $error_pdo_exception);

This one fixes Tim's too...

The new script:

for file in `grep -rl "assert[A-Za-z]*(.* t('[^')]*'));" core/modules/`
do
  mv $file $file.tmp
  sed  "s#\(assert[A-Za-z]*(.*\) t(\('[^')]*'\)));#\1 \2\);#g" $file.tmp > $file
  rm $file.tmp
  echo Fixed $file
done

Yes and no. They should be left out entirely for this patch.

Adjusted the script to skip all t()s that should be format_string(). Lame algorithm, but at least no false-positives. ;)

Thanks @sun and @zendoodles!

+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.phpundefined
@@ -164,7 +164,7 @@ class UpdateCoreTest extends UpdateTestBase {
-    $this->assertNoRaw('<li>' . t('There is a security update available for your version of Drupal.'));
+    $this->assertNoRaw('<li>' . 'There is a security update available for your version of Drupal.');

Reading up from the bottom. Got to here. This change is not correct. (The message parameter is being omitted, so this is the first argument. t() should not be stripped.

Let's give sun's script a shot, compare the diffstats, interdiff it up, and make sure we don't change these.

Okay... now we also make sure it's not the first parameter. I *think* this actually found another problem when there are two spaces before the t() instead of one though, so I may follow-up with yet another.

Fix the cases where t() is preceded by two spaces. interdiff.txt is to #201

Update summary with current status and detailed information for maintainers.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.