Remove !placeholder usage from SafeMarkup::format()

A bunch of these are covered off in #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests so added it as related.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -718,11 +718,11 @@ protected function validate() {
   -          $this->logError($this->t('Entity type mismatch on rename. !old_type not equal to !new_type for existing configuration !old_name and staged configuration !new_name.', array('old_type' => $old_entity_type_id, 'new_type' => $new_entity_type_id, 'old_name' => $names['old_name'], 'new_name' => $names['new_name'])));
   +          $this->logError($this->t('Entity type mismatch on rename. @old_type not equal to @new_type for existing configuration @old_name and staged configuration @new_name.', array('@old_type' => $old_entity_type_id, '@new_type' => $new_entity_type_id, '@old_name' => $names['old_name'], '@new_name' => $names['new_name'])));
   ...
   -          $this->logError($this->t('Rename operation for simple configuration. Existing configuration !old_name and staged configuration !new_name.', array('old_name' => $names['old_name'], 'new_name' => $names['new_name'])));
   +          $this->logError($this->t('Rename operation for simple configuration. Existing configuration @old_name and staged configuration @new_name.', array('@old_name' => $names['old_name'], '@new_name' => $names['new_name'])));
   

   This looks like a bug you mentioned, nice catch! Although does this doesn't seem to have to do with SafeMarkup::format(). If we are to resolve that here should we add tests here or split this off and add tests in a follow-up?

2. +++ b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
   @@ -22,7 +22,7 @@ class CacheTestController {
   -      '#markup' => SafeMarkup::format('This URL is early-rendered: !url. Yet, its bubbleable metadata should be bubbled.', ['!url' => $url->toString()])
   +      '#markup' => "This URL is early-rendered: {$url->toString()}. Yet, its bubbleable metadata should be bubbled."
   

   Nit: https://www.drupal.org/coding-standards#concat
   and missing comma at end.

   When you concatenate simple variables, you can use double quotes and add the variable inside; otherwise, use single quotes.

Regarding #3 totally agree. Made a follow-up #2565123: Move logic from SafeMarkup::format() from template_preprocess_locale_translation_update_info() to template

I took out the config fix for now and fixed a couple of nits. I reviewed this with color-words and without and I think this is RTBC.

As you can see from the interdiff other than the nits and the removal of unrelated fixes this is the same patch.
I ran config test locally and they still passed so I'm pretty confident that change is not needed here and we can open up a follow-up (I will after this comment.) If you or testbot disagrees with my assessment we can NW this, but we likely need to "Needs tests" if we keep that bug fix in.

And of course I missed a space and a . in the sentence changed. Of course.

Follow-up added #2565139: Fix config importer error log.

Realized the tests are fixed as to why the follow-up I created was failing:) #2565139: Fix config importer error log.

Thanks for re-applying the nits @alexpott

Commenting per #10.

Also noting that disruption to translated strings should be minimal. I count a max of 3 instances of translated strings per language.

Comment per #10.

Thanks everyone! Hopefully the others from the other issue will comment here as well. Meanwhile: I reviewed all the changes in the patch and they all seem correct... that said, I also have a couple questions about the mixed approaches taken.

Ideally, we would split these patches up based on the type of the fix (e.g.: remove all translation/formatting with exception messages, remove undeeded assertion mesages, etc.), not as a hodgepodge of different bugs. The issue title says "from SafeMarkup::format()" so that would suggest to me that it was removing all the cases in core where it was used with format() directly (as opposed to t())... but the first hunk in the patch plus one other are t(). Do any !placeholder uses remain with SafeMarkup::format() directly? Why is the first hunk included? Because they were in the configuration system, I guess? I'm not pushing back on committing them together since (as stated) all seem okay, but it does make the patch harder to review.

Specific questions/remarks:

