See this announcement: SafeMarkup::checkPlain() is now deprecated and will be removed, so we should adapt our code before that. (I don't think we're using SafeMarkup::set() anywhere, but we should of course check for that, too.)

CommentFileSizeAuthor
#7 2562689-7--remove_deprecated_safe_markup_method_calls--interdiff.txt4.65 KBdrunken monkey
#7 2562689-7--remove_deprecated_safe_markup_method_calls.patch17.85 KBdrunken monkey
#6 search-api-remove-safe-markup-check-plain-2562689-5.patch17.85 KBrashid_786
#3 search-api-remove-safe-markup-check-plain-2562689-3.patch14.45 KBrashid_786
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

rashid_786’s picture

rashid_786 CreditAttribution: rashid_786 as a volunteer commented

Found SafeMarkup::checkPlain() in lots of files. As per recommendation we should replace it with Html::escape().

rashid_786’s picture

rashid_786 CreditAttribution: rashid_786 as a volunteer commented
Status: Active » Needs review
FileSize
search-api-remove-safe-markup-check-plain-2562689-3.patch14.45 KB

Status: Needs review » Needs work

The last submitted patch, 3: search-api-remove-safe-markup-check-plain-2562689-3.patch, failed testing.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

use statement is missing.

rashid_786’s picture

rashid_786 CreditAttribution: rashid_786 as a volunteer commented
Status: Needs work » Needs review
FileSize
search-api-remove-safe-markup-check-plain-2562689-5.patch17.85 KB

Thanks @joshi.rohit100 for pointing. Patch updated with new changes.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
FileSize
2562689-7--remove_deprecated_safe_markup_method_calls.patch17.85 KB
2562689-7--remove_deprecated_safe_markup_method_calls--interdiff.txt4.65 KB

Thanks a lot for working on this, looks quite good already!

A few things could be simplified, though, and there's one tricky part I saw:

  1. +++ b/search_api_db/src/Plugin/search_api/backend/Database.php
@@ -271,7 +272,7 @@ class Database extends BackendPluginBase {
+          '#markup' => Html::escape(str_replace(':', ' > ', $this->configuration['database'])),

    This could now probably just use #plain_text. 

  2. +++ b/search_api_db/src/Plugin/search_api/backend/Database.php
@@ -315,7 +316,7 @@ class Database extends BackendPluginBase {
-      'info' => SafeMarkup::checkPlain(str_replace(':', ' > ', $this->configuration['database'])),
+      'info' => Html::escape(str_replace(':', ' > ', $this->configuration['database'])),

    This seems to lead to double-escaping, since the information will be included in a table – or, would be included, since it seems we had the wrong method name. Weird no-one noticed before.

    Anyways, seems the call can just be dropped. 

  3. +++ b/src/Controller/IndexController.php
@@ -51,7 +52,7 @@ class IndexController extends ControllerBase {
-    return SafeMarkup::checkPlain($search_api_index->label());
+    return Html::escape($search_api_index->label());

    This doesn't seem to work that way, or any other simple way – see #2563381: Hard to get page title and header both properly sanitized.

Revised patch attached, if no-one objects I'll commit it in a few days.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs review » Fixed

Committed.
Thanks again everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.