Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests

Here's all of them, let's see how many break and how many mistakes I made;)

Overshot and undershot a few. And testbot engage!

194 fails in one test class: Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest
That should be easy to knock down, if we find out why they are failing.

Sorry to jump the boat @xjm. Well now you have a big picture to run with I guess.

The reason for the many failures here:

-    return !parent::needsRemoval($html_tags, $elem);
+    return @parent::needsRemoval($html_tags, $elem);

That's wrong :)

Oh I probably broke a bunch like that;) I went for fast instead of accurate:P

There may be more that I missed but this cleans up a few including the one in #10 thanks @Wim Leers.

--color-words was helpful, thanks xjm for showing me that a while back

Now working on failing tests.

I am having problem in debugging these tests Drupal\migrate_drupal\Tests\MigrateTableDumpTest and Drupal\system\Tests\Common\AddFeedTest, any help regarding this would be great. Rest of the things are fixed.

Removed verbose messages.

Reroll of #22 thank you for knocking it down to 4 fails @subhojit777!

Going to see if I can get them down to 0 today.

Seems I missed a bunch of things... mostly in hook_help()... this patch has grown. I reverted my slight name changes from percent back to pct and @error_message back to @e.

Well that's nice that it's the same # of fails after almost doubling the patch size...

My interdiff may suck on the migrate fixes, but the other two are in it;)

Crossing fingers.

Hmm instead of reverting the migrate stuff I should have went the other direction. Missed a few more things in there.
That revert fixed one but broke the other worse:P

Did a few *.js ones as well.

One problem is the feed icon. I can't do it because we remove SafeMarkup from the Attribute classes and just escape it now. So the problem is that t('@title') will escape once, then the result is sent to the Attribute where it's escaped a second time. I'm surprised we didn't run into more of those cases. @xjm Can or should we work around this some other way?

Going to fix feed_icon in #2531824: Attribute class to check safe strings before escaping (has tests) and it's ilk.

Re-roll and touchups to see where this is at (litmus test)

I should probably do this on an ignore test issue...

