Rename SafeStringInterface to MarkupInterface and move related classes

SafeStringInterface is certainly now a bad name, given how it evolved over time. We need to make sure that MarkupInterface also matches the idea of EscapedString.
So do we consider the EscapedString the final markup representation of a string?

Is the disruption worth it? I think so - the more clarity this system has the better.

We can easily provide a BC layer here, so if needed, there is no disruption. On the other hand, quite some of those classes have been added since the last beta.

+1 to renaming this, SafeString is not a very descriptive name. Could we deprecate + trigger_error() rather than remove completely?

Given the input of t()/TranslatableString is markup as well, I don't see what's tricky about TranslatableString - it's exactly the same as FormattableString except it's translatable so its name should stay consistent. So probably either TranslatableString/FormattableString or TranslatableMarkup/FormattableMarkup? The only worry with the latter is that the docs specifically state to use no (or minimal) markup in the input string.

(Note we leave this in core and marked @internal because absolutely nothing should use this as it is a just mark this as not to be escaped by Twig object and unfit for use outside of the Renderer).

Can we prohibit access to this more strongly than just marking @internal? Ie fail if called from outside of core?

1. +++ b/core/includes/common.inc
   @@ -284,21 +284,21 @@ function format_size($size, $langcode = NULL) {
          case 'KB':
   -        return new TranslatableString('@size KB', $args, $options);
   +        return new TranslatableMarkup('@size KB', $args, $options);
          case 'MB':
   -        return new TranslatableString('@size MB', $args, $options);
   +        return new TranslatableMarkup('@size MB', $args, $options);
          case 'GB':
   

   TranslatableMarkup feels weird

2. +++ b/core/includes/errors.inc
   @@ -74,7 +74,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
   -      '@message' => SafeString::create(Xss::filterAdmin($message)),
   +      '@message' => Markup::create(Xss::filterAdmin($message)),
   

   At some point we could even have XssFilteredMarkup

+1000 for the concept. This HUGELY improves understandability, and thus DX.

it is feel to document that it should not be used and each module should create its own object if it needs to do this sort of thing but have a runtime check about were code is used feels very heavy handed.

I agree it's a slippery slope, but what's also dangerous is contrib doing Markup::create() on unsafe markup - so it may be worth checking that internal actually means internal here as I feel this needs stronger dissuasion than just an @internal annotation and there's no such thing as a @dangerous annotation. If not a runtime check (and knowing it's out of scope here), I'd also be happy with an assert() or that codesniffer check on contrib releases we discussed, with a warning on unsafe projects.

TranslatableMarkup feels weird

I thought the same thing but then embraced the weirdness... it actually clarifies that the input and output are markup - it's markup that can be translated to other languages.

Postponed #2569485: Add AttributeSafeStringInterface and UriAttributeSafeStringInterface, that should end up duplicate if this happens. fwiw I like this issue very much, the ambiguity of 'SafeString' is partly why the full scope of our problems wasn't realised until quite late, and this makes its purpose much more concrete.

1. +++ b/core/lib/Drupal/Component/Render/PlaceholderTrait.php
   @@ -2,10 +2,14 @@
   - * Contains \Drupal\Component\Utility\PlaceholderTrait.
   + * Contains \Drupal\Component\Render\PlaceholderTrait.
   
   +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -8,7 +8,7 @@
    namespace Drupal\Core\Annotation;
   

   This is confusing WRT \Drupal\Core\Render\PlaceholderGeneratorInterface + \Drupal\Core\Render\PlaceholderGenerator.

   They are vastly different kinds of placeholders, yet are in the same namespace, which suggests they're kinda the same thing.

   … except they're not in the same namespace: these live in Component, the ones I mentioned live in Core. Still, this is the most concerning part about this patch.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -187,7 +187,7 @@ protected function renderPlaceholder($placeholder, array $elements) {
   -    $elements['#markup'] = SafeString::create(str_replace($placeholder, $markup, $elements['#markup']));
   +    $elements['#markup'] = Markup::create(str_replace($placeholder, $markup, $elements['#markup']));
   
   @@ -292,7 +292,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -          $elements['#markup'] = SafeString::create($elements['#markup']);
   +          $elements['#markup'] = Markup::create($elements['#markup']);
   

   This makes so much sense that I basically don't have words for it. Except that it hurts. It hurts to see the old version and how painfully much better this is.

… except they're not in the same namespace: these live in Component, the ones I mentioned live in Core. Still, this is the most concerning part about this patch.

What about use MarkupPlaceholder and RenderPlaceholder?

This is confusing WRT \Drupal\Core\Render\PlaceholderGeneratorInterface + \Drupal\Core\Render\PlaceholderGenerator.

