SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   index 054d101..4beb133 100644
   --- a/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   
   --- a/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   @@ -114,7 +114,7 @@ public function set($property_name, $value, $notify = TRUE) {
   
   @@ -114,7 +114,7 @@ public function set($property_name, $value, $notify = TRUE) {
       */
   

   You can remove the use statement now, yeah!

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -145,7 +145,7 @@ public function tips($long = FALSE) {
   -          array('data' => SafeMarkup::format($tips[$tag][1]), 'class' => array('get'))
   +          array('data' => ['#markup' => $tips[$tag][1]], 'class' => array('get'))
   

   Yeah this call was effectively a SafemMarkup::set() call, right? On the other hand #markup will be ran through Xss::filterAdmin() which supports a supersets of these tags:

   array_diff($tips, $a);
   => []

3. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -372,8 +372,8 @@ function testFilterTipHtmlEscape() {
        $link = '<a href="' . $base_url . '">' . SafeMarkup::checkPlain(\Drupal::config('system.site')->get('name')) . '</a>';
   ...
   +    $link_as_code = '<code>' . htmlspecialchars($link, ENT_QUOTES, 'UTF-8') . '

   ';
   + $ampersand_as_code = '' . htmlspecialchars($ampersand, ENT_QUOTES, 'UTF-8') . '';
   

   Doesn't that result into double escaping now? It is just a test, but still ...

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -144,7 +143,9 @@ public function tips($long = FALSE) {
+          // The markup must be escaped.

@@ -175,7 +176,9 @@ public function tips($long = FALSE) {
+        // The markup must be escaped.

Should it be explained why? ... because you want to see the tag for itself?

This looks great and is hilarious that this was done so many times!

There are a few things in the patch that seem a bit out of the scope of the issue.

1. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -102,7 +101,7 @@ public function tips($long = FALSE) {
   -      'a' => array($this->t('Anchors are used to make links to other pages.'), '<a href="' . $base_url . '">' . SafeMarkup::checkPlain(\Drupal::config('system.site')->get('name')) . '</a>'),
   +      'a' => array($this->t('Anchors are used to make links to other pages.'), '<a href="' . $base_url . '">' . htmlspecialchars(\Drupal::config('system.site')->get('name'), ENT_QUOTES, 'UTF-8') . '</a>'),
   

   Not format, shouldn't be part of this patch.

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -144,8 +143,10 @@ public function tips($long = FALSE) {
   -          array('data' => SafeMarkup::format('<code>@var

   ', array('@var' => $tips[$tag][1])), 'class' => array('type')),
   ...
   + array('data' => ['#prefix' => '', '#markup' => htmlspecialchars($tips[$tag][1], ENT_QUOTES, 'UTF-8'), '#suffix' => ''], 'class' => array('type')),
   

   This one has arguments, doesn't count.

3. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -144,8 +143,10 @@ public function tips($long = FALSE) {
   -          array('data' => SafeMarkup::format($tips[$tag][1]), 'class' => array('get'))
   ...
   +          // The markup must not be escaped.
   +          array('data' => ['#markup' => $tips[$tag][1]], 'class' => array('get'))
   

   implicit admin xss filter?

4. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -175,8 +176,10 @@ public function tips($long = FALSE) {
   -        array('data' => SafeMarkup::format('<code>@var

   ', array('@var' => $entity[1])), 'class' => array('type')),
   ...
   + array('data' => ['#prefix' => '', '#markup' => htmlspecialchars($entity[1], ENT_QUOTES, 'UTF-8'), '#suffix' => ''], 'class' => array('type')),
   

   Same has arguments, out of scope.

5. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -175,8 +176,10 @@ public function tips($long = FALSE) {
   +        array('data' => ['#markup' => $entity[1]], 'class' => array('get'))
   

   These could get a nice comma at the end.

6. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -368,12 +368,15 @@ function testFilterTipHtmlEscape() {
   -    $link = '<a href="' . $base_url . '">' . SafeMarkup::checkPlain(\Drupal::config('system.site')->get('name')) . '</a>';
   +    $link = '<a href="' . $base_url . '">' . htmlspecialchars($site_name_with_markup, ENT_QUOTES, 'UTF-8') . '</a>';
   

   This is out of scope, not SafeMarkup::format()

7. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -368,12 +368,15 @@ function testFilterTipHtmlEscape() {
   -    $link_as_code = '<code>' . $link . '

   ';
   - $ampersand_as_code = '' . $ampersand . '';
   + $link_as_code = '' . htmlspecialchars($link, ENT_QUOTES, 'UTF-8') . '';
   + $ampersand_as_code = '' . htmlspecialchars($ampersand, ENT_QUOTES, 'UTF-8') . '';
   

   SafeMarkup::checkPlain? till that other issue gets in.

This doesn't need to be postponed on that, they are not related to the title of this.

This is a really good self contained fix that should just get in.

Not sure how this passed before really but code values need to be escaped. And now with this change it's probably does need to be postponed.

Postponing on #2550945: Add Html::escape()

Unpostponing this will likely conflict with #2504529: SafeMarkup does not escape some filter tips - remove SafeMarkup usage from FilterHtml

Better escaping testing with assertEscaped()

RTBC from me, just need to find someone to second it.

Seconding, this seems like a good idea!

The method documentation still looks good and grepping the code for any SafeMarkup::set() without test coverage and a second argument I found nothing.

Maybe this could use an assert('$args !== array()') if we wanted to also disallow SafeMarkup::checkPlain($string, array());?

I'm wondering whether there could be generic code which does not know its placeholders, so it maybe an empty array. For them the assert() would make it a bit harder.

Looks like we have all the format_string() calls here, so RTBC

Committed and pushed to 8.0.x. Thanks!

So contrib is now doing lovely things like this to work around the SafeMarkup::set removal

+++ b/src/EntityComparisonBase.php
@@ -83,7 +83,7 @@ class EntityComparisonBase extends ControllerBase {
-    $this->nonBreakingSpace = SafeMarkup::set(' ');
+    $this->nonBreakingSpace = SafeMarkup::format(' ', array());

https://www.drupal.org/node/2563017

Wow, dx be darned.

They haven't used SafeString:create() yet. At least that would only open up xss for their module instead of on the Safemakup sting list and all the site