Refactor use of SafeMarkup::set in \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments()

so I relooked up the difference between set and format to remind myself why this is one of the acceptable ways of removing set.
And format sanatizes the values for the placeholders first, and then marks the string as safe.

The patch stays in scope.

1. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -181,13 +181,13 @@ public static function preRenderConditionalComments($element) {
   -      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
   -      $element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
   +      $element['#prefix'] = SafeMarkup::format("\n<!--[if @expression]>\n@prefix", ['@expression' => $expression, '@prefix' => $prefix]);
   +      $element['#suffix'] = SafeMarkup::format("@suffix<![endif]-->\n", ['@suffix' => $suffix]);
   

   so manual testing this... doesn't include a value for $prefix (and $suffix). So I'm not 100% confident that we are manually testing the escaping of some of this.

2. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -181,13 +181,13 @@ public static function preRenderConditionalComments($element) {
   -      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
   -      $element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");
   +      $element['#prefix'] = SafeMarkup::format("\n<!--[if @expression]><!-->\n@prefix", ['@expression' => $expression, '@prefix' => $prefix]);
   +      $element['#suffix'] = SafeMarkup::format("@suffix<!--<![endif]-->\n", ['@suffix' => $suffix]);
   

   this bit is IR specific, ... do we want to require manual testing in IE?

... I can't remember the tag for manual testing in IE.

so, the strings we are concatenating, are first being run though Xss::filterAdmin() and my initial worry was that we might be marking the parts of the string safe, and then the whole concatenated string safe, and polluting the list of safe strings with fragments.

but, checking the docs on filterAdmin():

   * This method is preferred to
   * \Drupal\Component\Utility\SafeMarkup::xssFilter() when the result is
   * not being used directly in the rendering system (for example, when its
   * result is being combined with other strings before rendering). This avoids
   * bloating the safe string list with partial strings if the whole result will
   * be marked safe.

this is not marking the fragments safe, and seems to be exactly the recommended use case of filterAdmin().

I think @ is still the thing to use with filter... and looked to #2501683: Remove SafeMarkup::set in _update_message_text() and it seems to use @ in format even with strings we are assuming are safe.

----
so, doing a code review, this looks great.

but we might still need some manual testing steps for the prefix and suffix and for IE.

needs work to add tests for the prefix and suffix that comes from the element array

Shouldn't this use SafeString::create()?

To find reviewers: look at the git blame view for this file. I'd say general theme system maintainers are also good candidates to look at this.

Seems the prefix and suffix already have test coverage through HtmlTagTest::testPreRenderConditionalComments() :)

@akalata the #prefix/#suffix is actually not being used in core, but for manual testing, we can add the following code in JsCollectionRenderer::render() when it loops through the JS assets:

$element['#prefix'] = '<blink>test</blink>';
$element['#suffix'] = SafeString::create('<blink>test</blink>');

This may as well use SafeString::create(), patch attached.

This looks like a decent change to make, especially for performance.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -187,13 +188,13 @@ public static function preRenderConditionalComments($element) {
-      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]>\n" . $prefix);
-      $element['#suffix'] = SafeMarkup::set($suffix . "<![endif]-->\n");
+      $element['#prefix'] = SafeString::create("\n<!--[if $expression]>\n" . $prefix);
+      $element['#suffix'] = SafeString::create($suffix . "<![endif]-->\n");
...
-      $element['#prefix'] = SafeMarkup::set("\n<!--[if $expression]><!-->\n" . $prefix);
-      $element['#suffix'] = SafeMarkup::set($suffix . "<!--<![endif]-->\n");
+      $element['#prefix'] = SafeString::create("\n<!--[if $expression]><!-->\n" . $prefix);
+      $element['#suffix'] = SafeString::create($suffix . "<!--<![endif]-->\n");

Although in either case we need to know that we've done our homework.

So variables involved $expression, $prefix, $suffix, need to be documented as to why they are 'safe' (aka have they been escaped/filtered/etc previously?) and why the resulting string is also safe.

This needs work for documentation but I believe the direction should be good to go. #2 didn't need this extra documentation because it escaped thorugh @

The documentation was actually already there, just not visible in the patch.. it just still referred to SafeMarkup::set().

Looking at the complete actual function it's clear that everything has been at least admin-escaped: $expression is either IE, !IE or the safe value of $element['#browsers'], and $prefix/$suffix are the safe values of $element['#prefix'] / $element['#suffix'] themselves.

From manual testing I see before:

<!--[if lte IE 8]>
<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<![endif]-->

And after applying 2544262-20.patch I see:

<!--[if lte IE 8]>
<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<![endif]-->

So, these are exactly the same as expected.

Then, after modifying Drupal\Core\Asset\JsCollectionRenderer:render to be:

54	    // Defaults for each SCRIPT element.
55	    $element_defaults = array(
56	      '#prefix' => '<blink>test&</blink>',
57	      '#type' => 'html_tag',
58	      '#tag' => 'script',
59	      '#value' => '',
60	      '#suffix' => '<blink>test&</blink>',
61	    );

I see before:

<!--[if lte IE 8]>
test&amp;<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
test&amp;<![endif]-->

And after:

<!--[if lte IE 8]>
test&amp;<script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
test&amp;<![endif]-->

Which is expected.

And after changing it to:

55	    // Defaults for each SCRIPT element.
56	    $element_defaults = array(
57	      '#prefix' => SafeString::create('<blink>test&</blink>'),
58	      '#type' => 'html_tag',
59	      '#tag' => 'script',
60	      '#value' => '',
61	      '#suffix' => SafeString::create('<blink>test&</blink>'),
62	    );

I see before:

<!--[if lte IE 8]>
<blink>test&</blink><script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<blink>test&</blink><![endif]-->

And after I see:

<!--[if lte IE 8]>
<blink>test&</blink><script src="http://dev.d8.local/sites/default/files/js/js_IaU4wfZlByZ8at0yjyUe8AB03hw-jS1ZW5dZD4ep-wI.js"></script>
<blink>test&</blink><![endif]-->

Which I expect.

I also confirmed that all calls to SafeMarkup::set() are removed from the \Drupal\Core\Render\Element\HtmlTag::preRenderConditionalComments() method.

I reviewed the documentation on why SafeString::create() is still there and believe it is clear that $expression, $prefix, and $suffix are safe.

I believe it to meet the requirements of #2280965: [meta] Remove every SafeMarkup::set() call. Setting to RTBC.

for people reviewing this, note #2525908: HtmlTag render element's prefix and suffix can be marked safe when they are not changed elements to element.

+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -183,17 +184,17 @@ public static function preRenderConditionalComments($element) {
-    // Now calling SafeMarkup::set is safe, because we ensured the
-    // data coming in was at least admin escaped.
+    // We ensured earlier in this method that $expression, $prefix and $suffix
+    // were at least admin escaped, so now calling SafeString::create() is safe.

It's not "admin escaped" but "admin XSS filtered" and could maybe be massaged a bit more explicit to explain why the entire string is safe.

Also a note on the manual testing, since we changed from SafeMarkup::format() to SafeString::create() there should be no difference in the output and that test would really not test this specific issue but whether SafeString::create() == SafeMarkup::set() which very much should be the case (except for maybe passing in batch where the big list gets lost). Although good to test probably doesn't have much extra to say about this patch.

Updated the comment according to #24.

that interdiff looks like what was asked for. and #21 reviewed all the things. :) rtbc.

This IS nice. :) SafeString is good to use here because we definitely are internal to the render system and $prefix and $suffix have been admin filtered above if they are not marked safe.

Committed 7c4205a and pushed to 8.0.x. Thanks!