Remove SafeMarkup::checkPlain() for Drupal 9.0.x

The patch looks good. So long SafeMarkup::checkPlain() \o/

Sorry I didn't see the comments on the previous issue. After reading it again I agree that it could be more clear and the wording could be changed to be better. Maybe if there is specific comments about there, we could copy the feedback here.

1. +++ b/core/includes/form.inc
   @@ -574,10 +574,13 @@ function template_preprocess_form_element_label(&$variables) {
   - * returns any user input in the 'results' or 'message' keys of $context, it
   - * must also sanitize them first.
   + * returns any user input in the 'message' key of $context, it must also
   + * sanitize it first. The 'result' key is not displayed to the user and
   + * therefore it is responsibility of the batch callbacks to handle sanitization
   + * if required. If the 'result' key is being themed using item_list the callback
   + * can rely on Twig auto-escape.
   

   Is there a 'result' key or is this 'results'?

   Can we get away with not sanitizing it first?

   Yes this sounds a bit contradictory. It's using item list but yet still needs to be manually escaped by the callback. Is this a JS issue maybe?

2. +++ b/core/includes/form.inc
   @@ -621,10 +627,13 @@ function template_preprocess_form_element_label(&$variables) {
   - *     $context['results'][] = $row->id . ' : ' . SafeMarkup::checkPlain($row->title);
   + *     // This can be used by the batch finished callback. If this is displayed to
   + *     // the user via the item list theme then it will be auto-escaped.
   + *     $context['results'][] = $row->id . ' : ' . $row->title;
   

   This sounds like if the title was using t('@place') in the title then it would double escape by the auto-escape.

   Also nit: 80 chars

3. +++ b/core/includes/form.inc
   @@ -621,10 +627,13 @@ function template_preprocess_form_element_label(&$variables) {
   - *     $context['message'] = SafeMarkup::checkPlain($row->title);
   + *     // This is used in JavaScript and should be escaped.
   + *     $context['message'] = Html::escape($row->title);
   

   Why isn't it being auto-escaped automatically like the 'results'?

4. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -15,9 +15,13 @@
   + * - Strings sanitized by self::format() unless a !placeholder argument is not
   + *   already marked safe.
   

   Just a note: !placeholder may not be around for long but for now this is safe to say.

Some further feedback from @Wimleers:
#2545972-66: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping
#2545972-77: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

and from @xjm #2545972-115: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

and clarification on this followup by @alexpott #2545972-183: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

I'll try to consolidate and copy the relevant notes here but they all relate to form.inc

Discussing the batch API with @alexpott IRL. We may be able to check safe variables and escape "just before" sending to the JavaScript. Ideally we would do this in JavaScript but this may be a nice DX compromise and would be slightly easier if we get an idea on how to deal with safe strings in JS in the future.

I decided to not touch yet on sanitization part of code as I'm not familiar enough with batch API, but I worked on following comments in #12:

Is there a 'result' key or is this 'results'?

Confirmed that and changed

Can we get away with not sanitizing it first?

Took off the whole sentence

Also nit: 80 chars

Nailed it

Just a note: !placeholder may not be around for long but for now this is safe to say.

I couldn't find better rephrasing for this, so I decided to leave it as is for now

Going to take a stab at moving this forward.

Just posting that this issue will also block this issues current solution: #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list but its blocked by other issue too.

Reroll of the latest patch.

1. +++ b/core/includes/form.inc
   @@ -601,8 +602,11 @@ function template_preprocess_form_element_label(&$variables) {
   - *   $context['results'][] = $node->id() . ' : ' . SafeMarkup::checkPlain($node->label());
   - *   $context['message'] = SafeMarkup::checkPlain($node->label());
   + *   // This can be used by the batch finished callback. If this is displayed to
   + *   // the user via the item list theme then it will be auto-escaped.
   + *   $context['results'][] = $node->id() . ' : ' . $node->label();
   + *   // This is used in JavaScript and should be escaped.
   + *   $context['message'] = Html::escape($node->label());
   
   +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -130,36 +137,6 @@ public static function getAll() {
   -    static::$safeStrings[$string]['html'] = TRUE;
   

   So discussed briefly with @alexpott over the weekend and to avoid explaining what the finished batch API callback should/shouldn't do to variables we should just check the SafeStringInterface and escape if not safe otherwise just before it's sent to JavaScript, similar to what twig autoescape does.

   Ultimately JS wouldn't know of SafeStringInterface and escape it... but I think this is the best solution.

2. +++ b/core/includes/form.inc
   @@ -621,10 +625,13 @@ function template_preprocess_form_element_label(&$variables) {
   + *     // This is used in JavaScript and should be escaped.
   + *     $context['message'] = Html::escape($row->title);
   

   Same, avoid this and check and escape if unsafe.

#23 is the same as #15

Trying to address #23 :)

Here's what I was proposing, hopefully it does the trick.

This should fix the tests

Tested patch from #29. No issues found. RTBC?

Created #2573913: Create SafeMarkup::escapeIfUnsafe() out from #30.

Rerolling the patch, added some documentation and working on the test coverage now.

+++ b/core/includes/batch.inc
@@ -315,6 +341,15 @@ function _batch_process() {
+    // escaped accidentally, which would be more complex than escaping plain text

80 chars. The patch looks good but I don't think Im really qualified to RTBC this so leaving to someone else..

1. +++ b/core/includes/batch.inc
   @@ -102,6 +103,17 @@ function _batch_do() {
   +  // the javascript layer would be more complex, because we would need to
   

   s/javascript/JavaScript/

2. +++ b/core/includes/batch.inc
   @@ -102,6 +103,17 @@ function _batch_do() {
   +  if (!SafeMarkup::isSafe($message)) {
   +    $message = Html::escape($message);
   +  }
   +  if (!SafeMarkup::isSafe($label)) {
   +    $label = Html::escape($label);
   +  }
   

   +1

3. +++ b/core/includes/batch.inc
   @@ -167,10 +179,24 @@ function _batch_progress_page() {
   +  // the javascript layer would be more complex, because we would need to
   

   s/javascript/JavasScript/

4. +++ b/core/includes/batch.inc
   @@ -315,6 +341,15 @@ function _batch_process() {
   +    // the javascript layer would be more complex, because we would need to
   

   s/javascript/JavaScript/

5. +++ b/core/includes/form.inc
   @@ -571,14 +572,6 @@ function template_preprocess_form_element_label(&$variables) {
   - * Note: if the batch 'title', 'init_message', 'progress_message', or
   - * 'error_message' could contain any user input, it is the responsibility of
   - * the code calling batch_set() to sanitize them first with a function like
   - * \Drupal\Component\Utility\SafeMarkup::checkPlain() or
   - * \Drupal\Component\Utility\Xss::filter(). Furthermore, if the batch operation
   - * returns any user input in the 'results' or 'message' keys of $context, it
   - * must also sanitize them first.
   - *
   

   Yay!

6. +++ b/core/includes/form.inc
   @@ -749,7 +744,7 @@ function batch_set($batch_definition) {
   +    $batch_set['init_message'] = new FormattableString('@message<br>&nbsp;', ['@message' => $batch_set['init_message']]);;
   

   We want the mesage to be always escaped, right?

7. +++ b/core/includes/form.inc
   @@ -749,7 +744,7 @@ function batch_set($batch_definition) {
    
   

   s:<br>:<br />:

8. +++ b/core/modules/system/src/Tests/Batch/PageTest.php
   @@ -68,7 +68,7 @@ function testBatchProgressMessages() {
   +    $this->assertRaw('<div class="progress__description">Initializing.<br>&nbsp;</div>', 'Initial progress message appears correctly.');
   

   s:<br>:<br />:

9. +++ b/core/includes/form.inc
   @@ -601,8 +594,8 @@ function template_preprocess_form_element_label(&$variables) {
   - *   $context['results'][] = $node->id() . ' : ' . SafeMarkup::checkPlain($node->label());
   - *   $context['message'] = SafeMarkup::checkPlain($node->label());
   + *   $context['results'][] = $node->id() . ' : ' . $node->label();
   + *   $context['message'] = $node->label();
   

   +1, should we also document the escaping behavior here?

Thank you for all your reviews!

We want the mesage to be always escaped, right?

Well it will be, through the usage of the "@" placeholder.

+1, should we also document the escaping behavior here?

Did earlier.

Upload the current batch.

1. +++ b/core/includes/form.inc
   @@ -572,6 +572,12 @@ function template_preprocess_form_element_label(&$variables) {
   + * Note: The batch 'title', 'init_message', 'progress_message', 'error_message'
   + * and the keys 'results' and 'message' in $context might contain any user
   + * input. If the passed in value is not a safe string (like the result of a
   + * valid t() call), then the value will be escaped and displayed as such by the
   + * batch JavaScript.
   

   How about "Note: The following elements might contain user input:
   - $batch['title']
   - $batch['init_message']
   - $batch['progress_message']
   - $batch['error_message']
   - $context['results']
   - $context['messaeg']
   If the passed in value is not a SafeStringInterface object (such as an object returned from a valid t() call), then this value will be escaped an the batch JavaScript will display it as escaped as well."

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -130,6 +130,36 @@ public static function getAll() {
   +  public static function checkPlain($text) {
   

   :(

3. +++ b/core/modules/system/src/Tests/Batch/PageTest.php
   @@ -76,4 +78,17 @@ function testBatchProgressMessages() {
   +    $this->drupalGet('batch-test/test-title');
   ...
   +    $this->drupalGet($url->__toString());
   

   Do we want to assert anything here or merely check that the drupalGet is successful?

@alexpott: Does that also count for reviewing?

Can this issue be closed?

@alexpott, @stefan.r ok for me.

Duplicate of #2579701: remove SafeMarkup class.