They are vastly different kinds of placeholders, yet are in the same namespace, which suggests they're kinda the same thing.

… except they're not in the same namespace: these live in Component, the ones I mentioned live in Core. Still, this is the most concerning part about this patch.

Maybe this can just be documented in the PHPdocs, these "placeholders" are so vastly different that I think it's fine.

Otherwise PlaceholderTrait could also just be renamed or moved to Component/Utility...

What about use MarkupPlaceholder and RenderPlaceholder?

That could also work

Overall direction here looks reasonable and I think is important to clarify what's happening.

Changing PlaceholderTrait seems much less important than the others, so I'd just leave it.

Agreed, just documenting they're different things should be fine

Isn't PlaceholderTrait actually TranslationPlaceholderTrait? It's specifically for @something, :something and %something. All of which are translation syntax thingies. It just happens to be that translations can also contain markup. But first and foremost it seems to be about translations.

Hmm that's correct but it may add to the confusion as these placeholders are used in both TranslatableString (t()) and FormattableString (SafeMarkup::format()) - the first is for translations, the second isn't but otherwise acts the same.

If we're going to do this, it needs to be done in the next ~5 days. Probably ideal to get it in before beta16 (tomorrow) if possible.

Agreed, just documenting they're different things should be fine

Seems to be a good solution for now. SafeStringInterface will be used that often that we better figure out a good name for that. The placeholder issue is comparable small against that.

For #2575615: Introduce EscapedString and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list, EscapedStringInterface/EscapedString becomes EscapedMarkupInterface/EscapedMarkup.

I realize this one isn't part of this issue, but "escaped markup interface" seems oxymoronical to me. If it's escaped, it's no longer markup. Asking here since it seems related to the reasoning about naming for this patch (all of which looks fine to me FWIW).

I've thought about it a bit and I agree with not retaining two of everything for 8.0.0 -- the goal is to make the API less confusing, but leaving in all those deprecated wrappers would be more confusing.

I was at first concerned about dropping all these sudden changes on everyone a week before RC with no warning whatsoever. However, actually, most of the things listed here have just been added in the past two weeks I think, aside from SafeStringInterface and its implementations, which were always marked internal anyway. Is that a correct assessment? Did I miss anything?

Also, answers to #38 would be good beta eval material for the IS. :)

I haven't read the full patch or the issue comments yet, so this is just a drive-by question. Apologies if it was already discussed and answered:

In the issue summary:

SafeStringInterface is badly named for two reasons:

1) The output is not necessarily safe