One way that helps me find these is a case sensitive regex project search for:
['"]!(?!IE)[a-zA-Z]

And exclude the following folders/files: -vendor/*,-*.css,-*.json

Then when I find one I just search the project for instances of that same one and any remaining ! that may be left over in that file. I think it catches about 99% of the cases.

There a few false positives there including people using that ! to as their regex delimiter, but excludes the !IE false positive that is quite frequent.

This should help a bit further with migrate tests.

I think the error message one will likely be fixed in #2501319: Remove SafeMarkup::set() in Error::renderExceptionSafe() and _drupal_log_error() and improve backtrace formatting consistency in _drupal_log_error(), Error::renderExceptionSafe(), and DefaultExceptionSubscriber::onHtml ()

the fail
Drupal\Tests\migrate\Unit\MigrateExecutableTest

Parameter 0 for invocation Drupal\migrate\MigrateMessageInterface::display('Migration failed with source ...{<(', 'error') does not match expected value. Other MigrateExecutableTest.php 61 Drupal\Tests\migrate\Unit\MigrateExecutableTest->testImportWithFailingRewind()

@Ryan Weal said was failing sometimes, and

  public function testImportWithFailingRewind() {
    $exception_message = $this->getRandomGenerator()->string();

is randomly generating a message which sometimes has some stuff that gets escaped.

So, I think we should not randomly generate the message.
Either
a) set a message string that doesn't need escaping
or
b) set a message that does need escaping
or..
c) both

and then make the tests test what we expect will happen.

I seem to be getting the following for $logs[0]:

string(162) "Unexpected error during import with operation create for config_test.dynamic.primary: 'config_test' entity with ID 'secondary' already exists."

And the expected is:

string(142) "Unexpected error during import with operation create for config_test.dynamic.primary: 'config_test' entity with ID 'secondary' already exists."

Could this be the solution?

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -762,7 +763,7 @@ protected function processConfiguration($collection, $op, $name) {
        catch (\Exception $e) {
   -      $this->logError($this->t('Unexpected error during import with operation @op for @name: !message', array('@op' => $op, '@name' => $name, '!message' => $e->getMessage())));
   +      $this->logError($this->t('Unexpected error during import with operation @op for @name: @message', array('@op' => $op, '@name' => $name, '@message' => SafeMarkup::format($e->getMessage()))));
   

   The SafeMarkup::format around the $e->getMessage is consistent with what we do in Drupal\config\Tests\ConfigImporterTest::testSecondaryWriteSecondaryFirst

2. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
   @@ -296,7 +296,7 @@ function testSecondaryWriteSecondaryFirst() {
        $message = SafeMarkup::format("'config_test' entity with ID '@name' already exists", array('@name' => 'secondary'));
   -    $this->assertEqual($logs[0], SafeMarkup::format('Unexpected error during import with operation @op for @name: !message.', array('@op' => 'create', '@name' => $name_primary, '!message' => $message)));
   +    $this->assertEqual($logs[0], SafeMarkup::format('Unexpected error during import with operation @op for @name: @message.', array('@op' => 'create', '@name' => $name_primary, '@message' => $message)));
      }
   

   The $message is the return of SafeMarkup::format()

\Drupal\user\Tests\UserBlocksTest passes for me locally and it appears to have passed on the testbot.

1. +++ b/core/includes/install.core.inc
   @@ -2027,7 +2027,13 @@ function install_check_translations($langcode, $server_pattern) {
   +      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href="@
   +@
   +@server_url">@
   +@
   +@server_url</a>.', array('@
   +@
   +@server_url' => $server_url)),
   

   We are doing this a couple of times, any reason why?

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -762,7 +763,7 @@ protected function processConfiguration($collection, $op, $name) {
   +      $this->logError($this->t('Unexpected error during import with operation @op for @name: @message', array('@op' => $op, '@name' => $name, '@message' => SafeMarkup::format($e->getMessage()))));
   

   We shouldn't be using SafeMarkup::format() without arguments, it's basically the same as calling SafeMarkup::set() #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise

+++ b/core/modules/locale/locale.module
@@ -149,28 +149,36 @@ function locale_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('Translation files with translated interface text are imported automatically when languages are added on the <a href="@languages">Languages</a> page, or when modules or themes are enabled. On the <a href="@locale-settings">Settings</a> page, the <em>Translation source</em> can be restricted to local files only, or to include the <a href="@
+@
+@server">Drupal translation server</a>. Although modules and themes may not be fully translated in all languages, new translations become available frequently. You can specify whether and how often to check for translation file updates and whether to overwrite existing translations on the <a href="@locale-settings">Settings</a> page. You can also manually import a translation file on the <a title="User interface translation import" href="@import">Import</a> page.', array('@import' => \Drupal::url('locale.translate_import'), '@locale-settings' => \Drupal::url('locale.settings'), '@languages' => \Drupal::url('entity.configurable_language.collection'), '@
+@
+@server' => 'https://localize.drupal.org')) . '</dd>';
...
+      $output .= '<dd>' . t('You can check how much of the interface on your site is translated into which language on the <a href="@languages">Languages</a> page. On the <a href="@translation-updates">Available translation updates</a> page, you can check whether interface translation updates are available on the <a href="@
+@
+@server">Drupal translation server</a>.', array('@languages' => \Drupal::url('entity.configurable_language.collection'), '@translation-updates' => \Drupal::url('locale.translate_status'), '@
+@
+@server' => 'https://localize.drupal.org')) . '<dd>';

+++ b/core/modules/node/node.module
@@ -82,19 +82,23 @@ function node_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('The Node module makes a number of permissions available for each content type, which can be set by role on the <a href="@
+@
+@permissions">permissions page</a>.', array('@
+@
+@permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-node')))) . '</dd>';

+++ b/core/modules/path/path.module
@@ -20,13 +20,17 @@ function path_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('If you create or edit a taxonomy term you can add an alias (for example <em>music/jazz</em>) in the field "URL alias". When creating or editing content you can add an alias (for example <em>about-us/team</em>) under the section "URL path settings" in the field "URL alias". Aliases for any other path can be added through the page <a href="@aliases">URL aliases</a>. To add aliases a user needs the permission <a href="@
+@
+@permissions">Create and edit URL aliases</a>.', array('@aliases' => \Drupal::url('path.admin_overview'), '@
+@
+@permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-path')))) . '</dd>';

+++ b/core/modules/statistics/statistics.module
@@ -19,13 +19,17 @@ function statistics_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('The Statistics module includes a counter for each page that increases whenever the page is viewed. To use the counter, enable <em>Count content views</em> on the <a href="@statistics-settings">Statistics page</a>, and set the necessary <a href="@
+@
+@permissions">permissions</a> (<em>View content hits</em>) so that the counter is visible to the users.', array('@statistics-settings' => \Drupal::url('statistics.settings'), '@
+@
+@permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-statistics')))) . '</dd>';

+++ b/core/modules/taxonomy/taxonomy.module
@@ -41,20 +41,24 @@ function taxonomy_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<p>' . t('The Taxonomy module allows users who have permission to create and edit content to categorize (tag) content of that type. Users who have the <em>Administer vocabularies and terms</em> <a href="@
+@
+@permissions" title="Taxonomy module permissions">permission</a> can add <em>vocabularies</em> that contain a set of related <em>terms</em>. The terms in a vocabulary can either be pre-set by an administrator or built gradually as content is added and edited. Terms may be organized hierarchically if desired.', array('@
+@
+@permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-taxonomy')))) . '</p>';
+      $output .= '<p>' . t('For more information, see the <a href="@taxonomy">online documentation for the Taxonomy module</a>.', array('@taxonomy' => 'https://www.drupal.org/documentation/modules/taxonomy/')) . '</p>';

+++ b/core/modules/user/user.module
@@ -75,10 +75,14 @@ function user_help($route_name, RouteMatchInterface $route_match) {
+      return '<p>' . t('A role defines a group of users that have certain privileges. These privileges are defined on the <a href="@
+@
+@permissions">Permissions page</a>. Here, you can define the names and the display sort order of the roles on your site. It is recommended to order roles from least permissive (for example, Anonymous user) to most permissive (for example, Administrator user). Users who are not logged in have the Anonymous user role. Users who are logged in have the Authenticated user role, plus any other roles granted to their user account.', array('@
+@
+@permissions' => \Drupal::url('user.admin_permissions'))) . '</p>';

Missed some, also interdiffs are really needed when making changes on this size of a patch.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -762,7 +764,7 @@ protected function processConfiguration($collection, $op, $name) {
+      $this->logError($this->t('Unexpected error during import with operation @op for @name: @message', array('@op' => $op, '@name' => $name, '@message' => SafeString::create($e->getMessage()))));

I'm not sure if this is the best alternative to SafeMarkup::format either, don't know what the better alternative is though.
I looked this up and this actually had an @ before but was changed in #2514044: Do not use SafeMarkup::format in exceptions

Besides my open question in #75 the patch looks good, would be great of someone else can confirm that it's ok to use SafeString::create() here.

@geertvd regarding #75 using SafeString::create() instead of SafeMarkup, allows us to mark the string as safe without poluting the large list of safeStrings that it holds on it. So we are slowly but surely moving away from SafeMarkup and towards SafeString as it is closer to what we aimed to do in the original autoescape issue and what Twig_Markup does.

That being said... I think we need to just be careful just the same as SafeMarkup::set() can be abused if used improperly... If we don't mark the string as safe and we use '@' it will escape everything in that variable, I'm curious what's in that variable that would cause escaping to produce negative results?

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -7,6 +7,8 @@
   +use Drupal\Component\Utility\SafeMarkup;
   

   This is an un-used use statement.

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -762,7 +764,7 @@ protected function processConfiguration($collection, $op, $name) {
   -      $this->logError($this->t('Unexpected error during import with operation @op for @name: !message', array('@op' => $op, '@name' => $name, '!message' => $e->getMessage())));
   +      $this->logError($this->t('Unexpected error during import with operation @op for @name: @message', array('@op' => $op, '@name' => $name, '@message' => SafeString::create($e->getMessage()))));
   

   I could be wrong but this is because the message has backtraces in it's message or some other HTML?

+++ b/core/modules/config/src/Tests/ConfigImporterTest.php
@@ -296,7 +296,7 @@ function testSecondaryWriteSecondaryFirst() {
     $message = SafeMarkup::format("'config_test' entity with ID '@name' already exists", array('@name' => 'secondary'));

The problematic variable is this one.
Doesn't SafeMarkup::format() mark a string as safe? So I really don't understand why the @placeholder is not inserting it unescaped.
Could be saying something silly here, that SafeMarkup stuff can get confusing.

Reroll of patch on #80.

Further investigation into ConfigImporter & ConfigImporterTest to hopefully help answer questions in #79 & #81

The error in $e->getMessage() is an exception thrown here:

EntityStorageBase:: doPreSave()

protected function doPreSave(EntityInterface $entity) {
    $id = $entity->id();

    // Track the original ID.
    if ($entity->getOriginalId() !== NULL) {
      $id = $entity->getOriginalId();
    }

    // Track if this entity exists already.
    $id_exists = $this->has($id, $entity);

    // A new entity should not already exist.
    if ($id_exists && $entity->isNew()) {
      throw new EntityStorageException("'{$this->entityTypeId}' entity with ID '$id' already exists.");

It looks like the only characters to get escaped are the quotes unless the ID or entityTypeId have special characters. Not sure if this means we are comfortable with marking this safe? I guess if you can mess with the configuration import (files), you could do other security damage?

Looking at the output to the user, manual testing of this error produce the same visual output with the different escape / print methods. A screen grab is attached of that output. The source is different (the escaped single quote):

In ConfigImporter::processConfiguration()
Bang ( '!message' => $e->getMessage() )

<div role="alert">
  <h2 class="visually-hidden">Error message</h2>
    Unexpected error during import with operation create for config_test.dynamic.primary: &#039;config_test&#039; entity with ID &#039;secondary&#039; already exists.
</div>

SafeString @ ( '@message' => SafeString::create($e->getMessage()) )

<div role="alert">
  <h2 class="visually-hidden">Error message</h2>
    Unexpected error during import with operation update for config_test.dynamic.primary: 'config_test' entity with ID 'secondary' already exists.
</div>

@ only ( '@message' => $e->getMessage() )

<div role="alert">
  <h2 class="visually-hidden">Error message</h2>
    Unexpected error during import with operation update for config_test.dynamic.primary: &#039;config_test&#039; entity with ID &#039;secondary&#039; already exists.
</div>

Regarding ConfigImporterTest::testSecondaryWriteSecondaryFirst(), we don't need to mark these safe, as long as the assertions match the logged value. To avoid using SafeMarkup::format, we could do something like:

+use Drupal\Component\Utility\Html;

@@ -295,8 +296,8 @@ function testSecondaryWriteSecondaryFirst() {
 
     $logs = $this->configImporter->getErrors();
     $this->assertEqual(count($logs), 1);
-    $message = SafeMarkup::format("'config_test' entity with ID '@name' already exists", array('@name' => 'secondary'));
-    $this->assertEqual($logs[0], SafeMarkup::format('Unexpected error during import with operation @op for @name: @message.', array('@op' => 'create', '@name' => $name_primary, '@message' => $message)));
+    $message = Html::escape("'config_test' entity with ID 'secondary' already exists");
+    $this->assertEqual($logs[0], "Unexpected error during import with operation create for " . $name_primary . ": " . $message . ".");
   }

We can drop the Html::escape() if the single quotes aren't escaped in the error.

I'm sure there are more efficient methods to reproduce the error manually, but here's how I did it:

1. Grab Exported config from failing test. I’ve placed a copy of this: https://github.com/justAChris/DrupalConfigFail
2. Add config_test, config_import_test modules from config test directory /core/modules/config/tests/ and Enable
3. Export databse config to staging folder (drush config-export)
4. Add failing test files from step 1 to staging folde
5. Synchronize configuration at /admin/config/development/configuration, observe

I'd imagine it is uncommittable as well even if it did get to that stage. Not sure if it's helpful but attached a list of all the files touched by the latest reroll (#85).
205 Files, not sure the method to break this down, by module & core subsystem?

That is helpful @justAChris: Some things relate to their tests but for the most part they can be hunked together.
Could you do the same thing but also show the diff --stats so we know the how many changes are in the files?

Let's find the largest set of changes in a module or subsystem and group a couple of those to hack hunks of this patch down quick. Then we can deal with the one-off and smaller bits likely in this issue as hopefully a <100K patch.

Stats for latest reroll attached.

Not sure if @xjm intended to move this planning to #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand since that was marked as meta.

At the very least this is back to Active now, if not closed (duplicate).

Whoops meta meta

OK perfect, while doing this I realized a bunch were in hook_help text, maybe even the majority. Just a side note...

Here's the list in 85 ordered by changes.

@justAChris If we close this we just need to make sure people who worked on it get credited in the spin-off issue(s).

Here's a proposal for splitting this into chunks:

1. Contact: core/modules/contact/*
2. Views: core/modules/views/*
3. Comment: core/modules/comment/*
4. Migrate: core/modules/migrate/*
5. Skipping System module because it's a bit of a grab bag and Form stuff.
6. Views UI: core/modules/views_ui/*
7. Language: core/modules/language/*
8. Locale: core/modules/locale/*
9. Update: core/modules/update/*
10. User: core/modules/user/*
11. SimpleTest: core/modules/simpletest/*
12. Search: core/modules/search/*
13. Help: core/modules/help/*
14. Skipping SysLog Stuff but may affect Migrate
15. Node: core/modules/node/*
16. Config Translation: core/modules/config_translation/*
17. Block & Block Content: core/modules/block*
18. Aggregator: core/modules/aggregator/*

Cherry picking a bit but how does that sound?

First child issue added (for contact module): #2559435: Replace !placeholder with @placeholder in Contact module

Items 1-18 on #93 have been opened (with exception of 5 & 14) as child issues of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand and marked related to this.

Per conversation w/ @joelpittet on irc, this issue should cover anything not covered by new issues, re-titled as such. This issue may be postponed until we determine the scope of "remaining".

For future reference, @joelpittet won the issue opening race :p

Here's a re-roll and just to help with those split offs I've added a test for all help pages!

Maybe it would be worth doing the tests in one shot with this test in play that way people aren't doing the hook_help() test which are probably the bulk of this patch.

I opened it up, it may mean a bit of re-rolling on some of the other issues but I think it will be worth it because it's 300k from this patch and and automated test to cover them all in two assert lines.

#2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations

+++ b/core/includes/file.inc
@@ -371,8 +371,8 @@ function file_save_htaccess($directory, $private = TRUE, $force_overwrite = FALS
+    $variables = array('%directory' => $directory, '@htaccess' => '<br />' . nl2br(Html::escape($htaccess_lines)));
+    \Drupal::logger('security')->error("Security warning: Couldn't write .htaccess file. Please create a .htaccess file in your %directory directory which contains the following lines: <code>@htaccess

", $variables);

This is double escape in a nut-shell. See #2564321: file_save_htaccess() generates error logs which are escaped incorrectly for a proper fix.

I will review the issue referenced in #98 and prepare a patch to address the double escape.

Reroll necessary since the following issue was fixed: #2560733: Remove a few problematic t() calls from tests

I will address the feedback in #98 next.

This addresses the feedback in #98.

If #2564321: file_save_htaccess() generates error logs which are escaped incorrectly gets committed before this issue we will be removing the changes to core/includes/file.inc entirely as the code change is identical.

Since we are doing that fix in #2564321: file_save_htaccess() generates error logs which are escaped incorrectly we shouldn't do the fix here also, IMO it should just remove that from the patch right away.

@geertvd that had occurred to me but if we did that we would need to postpone this issue until that one was committed?

My opinion is that I feel comfortable including it in both as it is not the significant portion of either patch. And it removes the dependency chain for each issue.

Happy to fall in though but I understood from the comment in #98 by @alexpott that including the same fix was being requested.

Leaving as needs review unless anyone feels very strongly about it.

We're also discussing the ! in #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness. Given the discussions there I think we should postpone this.

Can Ryan Weal, nlisgo, subhojit777, and justAChris comment on #2564353: Remove !placeholder usage from SafeMarkup::format() so they can get credit.

I think we could rescope this issue to fix all of the !placeholder by doing the following regex on the code base [\s>.\(]t\('[^']*(?<!href=")![a-zA-Z].

This issue should not convert:

• !placeholder where the placeholder is a url
• !placeholder where output of t() is being used for non-HTML such as an email

There will be around 170 or so things to fix.

@xjm try this if you use ag.
ag '[\s>.\(]t\('[^']*(?<!href=")![a-zA-Z]'
Single quotes being of utmost importance.

EDIT: Well that didn't work actually, bash sucks and I forgot to escape things...

Let me try!

There are still changes... I am continue tomorrow.

@xjm the bash command, I think that have false positives, here a example:

core/lib/Drupal/Core/Datetime/Element/Datelist.php: $form_state->setError($element, t('The %field date is invalid.', array('%field' => !empty($element['#title']) ? $element['#title'] : '')));

This will need a reroll now that #2564353: Remove !placeholder usage from SafeMarkup::format() has landed and that will fix the config tests.

+++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
@@ -67,8 +67,8 @@
-   *         $message['subject'] = t('Notification from !site', $variables, $options);
-   *         $message['body'][] = t("Dear !username\n\nThere is new content available on the site.", $variables, $options);
+   *         $message['subject'] = t('Notification from @site', $variables, $options);
+   *         $message['body'][] = t("Dear @username\n\nThere is new content available on the site.", $variables, $options);

+++ b/core/modules/contact/contact.module
@@ -140,14 +140,14 @@ function contact_mail($key, &$message, $params) {
-      $message['subject'] .= t('[!form] !subject', $variables, $options);
-      $message['body'][] = t("!sender-name (!sender-url) sent a message using the contact form at !form-url.", $variables, $options);
+      $message['subject'] .= t('[@form] @subject', $variables, $options);
+      $message['body'][] = t("@sender-name (@sender-url) sent a message using the contact form at @form-url.", $variables, $options);
...
-      $message['subject'] .= t('[!form] !subject', $variables, $options);
+      $message['subject'] .= t('[@form] @subject', $variables, $options);

@@ -157,10 +157,10 @@ function contact_mail($key, &$message, $params) {
-      $message['subject'] .= t('[!site-name] !subject', $variables, $options);
-      $message['body'][] = t('Hello !recipient-name,', $variables, $options);
-      $message['body'][] = t("!sender-name (!sender-url) has sent you a message via your contact form at !site-name.", $variables, $options);
-      $message['body'][] = t("If you don't want to receive such emails, you can change your settings at !recipient-edit-url.", $variables, $options);
+      $message['subject'] .= t('[@site-name] @subject', $variables, $options);
+      $message['body'][] = t('Hello @recipient-name,', $variables, $options);
+      $message['body'][] = t("@sender-name (!sender-url) has sent you a message via your contact form at @site-name.", $variables, $options);
+      $message['body'][] = t("If you don't want to receive such emails, you can change your settings at @recipient-edit-url.", $variables, $options);

+++ b/core/modules/contact/src/MailHandler.php
@@ -91,7 +91,7 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
-      $sender_cloned->name = $this->t('!name (not verified)', array('!name' => $message->getSenderName()));
+      $sender_cloned->name = $this->t('@name (not verified)', array('@name' => $message->getSenderName()));

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -83,7 +83,7 @@ function testSendPersonalContactMessage() {
-    $this->assertEqual($mail['subject'], t('[!site-name] !subject', $variables), 'Subject is in sent message.');
+    $this->assertEqual($mail['subject'], t('[@site-name] @subject', $variables), 'Subject is in sent message.');

Not correct - these are non html usage. And should not be changed in this issue.

Reroll and @alexpott's suggestions.

Posting a the results of the two regular expressions used thus far to find !placeholder in non href strings for comparison. Just cross verifying that they mainly produce the same results (with a few exceptions) to make sure we attempt to find all instances, which is the goal regardless of what regex is used.

non-href-slice-n-dice.txt using the grep currently listed in the issue summary.
non-href-lookbehind.txt is using the regex found in #107

Per #114 and irc conversation with @xjm and @joelpittet, the subset of this patch that are contained in test assertions has been split off: #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only.

This new issue should be actionable now as its resolution appears to be independent of the result of planning in the parent meta issue.

Managed to fix one failing test.

Straight reroll.

Updating Bash command in issue summary to indicate that the replacement/removal in Tests should be handled in a different issue, currently #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only.

I am aware that the replacements in tests will be handled in a different issue, but still investigate the errors. They need to be fixed any way. This is the first one:

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -78,7 +78,7 @@ public function testNodeEdit() {
     // Check that the title and body fields are displayed with the correct values.
     $active = '<span class="visually-hidden">' . t('(active tab)') . '</span>';
-    $link_text = t('!local-task-title!active', array('!local-task-title' => t('Edit'), '!active' => $active));
+    $link_text = t('@local-task-title@active', array('@local-task-title' => t('Edit'), '@active' => $active));
     $this->assertText(strip_tags($link_text), 0, 'Edit tab found and marked active.');
     $this->assertFieldByName($title_key, $edit[$title_key], 'Title field displayed.');

This test fails because @placehoders are escaped the $link_text will now for example contain < instead of <

+++ b/core/includes/install.inc
@@ -558,7 +558,7 @@ function drupal_verify_profile($install_state) {
diff --git a/core/lib/Drupal/Core/Config/ConfigImporter.php b/core/lib/Drupal/Core/Config/ConfigImporter.php

diff --git a/core/lib/Drupal/Core/Config/ConfigImporter.php b/core/lib/Drupal/Core/Config/ConfigImporter.php
index c02d4fe..13b8795 100644

index c02d4fe..13b8795 100644
--- a/core/lib/Drupal/Core/Config/ConfigImporter.php

--- a/core/lib/Drupal/Core/Config/ConfigImporter.php
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -762,7 +762,7 @@ protected function processConfiguration($collection, $op, $name) {

@@ -762,7 +762,7 @@ protected function processConfiguration($collection, $op, $name) {
       }
     }
     catch (\Exception $e) {
-      $this->logError($this->t('Unexpected error during import with operation @op for @name: !message', array('@op' => $op, '@name' => $name, '!message' => $e->getMessage())));
+      $this->logError($this->t('Unexpected error during import with operation @op for @name: @message', array('@op' => $op, '@name' => $name, '@message' => $e->getMessage())));
       // Error for that operation was logged, mark it as processed so that

Working on a new patch ...

The ConfigImporterTest test fails because @message contains quotes that get encoded due to the @placeholder

@Sutharsan thanks for working on this. Would you like to write a patch for this issue #2509218: Ensure that SafeString objects can be used in non-HTML contexts as well. There are some tests failing there which are same as you just fixed here.

@sughojit777 I Currently focus on those issues that are likely to be marked critical as per #2506427-51: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand.

Question. If this is scriptable, would it help if we instituted a small "commit freeze," rolled the mega-patch once, and committed it all in one go? If so, I'd be available later on today to do this.

would it help if we instituted a small "commit freeze" ...

Nah, chasing head is fun ;) I'm currently checking the existing 'Replace !placeholder' patches. 1/3rd needs a reroll and most of the patches are only 6 - 10 days old. A freeze would help, but lets not rush. Get a combined and bot tested patch first, do some basic testing and then find a good moment to commit. Rerolling these patches is not super complicated and rerolling a single patch is already a lot better than 18 of them.

This is the combined patch of the following issues:

• 2506445 This issue
• 2551743 Config, Credits to joelpittet, lauriii, Sutharsan
• 2559435 Contact module, Credits to geertvd, izus, Sutharsan
• 2559441 Views module, Credits to geertvd, joelpittet, Sutharsan
• 2559445 Aggregator module, Credits to geertvd
• 2559447 Block & Block Content modules, Credits to joelpittet, geertvd
• 2559449 Config Translation module, Credits to geertvd, joelpittet
• 2559451 Node module, Credits to geertvd, joelpittet, Sutharsan
• 2559453 Help module, Credits to joelpittet
• 2559455 Search module, Credits to joelpittet, izus
• 2559459 User module, Credits to HOG, andypost, borisson_
• 2559461 Comment module, Credits to izus, joelpittet, Sutharsan
• 2559467 Migrate and migrate_drupal modules, Credits to joelpittet
• 2559469 Update module, Credits to izus, joelpittet, Sharique
• 2559471 views_ui module, Credits to joelpittet
• 2559475 Language module Credits to Sutharsan, joelpittet
• 2559479 Locale module, Credits to joelpittet, Sutharsan
• 2560783 hook_help(), Credits to joelpittet, lauriii, Sutharsan

Pls ignore patch in #140, it doesn't apply.

Ok, new patch.
This replaces the !placeholders in *Test.php files according to the regex in the issue summary.

@Sutharsan: Out of curiosity, what command did you pipe that into to do the actual find/replacing? This way you could go to bed sometime soon and someone from North America / Australia could take over if they were so inclined. :D

@webchick, yes going to bed is my next priority.
I used the patch from #143 as starting point and only used the Test.php file from it (the regex script filters on presence of 'Text.php' in the file name). Next I used the regex to find the remaining !placeholders and find/replace the with an editor (not typing, too much risk of typo). As last step (this new patch) check for 'href=' or 'src=' in the patch and revert those changes. And search for usage in mail context (#144). That was my recipe.

This patch reverts changes for URL usage. No mail (or other non-html) usage identified.

Removed lines highlighted in comments #155 - #160.

Updated Issue Summary to indicate issues that should be filed to resolve this edge cases. I haven't yet filed them myself.
Regarding #158, those tests already exist in the hook_help() issue: #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations. We should keep that issue open, but at postponed since it may come in handy if we change the placeholder in URLs

Also, haven't cross referenced against the bash script or any other regex to ensure that we've captured the entire scope.

Revert core/modules/locale/src/Tests/LocaleJavascriptTranslationTest.php correctly this time.

Drupal\views\Tests\Plugin\DisplayFeedTest? Sounds like that may be spurious. Trying a re-rest, since DrupalCI liked it well enough.

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -79,12 +79,12 @@ function testSendPersonalContactMessage() {
-      '!site-name' => $this->config('system.site')->get('name'),
-      '!subject' => $message['subject[0][value]'],
-      '!recipient-name' => $this->contactUser->getUsername(),
+      '@site-name' => $this->config('system.site')->get('name'),
+      '@subject' => $message['subject[0][value]'],
+      '@recipient-name' => $this->contactUser->getUsername(),
...
-    $this->assertEqual($mail['subject'], t('[!site-name] !subject', $variables), 'Subject is in sent message.');
-    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['!recipient-name']) !== FALSE, 'Recipient name is in sent message.');
+    $this->assertEqual($mail['subject'], t('[@site-name] @subject', $variables), 'Subject is in sent message.');
+    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['@recipient-name']) !== FALSE, 'Recipient name is in sent message.');

These are for email tokens and I think they shouldn't be converted. I could be wrong but that is what I recall from one of the other issue and discussions with @effulgentsia

These are for email tokens and I think they shouldn't be converted.

You are correct. No output escaping in email context, or other non-html output such as a webservice response. Their front-end (e.g. mail client) is responsible for that.

Created the follow-up issue for #156, #157 and #158: #2567727: LocaleJavascriptTranslationTest needs test coverage for @placeholders in JavaScript

As far as #171 goes, looks OK to me.

About 20 of the replacements in the latest patch are in format_string() which looks to be outside the scope of this issue

+++ b/core/modules/aggregator/src/Tests/UpdateFeedItemTest.php
@@ -37,10 +37,10 @@ public function testUpdateFeedItem() {
-    $this->assertResponse(array(200), format_string('URL !url is accessible', array('!url' => $edit['url[0][value]'])));
+    $this->assertResponse(array(200), format_string('URL @url is accessible', array('@url' => $edit['url[0][value]'])));

And also, potentially wrong given 9.0.0 deprecation

On the positive side, running the bash grep in the issue summary against the patched codebase only returns items we have specifically excluded above:

core/modules/contact/src/Tests/ContactPersonalTest.php:    $this->assertEqual($mail['subject'], t('[!site-name] !subject', $variables), 'Subject is in sent message.');
core/modules/system/src/Tests/Common/XssUnitTest.php:    $text = t('Verbatim text: !value', array('!value' => '<script>'));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertFieldByXpath('//input[@type="submit"]', t('Duplicate !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertFieldByXpath('//input[@type="submit"]', t('Delete !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertFieldByXpath('//input[@type="submit"]', t('Disable !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertNoFieldByXpath('//input[@type="submit"]', t('Enable !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->drupalPostForm(NULL, NULL, t('Disable !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertFieldByXpath('//input[@type="submit"]', t('Enable !display_title', $placeholder));
core/modules/views_ui/src/Tests/DisplayTest.php:    $this->assertNoFieldByXpath('//input[@type="submit"]', t('Disable !display_title', $placeholder));

1. +++ b/core/modules/config_translation/tests/src/Unit/ConfigEntityMapperTest.php
   @@ -70,7 +70,7 @@ protected function setUp() {
   -      'title' => '!label language',
   +      'title' => '@label language',
   

   Not in t()

2. +++ b/core/modules/config_translation/tests/src/Unit/ConfigFieldMapperTest.php
   @@ -50,7 +50,7 @@ protected function setUp() {
   -      'title' => '!label field',
   +      'title' => '@label field',
   

   Also not in t()

3. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -59,7 +59,7 @@ protected function setUp() {
   -    $exception_message = $this->getRandomGenerator()->string();
   +    $exception_message = $this->getRandomGenerator()->name();
   

   Is this supporting some other Test assertion? seems wrong in this scope. EDITED: Maybe needed, see #52. Maybe run these tests locally without this update and see if it still fails?

4. +++ b/core/modules/node/src/Tests/NodeEditFormTest.php
   @@ -77,8 +78,8 @@ public function testNodeEdit() {
   -    $active = '<span class="visually-hidden">' . t('(active tab)') . '</span>';
   -    $link_text = t('!local-task-title!active', array('!local-task-title' => t('Edit'), '!active' => $active));
   +    $active = SafeMarkup::format('<span class="visually-hidden">@active_tab</span>', array('@active_tab' => t('(active tab)')));
   +    $link_text = t('@local-task-title@active', array('@local-task-title' => t('Edit'), '@active' => $active));
   

   Should we even be touching the first line here? no placeholder. Or does it affect the second line? Is this really the best way to do this in that case? EDITED: Explanation in #133, but does this still apply since we reduced the scope to tests only?

About 20 of the replacements in the latest patch are in format_string() which looks to be outside the scope of this issue

I'm ok with those being included in this patch. Is there a reason not to? The issue summary currently says "in t() usage only". Should we change the issue summary to also include SafeMarkup::format() usage or do we have/want a separate issue for that and then remove those from this patch?

And also, potentially wrong given 9.0.0 deprecation

If we include them in the patch, I'd be ok with either them being changed to SafeMarkup::format() while we're at it (since we're changing that line anyway), or letting that conversion be a separate issue.

Re #176.1/2: They are in t(), just indirectly, via ConfigEntityMapper::getTitle(). However, because of that, we also need to change the prefix used in that method, so let's remove that from here and do it in #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only.

Re #176.4: I don't get the question. This is in a test.

Re #176.3: I opened #2567793: Random test failures in MigrateExecutableTest when random string returns unescaped html entities. Can anyone help to quickly resolve that issue? Not sure if it's a hard blocker or soft blocker for this one.

Interesting. On its own, #2567793: Random test failures in MigrateExecutableTest when random string returns unescaped html entities passes. Therefore, trying this to see if something in this patch is making it fail.

These two test modules are overlooked by the regex:
FormTestVerticalTabsForm.php:
t('Field !num', array('!num' => $i)),
'#title' => t('Field !num', array('!num' => $i)),

node_test.module:
t('Value of testElement RSS element for node !nid.', ...
t('Extra data that should appear only in the RSS feed for node !nid.' ...
t('Extra data that should appear everywhere except the RSS feed ... !nid ...