Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString

This is suitably novice.

Removed @internal tag.

I spoke with @alexpott about this issue. I don't think this is a good idea. SafeStringInterface should be treated as internal for the same reason SafeMarkup::set() is: it declares an arbitrary string as safe, rather than ensuring that the string is safe. While it's true that it has fewer side effects than SafeMarkup::set(), it still allows a string to skip sanitization, circumventing Twig autoescape and/or the markup filtering in the renderer, and is therefore not a good pattern for modules to use.

That doesn't mean I know what to do about #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender though. :P

What about making that more clear by renaming SafeString::create() to SafeString::createFromSafeString()
(I agree that is super confusing but what about something similar?)

Views is intercepting the render system and doing it's own funky stuff. Therefore it needs to preserve safeness whilst it does this.

also applies to #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module.

It's 1) a filter (which means the output of the filter is going to be assumed to be safe anyway), 2) using the render system as part of its filtering (because it wants to replace a wrap a certain piece of markup in a themeable piece of markup, i.e. it needs the render/theme system).

See #2547741-22: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module for more details about why this is ok.

In the particular case of #2547741, it could be argued that we need something like FilteredString for those filters that want/need to use the render/theme system (otherwise Twig would auto-escape my already-filtered string and… that'd break things). But then the question is: is it really worth having another thing that is very much like SafeString? I think semantically speaking, the answer is yes. But then the discoverability of that becomes a problem. Although there we could argue that it should just be explicitly documented in the filter system docs, for example If your filter uses Twig to render something, then you need to make sure that it receives all its input as FilterString instances. — or something like that.

Similarly, I think (this is based on a very quick skim of #2501931, I could be wrong here) Views could have a ViewsRenderPipelineSafeString implements SafeStringInterface value object. Because that's the real reason that Views needs to deal with this at all: because it has its own render pipeline. (For good reasons, render arrays are only for HTML, Views also wants to generate JSON etc.)

#12: sounds good (@internal to the module, plus documenting on SafeStringInterface).

I agree with #11., let's make doing the right thing easy and make it harder if you are doing something more complex.
This would then basically mean that we mark the issue as won't fix, right and create subissues for the various subsystems with the requirement of such an object.

Wouldn't it be better to do it here, where we already have the entire discussion/reasoning that led to this?

+++ b/core/modules/views/src/Render/ViewsRenderPipelineSafeString.php
@@ -0,0 +1,26 @@
+class ViewsRenderPipelineSafeString extends SafeString {
+}

I would have made SafeString a trait and use it here

IMO this looks great. Just some nits below.

I just have one bigger remark: I'm not sure I entirely like the "interrupt the render pipeline" explanation. I don't think Views "interrupts" the render pipeline, nor does Filter. I think both have their own render pipeline, but for integration purposes, they happen to choose to also use the render/theme system. Which means data is flowing from their own render pipeline into the regular render pipeline, and because they do that, they need to deal with this, otherwise the input they send into the regular render pipeline would be auto-escaped.

I guess "interrupt" is a very terse way of saying regular render pipeline -> CUSTOM RENDER PIPELINE ( -> reuses regular render pipeline for bits and pieces) -> regular render pipeline — because in the end, the output of Filter and (some of) the output of Views make it back into the regular render pipeline. But that's kind of a detail here; what matters most is that they reuse the regular render pipeline.

(Which is why the only problematic case in Filter is FilterCaption, which is the only filter calling the render/theme system.)

1. +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
   @@ -16,8 +16,11 @@
   + *   implements a custom render pipeline for fast rendering of fields. All
   
   +++ b/core/lib/Drupal/Core/Render/SafeString.php
   @@ -19,8 +19,11 @@
   + *   pipeline for fast rendering of fields. All objects that extend this object
   

   "for fast rendering of fields" -> I don't think this is the main reason, I think the main reason is that Views also wants to render JSON etc.

   I'll leave this to @dawehner.

2. +++ b/core/modules/views/src/Render/ViewsRenderPipelineSafeString.php
   @@ -0,0 +1,26 @@
   + *   This object is marked as internal because it should only be used during
   

   s/during/in/

   ?

1. index 3729027..2e2bb66 100644
   --- a/core/lib/Drupal/Component/Utility/SafeStringInterface.php
   
   --- a/core/lib/Drupal/Component/Utility/SafeStringInterface.php
   +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
   

   Should we point to the trait from the Interface, maybe an @see ?

2. +++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
   @@ -10,14 +10,23 @@
     *
   + * All objects that implement this interface marked @internal.
   

   This is a odd line ...

+++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
@@ -708,7 +708,7 @@ protected function renderFields(array $result) {
-              $rendered_field = SafeString::create(str_replace($placeholders, $values, $rendered_field));
+              $rendered_field = ViewsRenderPipelineSafeString::create(str_replace($placeholders, $values, $rendered_field));

This totally makes sense, because it's replacing Views' "post-render tokens", which are most definitely a Views render pipeline-specific concept.

RTBC.

+++ b/core/lib/Drupal/Component/Utility/SafeStringInterface.php
@@ -10,17 +10,23 @@
+ *   regular render pipeline internally and passes processed data
+ *   into it. For example, Views implements a custom render pipeline in order to

Nit, can be fixed on commit: "into it. For" still fits on the previous line.

Cross-posted.

So I feel like the views render pipeline usage is something that we could eventually at some point factor out -we've already done that for a lot of Views's custom rendering. Seems odd to add the trait and docs for one very unique use case.

However in irc alexpott pointed me to FilterCaption, which while not changed in this patch yet shows there's two use-cases in core. Issue for that is #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module.

Given two use cases, and one of them being a filter which is not an uncommon case, this makes sense then after all.

Committed/pushed to 8.0.x, thanks!

I think this patch introduces a bug related to result groups. Created #2553035: Views grouping in results is broken because of SafeString as follow-up.