Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module

I think this is what you wanted to do? :)

Can you reupload the patch please?

#5 Re-roll

+++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
@@ -59,7 +60,7 @@ public function process($text, $langcode) {
+          '#node' => SafeString::create($node->C14n()),

C14N()

Otherwise this looks to be OK, this also already has test coverage through FilterUnitTest

Fixed typo in #12, thanks.

#15: yes, thanks :)

#15: ehm, no, there are other things still using methods on SafeMarkup.

Re-uploading #14 and RTBC'ing it.

+++ b/core/modules/filter/src/Plugin/Filter/FilterCaption.php
@@ -59,7 +60,7 @@ public function process($text, $langcode) {
+          '#node' => SafeString::create($node->C14N()),

So SafeString::create() has some of the same issues that SafeMarkup::set() has since it can easily introduce XSS if used incorrectly. However it is way better because it does not have side effects. I think we should tackle #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString first. And document the beejeebus out of when and where to use it because I can't see another way of doing this sanely.

Let's postpone on that issue and add a comment explaining why we have to use the unsanitized canonicalized node here?

Why: because this is a text format filter, and after filtering, we always assume the output is safe. We have to. See #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), which added this to \Drupal\filter\Element\ProcessedText::preRenderText():

    // Filtering and sanitizing have been done in
    // \Drupal\filter\Plugin\FilterInterface. $text is not guaranteed to be
    // safe, but it has been passed through the filter system and checked with
    // a text format, so it must be printed as is. (See the note about security
    // in the method documentation above.)
    $element['#markup'] = SafeMarkup::set($text);

This is simply part of that.

Heh, I meant a code comment :)

I misread "document the beejeebus out of when and where to use it" as "document the beejeebus out of whenever and wherever we use it", but actually the latter is not a bad idea either...

per #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString

I think it'd be a good idea to also change

$element['#markup'] = SafeMarkup::set($text);

to

$element['#markup'] = FilteredString::create($text);

in \Drupal\filter\Element\ProcessedText::preRenderText().

Other than that, I have no remarks :)

I was about to open another issue for that but may as well add that in here :)

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -124,7 +124,7 @@ public static function preRenderText($element) {
+    $element['#markup'] = FilteredString($text);

oops

#29: heh, indeed. I also missed it.

Looks like we need one more string cast or equality instead of identicalness assertion.

Actually the problem is with Views. When we go to test/serialize/node-field?_format=json, in Drupal\views\Plugin\views\field\FieldPluginBase::advancedRender() we call ::renderText(), where we append an empty string to $value SafeString so it becomes unsafe again and gets escaped later on.

Maybe just this workaround to make tests pass and we address the bug in a followup?

I think the problem you're running into will be fixed by #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender

@alexpott #30 works when using the patch in that issue so let's postpone this on that issue being committed.

We could then re-RTBC #30 if it's green on re-testing.

No it's fine, I queued the wrong patch -- it's #30 (which is green now) we want to commit , not #36.

Oh, okay. Could you reupload #30 then, to avoid confusion?

Re-uploaded #30

This will need a retest.

So… #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender didn't fix it after all?

I think it did, because 30 is green

So we had a timing issue with reverting #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender I think we're good to here.

Then we're good to go here :)

Another one bites the dust. Committed 531d0a4 and pushed to 8.0.x. Thanks!