SafeMarkup does not escape some filter tips - remove SafeMarkup usage from FilterHtml

This is a duplicate of #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise. Closing this one because the other issue has the complete fix.

This issue now is a blocker for #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise

From @joelpittet missing some commas.

Reviewing at MWDS

Manually tested: both the "short" and "long" filter tips are rendered identically in HTML (and therefore appear visually identical). I would RTBC based on that, but would like some outside confirmation that

The approach taken mitigates all security concerns by falling back to the admin filter

The approach taken mitigates all security concerns by falling back to the admin filter

is implicit by #markup render array using Xss::filterAdmin if the markup is not marked safe.

@akalata will add screenshots.

It'd be great to get #2550945: Add Html::escape() in first because we've now found a good reason why we want Html::encodeEntities() - PHP versions add new features that we might want to make use of.

Adding screenshots from my manual testing.

Yeah, now that #2550945: Add Html::escape() has consensus, let's update this to use that method. Thanks!

Replaced htmlspeciachars() with Html::escape().

1. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -144,8 +144,12 @@ public function tips($long = FALSE) {
   +          array('data' => ['#prefix' => '<code>', '#markup' => Html::escape($tips[$tag][1]), '#suffix' => '

   '], 'class' => array('type')),
   

   Nice, you are so used to [] now, you just cannot use it anymore :)

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -175,8 +179,12 @@ public function tips($long = FALSE) {
   +        array('data' => ['#prefix' => '<code>', '#markup' => Html::escape($entity[1]), '#suffix' => '

   '], 'class' => array('type')),

   +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -368,12 +368,15 @@ function testFilterTipHtmlEscape() {
   + $link = '' . htmlspecialchars($site_name_with_markup, ENT_QUOTES, 'UTF-8') . '';
   ...
   + $link_as_code = '' . htmlspecialchars($link, ENT_QUOTES, 'UTF-8') . '';
   + $ampersand_as_code = '' . htmlspecialchars($ampersand, ENT_QUOTES, 'UTF-8') . '';
   

   Should we use Html::escape() here as well?

1. Fixed - we shouldn't be mixing formats.
2. Yep - nice spot :)
That'll teach me for going from postponed to rtbc.

Fixed the syntax error

filtered out the other htmlspecialchars in that test.

Reviewing

@Dorf found that we had still SafeMarkup being used in the test, instead of opening another issue for that, I think we can remove it here too.

+++ b/core/modules/filter/src/Tests/FilterAdminTest.php
@@ -312,7 +312,7 @@ function testFilterAdmin() {
-    $this->assertText(SafeMarkup::checkPlain($text), 'The "Plain text" text format escapes all HTML tags.');
+    $this->assertText(Html::escape($text), 'The "Plain text" text format escapes all HTML tags.');

$this->assertEscaped()?

Applied cleanly and found no issues on filter/tips or on restricted html. Screencaps of head and patched code and UI.

Good call @alexpott, re #23

This conflicts with #2547851: SafeMarkup::format() should require arguments without them it is just SafeMarkup::set() in disguise Whichever gets in first I'll re-roll the other one.

Reviewing

Looks good. Ran the test just to be sure and it came back with 232 passes!

Good job, @joelpittet!

Random testbot failure. on \Drupal\system\Tests\Theme\FastTest