1. So it looks like most of the changes in this patch are "It never needed to be ! in the first place, so just use @." I reviewed all of those changes and confirmed that in each case the placeholder is used simply in text (not inside an HTML tag or anything). Those are all fine.

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -28,13 +27,11 @@ public function parse($filename) {
   -        $message = SafeMarkup::format("Unable to parse !file: !error", array('!file' => $filename, '!error' => $e->getMessage()));
   -        throw new InfoParserException($message);
   +        throw new InfoParserException("Unable to parse $filename " . $e->getMessage());
   ...
   -        $message = SafeMarkup::format('Missing required keys (!missing_keys) in !file.', array('!missing_keys' => implode(', ', $missing_keys), '!file' => $filename));
   -        throw new InfoParserException($message);
   +        throw new InfoParserException('Missing required keys (' . implode(', ', $missing_keys) . ') in ' . $filename);
   

   These fall under the category of "Don't sanitize exception messages because this unnecessarily couples the code and also the Error class sanitizes the output if you have error reporting to screen turned on, which you shouldn't in production anyway." Is that a correct summary? And is that documented somewhere as a best practice?

3. +++ b/core/modules/config/src/Tests/SchemaCheckTestTrait.php
   @@ -33,19 +32,19 @@ public function assertConfigSchema(TypedConfigManagerInterface $typed_config, $c
   -      $this->fail(SafeMarkup::format('No schema for !config_name', array('!config_name' => $config_name)));
   +      $this->fail("No schema for $config_name.");
   ...
   -      $this->pass(SafeMarkup::format('Schema found for !config_name and values comply with schema.', array('!config_name' => $config_name)));
   +      $this->pass("Schema found for $config_name.");
   ...
   -        $this->fail(SafeMarkup::format('Schema key @key failed with: @error', array('@key' => $key, '@error' => $error)));
   +        $this->fail("Schema key $key failed with: $error.");
   
   +++ b/core/modules/simpletest/src/TestBase.php
   @@ -754,10 +754,7 @@ protected function assertNotIdentical($first, $second, $message = '', $group = '
   -    $message = $message ?: SafeMarkup::format('!object1 is identical to !object2', array(
   -      '!object1' => var_export($object1, TRUE),
   -      '!object2' => var_export($object2, TRUE),
   -    ));
   +    $message = $message ?: var_export($object1, TRUE) . ' is identical to ' . var_export($object2, TRUE);
   

   These changes are: "There's no point in sanitizing assertion messages, and the SimpleTest results page is XSS filtered at the end anyway even if you are running it in production, which you shouldn't." Right? And have we documented that best practice yet as well?

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2718,7 +2718,7 @@ protected function assertUrl($path, array $options = array(), $message = '', $gr
   -    return $this->assertTrue($match, $message ? $message : SafeMarkup::format('HTTP response expected !code, actual !curl_code', array('!code' => $code, '!curl_code' => $curl_code)), $group);
   +    return $this->assertTrue($match, $message ? $message : SafeMarkup::format('HTTP response expected @code, actual @curl_code', array('@code' => $code, '@curl_code' => $curl_code)), $group);
   
   @@ -2744,7 +2744,7 @@ protected function assertResponse($code, $message = '', $group = 'Browser') {
   -    return $this->assertFalse($match, $message ? $message : SafeMarkup::format('HTTP response not expected !code, actual !curl_code', array('!code' => $code, '!curl_code' => $curl_code)), $group);
   +    return $this->assertFalse($match, $message ? $message : "HTTP response not expected $code, actual $curl_code", $group);
   

   The second one here is another "Don't needlessly sanitize assertion messages." But why is it different from the first one then, which is just replacing ! with @?

5. +++ b/core/modules/system/src/Tests/Module/UninstallTest.php
   @@ -72,7 +72,7 @@ function testUninstallPage() {
   -    $this->assertText($node_type->label(), SafeMarkup::format('The entity label "!label" found.', array('!label' => $node_type->label())));
   +    $this->assertText($node_type->label());
   
   @@ -88,7 +88,7 @@ function testUninstallPage() {
   -      $this->assertText($label, SafeMarkup::format('The entity label "!label" found.', array('!label' => $label)));
   +      $this->assertText($label);
   

   These are "Useless assertion messages for assertText() make debugging harder, so might as well just remove them." That's something we tell contributors all the time, though come to think of it, maybe a followup novice issue to explicitly say that on the methods wouldn't go amiss.

6. +++ b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
   @@ -22,7 +22,7 @@ class CacheTestController {
   -      '#markup' => SafeMarkup::format('This URL is early-rendered: !url. Yet, its bubbleable metadata should be bubbled.', ['!url' => $url->toString()])
   +      '#markup' => 'This URL is early-rendered: ' . $url->toString() . '. Yet, its bubbleable metadata should be bubbled.',
   

   This one is test code, so there's no need to sanitize it there either. #markup will successfully XSS filter it anyway, which is certainly sufficient for the test code.

Oh also, I agree with #2 / #3 about the fishy (pun!) use of SafeMarkup::format() in locale, and also agree that it can be considered out of scope here. But also I'm not 100% clear on what is in scope. ;)

The line from #18.2 that makes this clear (I've reformatted the text to make it a bit more clear).
https://www.drupal.org/node/608166

The Exception's message should include a hint to the values that caused the exception.
Formatting messages should be done by concatenating strings or using sprintf().
Values should be surrounded by single quotes.
DO NOT translate the message.
DO NOT use SafeMarkup::format().

Thank you for the reference.

I've updated the issue summary to clarify the proposal.

After the scope clarification I found a missing reference that wasn't resolve in this issue:
core/lib/Drupal/Core/Installer/Form/SelectLanguageForm.php:73

SafeMarkup::format('<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>. If you do not want this, select <a href="!english">English</a>

1. Looks like #16.3 was changed to simple replace ! with @.
2. #16.5 needs a follow-up novice issue.
3. I've tried to add #16.6 to the issue summary but it may open up the scope a bit much for this issue.

This looks really good still, I only spotted one unreplaced !placeholder and the only other was the test for SafeMarkup.

Comment as per #10

Also, my question about why there are t() included in this patch is still outstanding as far as I can tell. It does not seem to be addressed in #18.

Also:

Yep will create. Personally I think this should just be fixed with always including the default message even if a custom message is used - ala PHPUnit

I tried to do that 3 years ago and it got blocked, mired, gnarled, stuck, and sidetracked. Getting in a simple novice docs patch that tells novices that omitting the optional parameter is preferred is a much easier change. We might as well tell devs in the codebase what we tell them in reviews.

Regarding point #16.5 actually I think this is already covered by:

   * @param $message
   *   (optional) A message to display with the assertion. Do not translate
   *   messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed
   *   variables in the message text, not t(). If left blank, a default message
   *   will be displayed.

No, I don't think that covers it. The followup I'm suggesting for assertText() etc. would explicitly tell developers "In most cases you should omit this parameter" or something. But anyway, none of that is part of the scope of this. :)

Re #25

Oh I didn't explicitly say why t() was included in the patch above:

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -718,11 +718,11 @@ protected function validate() {
-          $this->logError($this->t('Entity type mismatch on rename. !old_type not equal to !new_type for existing configuration !old_name and staged configuration !new_name.', array('old_type' => $old_entity_type_id, 'new_type' => $new_entity_type_id, 'old_name' => $names['old_name'], 'new_name' => $names['new_name'])));
+          $this->logError($this->t('Entity type mismatch on rename. @old_type not equal to @new_type for existing configuration @old_name and staged configuration @new_name.', array('@old_type' => $old_entity_type_id, '@new_type' => $new_entity_type_id, '@old_name' => $names['old_name'], '@new_name' => $names['new_name'])));
...
-          $this->logError($this->t('Rename operation for simple configuration. Existing configuration !old_name and staged configuration !new_name.', array('old_name' => $names['old_name'], 'new_name' => $names['new_name'])));
+          $this->logError($this->t('Rename operation for simple configuration. Existing configuration @old_name and staged configuration @new_name.', array('@old_name' => $names['old_name'], '@new_name' => $names['new_name'])));

+++ b/core/modules/config/src/Tests/ConfigImportRenameValidationTest.php
@@ -110,7 +110,7 @@ public function testRenameValidation() {
-        SafeMarkup::format('Entity type mismatch on rename. !old_type not equal to !new_type for existing configuration !old_name and staged configuration !new_name.', array('old_type' => 'node_type', 'new_type' => 'config_test', 'old_name' => 'node.type.' . $content_type->id(), 'new_name' => 'config_test.dynamic.' . $test_entity_id))
+        SafeMarkup::format('Entity type mismatch on rename. @old_type not equal to @new_type for existing configuration @old_name and staged configuration @new_name.', array('@old_type' => 'node_type', '@new_type' => 'config_test', '@old_name' => 'node.type.' . $content_type->id(), '@new_name' => 'config_test.dynamic.' . $test_entity_id))

@@ -153,7 +153,7 @@ public function testRenameSimpleConfigValidation() {
-        SafeMarkup::format('Rename operation for simple configuration. Existing configuration !old_name and staged configuration !new_name.', array('old_name' => 'config_test.old', 'new_name' => 'config_test.new'))
+        SafeMarkup::format('Rename operation for simple configuration. Existing configuration @old_name and staged configuration @new_name.', array('@old_name' => 'config_test.old', '@new_name' => 'config_test.new'))

We've changed the tests as per fixing the !old_name and !new_type keys in SafeMarkup::format(). And those failed because they needed the same input from the t() they were comparing against. I only kinda mentioned that in passing on #13, sorry should have been more explicit with my note.

I recant my needs work for #22. @stefan_r mentioned it was part of #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests because it's a !url pattern, so can be ignored for this issue.

Created a follow-up for #22 as it really should be a t() anyways.

#2565981: Remove $this->t() and SafeMarkup::format() in SelectLanguageForm

Still RTBC from my stance, did a --color-words to review the changes were more ! to @ and less removal of unnecessary (scope creepy) SafeMarkup::format()s

Updated Proposal to make that more clear.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -718,11 +718,11 @@ protected function validate() {
   +          $this->logError($this->t('Entity type mismatch on rename. @old_type not equal to @new_type for existing configuration @old_name and staged configuration @new_name.', array('@old_type' => $old_entity_type_id, '@new_type' => $new_entity_type_id, '@old_name' => $names['old_name'], '@new_name' => $names['new_name'])));
   

   So I asked for elaboration on #27 in IRC. This:
   array('old_type' => $old_entity_type_id, 'new_type' => $new_entity_type_id, 'old_name' => $names['old_name'], 'new_name' => $names['new_name'])
   ...is not a valid placeholder array. Let's zoom in again:
   array('old_type'

   There's no placeholder token at all. And not only that, but we were testing for there being no placeholder token, with SafeMarkup::format() with !placeholders that are just placeholders. So obviously we can't update those tests without also updating the strings to be not broken, and it'd be silly to move that into a separate blocking issue.

2. +++ b/core/modules/locale/locale.pages.inc
   @@ -80,10 +80,10 @@ function template_preprocess_locale_translation_update_info(array &$variables) {
   -          '!info' => $update['info'],
   +          '@info' => $update['info'],
   

   This was the only change in the patch that wasn't obvious to me. From the context of this preprocess, this info variable really could be anything, so I checked with @alexpott. This is set in /TranslationStatusForm::prepareUpdateData() to the result of TranslationStatusForm::createInfoString(), which always returns exactly some t(). So it's already in the safe list and there is no change to it by using @. Related followup: #2565123: Move logic from SafeMarkup::format() from template_preprocess_locale_translation_update_info() to template

Committed and pushed to 8.0.x. Thanks! (The followup is only tangentially related at this point so wasn't worth holding the commit for.)