In the patch:

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -65,7 +67,7 @@ class SafeMarkup {
   public static function isSafe($string, $strategy = 'html') {
     // Do the instanceof checks first to save unnecessarily casting the object
     // to a string.
-    return $string instanceOf SafeStringInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
+    return $string instanceOf MarkupInterface || isset(static::$safeStrings[(string) $string][$strategy]) ||
       isset(static::$safeStrings[(string) $string]['all']);
   }

I'm not quite clear how to interpret this. Is MarkupInterface a better name partially because it doesn't claim to be "safe"? But then why is isSafe() only checking MarkupInterface and not something more strictly named (e.g., a SafeMarkupInterface that extends MarkupInterface)?

It's not clear from the summary whether PlaceholderTrait is or isn't part of this patch. It looks like it is in the current patch.

That name seems less unambiguous not as obviously a good choice for me. I'd suggest something FormattableStringPlaceholderTrait... since all translatable strings are also formatted strings? But I'd suggest spinning it off into its own issue to get the other ones in maybe. #2577785: Remove PlaceholderTrait seems to only be about docs and I don't think documentation solves the confusion (which is what caused us all these SafeFoo problems in the first place, of course).

isSafe() I think we are only retaining for BC reasons, thence the inconsistency in naming. But I do see how there could be confusion between the differing intents of AnyMarkupYouLikeInterface and SufficientlySanitizedMarkupInterface. MarkupThatYourSiteWillPrintJustLikeThisInterface. MarkupThatMustNotContainOtherwiseUnsanitizedUserInputInterface. Or in other words, I do see/share the concern about the name MarkupInterface not warning people away or clarifying what the implications of implementing the interface are, whereas with SafeStringInterface, it was less clear for consumers, but more clear for people actually constructing one, that there was some expectation about what should go in there in the first place.

I guess this partly depends if we are keeping the @internal on the MarkupInterface. @alexpott said something about needing to remove it (the patch does not, which is good as that would be scope creep). But to me that increases the risk as well.

Another concern, I might expect MarkupInterface to be the object equivalent of #markup in render arrays. But it's not, at all.

I think the current patch is mostly fine as is. The concerns can mostly be documented (and MarkupInterface is still @internal) so if we mention clearly in the code that the placeholders in PlaceholderTrait and PlaceholderGenerator are different things, we only have a problem if people use PlaceholderTrait without reading the docs.

But realistically it will only be used in FormattableString and TranslatableString anyway, which brings me to...

I'd suggest something FormattableStringPlaceholderTrait... since all translatable strings are also formatted strings?

Technically not yet... But if we codify that, that may work? Could we maybe get rid of PlaceholderTrait, move its code into FormattableString, and make TranslatableString extend FormattableString?

Oh and despite how silly it sounds I actually quite like SufficientlySanitizedMarkupInterface...

To elaborate on #40, my first thought when seeing MarkupInterface was yay, in a follow-up we'll be able to make Vocabulary::getDescription() return an object that implements that as a way of informing callers that what's returned is HTML and not plain-text. But, with the current patch treating MarkupInterface as a signal that the markup is also safe, we couldn't do that.

Additionally, as #38 points out, EscapedMarkup would not be accurate, since the result is not markup, it's only HTML-encoded text.

Therefore, here's some possible suggestions to think about, all open to modifying or shooting down:

• Instead of MarkupInterface, have a HtmlStringInterface.
• Add a separate SafeHtmlStringInterface or add isSafe() to HtmlStringInterface.
• Rename #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list to HtmlEscapedText.
• Either keep FilteredMarkup, TranslatableMarkup, etc. or rename them to FilteredHtml, TranslatableHtml, etc.
• Maybe have a concrete generic class that implements HtmlStringInterface, but do not have a concrete generic class that implements SafeHtmlStringInterface (as SafeString does in HEAD and Markup does in the current patch). Instead, create a RenderedMarkup or RenderedHtml class for the Renderer to use for its output.

Re #47 - I agree that having a separate object signaling that something is an HTML fragment could be nice, but not sure about some of the other remarks:

1. For Vocabulary::getDescription we could still make it an object with another interface to signal it contains HTML markup that doesn't implement MarkupInterface (or whatever we call the interface that signals safeness)
2. We can either be correct or be brief - if we want to be brief I still think Markup is fine and we can document where we're not 100% correct. It might mean there's a bit of a learning curve but is that really such a problem?
3. If we want to be 100% correct, EscapedMarkup may technically not be "markup", but it is not plain-text either. In isolation, its contents are technically not "markup", but they will almost always be contained within markup - and again these are all things that can be documented.

   So I think EscapedMarkup is fine, as the result is escaped HTML markup, i.e. HTML markup where the '"&<> are turned into HTML entities. Technically the result would be "HTML text containing HTML-escaped HTML markup".

4. As to the suggested HtmlEscapedText, it still implies plain-text, which is a confusion from D7 we want to avoid, so maybe HtmlEscapedHtmlText?
5. For the "safe" HTML string I do think we should come up with a better name than SanitizedAnything/SafeAnything and ::isSafe() as it's not necessarily safe (or fully sanitized), and here misnaming things actually poses a security risk - when rendered to string a SafeString object can still contain unsanitized HTML and it's only "safe" for use within HTML fragments and not within other contexts such as JSON, HTML attributes or CSS/Javascript embedded into HTML.
6. HTMLStringInterface is not really more correct than MarkupInterface either, as it only refers to HTML text or a series of HTML nodes, and excludes other contexts within an HTML document (such as anything between the "<" and ">" of an HTML tag)

This is maybe a different angle from what has been discussed so far.
For me HTML, given its name, consists of markup. Whether its actual tags or whether escaped strings, its all markup. So what represents a MarkupInterface for me is an object which will be printed to the resulting HTML just as it is, without any additional massaging on the outside (internal details don't matter). An EscapedMarkup will be a string which will be printed in an escaped way to the final HTML markup, a Markup object will just print out its value to the final HTML.

Sometimes we just assume that people are stupid and by that cause so much confusion that at the end there is more harm caused.

#50 has good reasons to stick to Markup.

I'm not sure the name is perfect, but it's 1000 times better than SafeString.

+1 for Markup

Given the very convincing explanation in #50 and the fact that my sole other concern (PlaceholderTrait being confusing, see #22.1) is being addressed in #257775 and will remove that trait, I think this is actually ready now :)

We've had a good, valuable, and IMHO important discussion here, and we are concluding that what the patch already does, is the sensible thing to do.

Hence: RTBC.

Whee!

I can give the reroll a try in ~15min... unless @alexpott was already working on this?

Reviewed the interdiff with patch I had in my own testing issue and didn't see anything odd - just replacements about recent changes and more SafeString -> Markup conversions. SafeStringNormalizer hasn't been renamed here but that may be followup material.

The interdiff is not really more informative than the patch itself so let's just try to get this green for now...

The SafeStringInterface -> MarkupInterface.php conversion was not done as git mv though, it would be nice if we didn't lose the version info for git blame and for reviewability

Reuploading #57 without the bit about the drupal-8.language-enabled.php binary file just to see what the testbot thinks

As to the git mv, that didn't work for me either, I guess git decides it is a new file when it's too different despite logging a rename?

Maybe -rename-threshold would help?

Try git diff -M10%.

I found only a single nitpick. Once this is green, this is ready IMHO.

1. +++ b/core/lib/Drupal/Component/Render/MarkupInterface.php
   @@ -0,0 +1,45 @@
   +/**
   + * Marks an object's __toString() method as returning markup.
   + *
   + * Objects that implement this interface will not be automatically XSS filtered
   + * by the render system or automatically escaped by the theme engine.
   

   Super clear! :)

2. +++ b/core/lib/Drupal/Component/Render/MarkupInterface.php
   @@ -0,0 +1,45 @@
   + * pipeline in order to render JSON and to fast render fields. By contrast,
   

   "to fast render fields" sounds wrong.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -292,7 +292,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -          $elements['#markup'] = SafeString::create($elements['#markup']);
   +          $elements['#markup'] = Markup::create($elements['#markup']);
   

   <3

4. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -27,7 +27,7 @@
   -   * @return \Drupal\Component\Utility\SafeStringInterface
   +   * @return \Drupal\Component\Render\MarkupInterface
       *   The rendered HTML.
   

   <3

5. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -404,7 +404,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   -    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf SafeStringInterface)) {
   +    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf MarkupInterface)) {
   

   <3

6. +++ b/core/modules/filter/filter.module
   @@ -286,7 +286,7 @@ function filter_fallback_format() {
   - * @return \Drupal\Component\Utility\SafeStringInterface
   + * @return \Drupal\Component\Render\MarkupInterface
     *   The filtered text.
   

   <3

7. +++ b/core/themes/engines/twig/twig.engine
   @@ -46,7 +46,7 @@ function twig_init(Extension $theme) {
   - * @return string|\Drupal\Component\Utility\SafeStringInterface
   + * @return string|\Drupal\Component\Render\MarkupInterface
     *   The output generated by the template, plus any debug information.
   

   <3

Agree about #64.2 but that was also already on the previous docs (which is hard to see precisely because git doesn't consider it a rename :), so maybe let's not fix that here

+++ b/core/modules/filter/filter.module
@@ -286,7 +286,7 @@ function filter_fallback_format() {
- * @return \Drupal\Component\Utility\SafeStringInterface
+ * @return \Drupal\Component\Render\MarkupInterface
  *   The filtered text.

Would be even better if this said 'The formatted text'.

Note: @alexpott didn't mention in #57 but I helped write some of those docs. :P

Carry on. :)

#66: Let's rename filter.module to text_format.module in D9? :P

Checking the credit box for @xjm per #67. Possibly other participants on this issue need to get that box checked too, but I'll leave that for whoever commits this.

+++ b/core/lib/Drupal/Component/Render/MarkupInterface.php
@@ -0,0 +1,45 @@
+ * @see \Drupal\Core\Template\TwigExtension::escapeFilter()
...
+ * @see \Drupal\Core\StringTranslation\TranslatableMarkup
+ * @see \Drupal\views\Render\ViewsRenderPipelineMarkup

Why does that not fail the component test?
I'd strongly recommend that the component test removes its strictness for comments, but people didn't liked that when I tried to change that

@dawehner: I think the test already excludes "@see" lines.

Alright, I think we will the change record updates later.

Gave this another look and very happy with how this looks, much clearer.

As to the change record updates, @alexpott said in #37 he'd do those once this issue was in.

RTBC++

#78 looks great to me. I'll commit it once DrupalCI says it passes tests. Unless someone knocks it back before then.

Pushed to 8.0.x! "Needs work" for the CR updates.

I used git apply --index of the #78 patch in an attempt to preserve history. When I do a git log --follow, I see the retained history for most of the files, but not for MarkupInterface and FilteredMarkup, despite the #78 patch showing those as renames. I'm not clear on whether that history is lost forever, or still available with some magic option passed to git log. http://stackoverflow.com/questions/14832963/how-can-i-control-the-rename... claims the latter, but I don't know if that's accurate.

@alexpott informed me that git log -M10% --follow core/lib/Drupal/Component/Render/MarkupInterface.php shows history. Yay!