Ensure that SafeString objects can be used in non-HTML contexts

+1 for "deprecating" the option this way.

It's interesting that the patch is green when my Views patch has a failure though -- didn't figure out how that happened yet. :)

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -209,38 +209,28 @@ public static function checkPlain($text) {
-   *   - !variable: Inserted as is, with no sanitization or formatting. Only
-   *     use this when the resulting string is being generated for one of:
-   *     - Non-HTML usage, such as a plain-text email.
-   *     - Non-direct HTML output, such as a plain-text variable that will be
-   *       printed as an HTML attribute value and therefore formatted with
-   *       self::checkPlain() as part of that.
-   *     - Some other special reason for suppressing sanitization.

I think we need to keep documentation of this indicating it is a legacy placeholder type that now behaves the same as @ but is deprecated in 8.0.x and that support for it will be removed before 9.0.0.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -176,6 +171,26 @@ protected function doTranslate($string, array $options = array()) {
+    $html = isset($options['html']) ? $options['html'] : TRUE;

Could this be a bit more generic so it's inline with other strategies in twig? Doesn't have to be exact 'is_safe' => ['html']

But the idea is that you can make it safe for different contexts. ['js', 'html'] or ['all'], what do you think?
https://github.com/twigphp/Twig/blob/5fdbd991bfcf5ea2492c9ab074a2d2cde18...
https://github.com/twigphp/Twig/blob/5fdbd991bfcf5ea2492c9ab074a2d2cde18...

Also I have tests over here #2531824: Attribute class to check safe strings before escaping (has tests) for a bug that looks like it's partially addressed by this patch for feed_icon.

I think given #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list we have another option here. If t() returns a TranslationWrapper then we can add a method on it to do this. See patch.

I like the idea in #8, it would solve part of the concern in #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness

1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -136,4 +136,31 @@ public function formatPluralTranslated($count, $translation, array $args = array
   +   * Never call translate($user_text) where $user_text is text that a user
   

   s/translate/translateForNonHtml/

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -136,4 +136,31 @@ public function formatPluralTranslated($count, $translation, array $args = array
   +   * entered; doing so can lead security problems. The output of this method is
   +   * intended for non-HTML usages. If the caller wants to ensure no HTML tags
   

   Should we be more forceful about how dangerous this is if it ends up in HTML (or javascript)?

3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
   @@ -136,4 +136,31 @@ public function formatPluralTranslated($count, $translation, array $args = array
   +   *   on the first character of the key, the value is escaped and/or themed.
   

   Maybe just "wrapped in <em>" as opposed to themed?

4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -265,4 +265,15 @@ public function getNumberOfPlurals($langcode = NULL) {
   +  public function translateForNonHtml($string, array $args = array(), array $options = array()) {
   +    $string = $this->doTranslate($string, $options);
   

   Just an idea, but if we're worried about developers not thinking when they use this, maybe a 'strip_tags' key in the options array that defaults to TRUE...

5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -128,4 +145,13 @@ public function jsonSerialize() {
   +   * Returns the translation unescaped for usages in non-HTML.
   

   Should we list a few example use cases here, i.e. error logs and plain text email messages?

6. +++ b/core/modules/contact/contact.module
   @@ -7,8 +7,10 @@
   +
   

   newline

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -265,4 +265,15 @@ public function getNumberOfPlurals($langcode = NULL) {
+      $string = strtr($string, $args);

If we have a string that's intended for non-HTML output only this muddles the meaning of the placeholder prefixes a bit as both @, ! and any other prefix would output the unescaped/unfiltered string... but I guess there's no way around that when we have a TranslationWrapper that has a single translatable string that may be output to both HTML and non-HTML.

Reducing the number of failing tests

+++ b/core/modules/system/src/Tests/Common/XssUnitTest.php
@@ -41,7 +41,7 @@ function testT() {
-    $this->assertEqual($text, 'Verbatim text: <script>', 't replaces verbatim string as-is.');
+    $this->assertEqual($text, 'Verbatim text: &lt;script&gt;', 't replaces and escapes string.');
   }
 
   /**
diff --git a/core/modules/system/src/Tests/Theme/TwigTransTest.php b/core/modules/system/src/Tests/Theme/TwigTransTest.php

diff --git a/core/modules/system/src/Tests/Theme/TwigTransTest.php b/core/modules/system/src/Tests/Theme/TwigTransTest.php
index 21aef18..d30e753 100644

index 21aef18..d30e753 100644
--- a/core/modules/system/src/Tests/Theme/TwigTransTest.php

--- a/core/modules/system/src/Tests/Theme/TwigTransTest.php
+++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php

+++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
+++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
@@ -139,7 +139,7 @@ protected function assertTwigTransTags() {

@@ -139,7 +139,7 @@ protected function assertTwigTransTags() {
     );
 
     $this->assertRaw(
-      'PAS-THRU: &"<>',
+      'PAS-THRU: &amp;&quot;&lt;&gt;',

I have just updated the tests since now ! will behave similar to @.

We need to move #2506445-170: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests here. This is the diff:

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -79,12 +79,12 @@ function testSendPersonalContactMessage() {
-      '!site-name' => $this->config('system.site')->get('name'),
-      '!subject' => $message['subject[0][value]'],
-      '!recipient-name' => $this->contactUser->getUsername(),
+      '@site-name' => $this->config('system.site')->get('name'),
+      '@subject' => $message['subject[0][value]'],
+      '@recipient-name' => $this->contactUser->getUsername(),
...
-    $this->assertEqual($mail['subject'], t('[!site-name] !subject', $variables), 'Subject is in sent message.');
-    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['!recipient-name']) !== FALSE, 'Recipient name is in sent message.');
+    $this->assertEqual($mail['subject'], t('[@site-name] @subject', $variables), 'Subject is in sent message.');
+    $this->assertTrue(strpos($mail['body'], 'Hello ' . $variables['@recipient-name']) !== FALSE, 'Recipient name is in sent message.');

These are for email tokens.

I think we should probably postpone this on #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list at this point, since that will change this patch.

Also, I think we should actually separate the two scopes of this issue. "Add new API for t() to work for non-HTML text" can mean simply using the new behavior from #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list to allow @placeholder to be used for emails since they would then be escaped at the same time as other strings, which will improve the DX overall by simplifying things.

However, deprecating !placeholder and making it behave like @placeholder is much broader in scope and does not have consensus. We may actually remove it instead, and anyway fixing the test failures for that change will duplicate efforts on #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests and friends.

Can we create a separate issue for that if we still think it's worth doing (as opposed to just removing them all)?

Also, there are some incorrect hunks in the current patch.

One thing #28 does not mention is the risk of adding "!placeholder by another name".

Also, I'm confused that all the suggestions in #28 involve adding methods. I thought that the idea was that converting to late rendering of the translations would mean that the placeholders could be escaped then, in the render process, so they wouldn't be escaped otherwise, same as the expectation for Twig.

Also, I'm confused that all the suggestions in #28 involve adding methods. I thought that the idea was that converting to late rendering of the translations would mean that the placeholders could be escaped then, in the render process, so they wouldn't be escaped otherwise, same as the expectation for Twig.

Well yeah at some point though you need to convert the object to a string. Maybe we could also go with TranslationWrapper->render($strategy)

Re-titled and updated issue summary to try to summarize the two approaches.

Also if we go for option #2, this wouldn't need to be postponed, so moving back to active for the discussion at least.

Not sure whether this is BS, since I just started to get my feet wet with the SafeMarkup stuff, but it seems to me that here we are needing an alternative sanitization logic. If we were able to instantiate a different sanitization service depending on the mime type of the output, we could use placeholders semantically and let each service figure out the most appropriate sanitization strategy. We could default to text/html as sanitization context and allow to specify alternative ones via $options. For example:

$args = ['@user_name' => $account->getName()];
// HTML email, @user_name is escaped.
t('Welcome @user_name!', $args);
// Plain text email, @user_name is not escaped.
t('Welcome @user_name!', $args, ['output' => 'text/plain']);

Well, the problem is that like for tokens, the place which generates the t() cannot know yet, how its gonna be used, so making it lazy would help with that.
@plach
I think we should do something, that is as parallel as twig, which means you would pass a sanitization strategy?

Well, the problem is that like for tokens, the place which generates the t() cannot know yet, how its gonna be used, so making it lazy would help with that.

So that's true for t() strings that end up in e-mail bodies - but we (should) already use MailHelper::htmlToText() for that.

I'm not sure it's true for e-mail subjects though - otherwise those places wouldn't be explicitly using the !placeholder to avoid sanitization.

i.e. $message['subject'] .= t('[!site-name] !subject', $variables, $options); from contact.module

Well, IMHO for email subjects we should be able to strip all tags.

@dawehner:

Well, the problem is that like for tokens, the place which generates the t() cannot know yet, how its gonna be used, so making it lazy would help with that.

On one hand we cannot have reliable output sanitization without knowing the output format, OTOH I realize that code generating token might miss the required contextual information, at least currently.

I think we'd have two ways to cope with this:

• We require a string requiring sanitization to be provided the output format as contextual information, as I was suggesting in the example above. For tokens this would mean passing the output format in $options, as we are doing for language right now. How we'd implement that practically I'm not sure about: maybe a user could choose between [title] and [title:plain] or stuff like that.
• Indeed a better solution DX-wise could be to lazily sanitize/stringify dynamic strings. For instance we could extend the approach introduced in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list and introduce a DynamicStringWrapper that could be passed around until contextual information about output format is finally available. This would allow us to have something like the following:
  

  $dynamic_string = t('Welcome, @user_name!', ['@user_name' => $account->getName()]);
  
  // Render the string as HTML, inherit Twig autoescaping.
  $build = ['#markup' => $dynamic_string];
  
  // Send as email content.
  function mymodule_send_welcome_email(DynamicStringWrapper $dynamic_string) {
    $dynamic_string->setOutputFormat('text/plain');
    // Send plaintext email, no escaping.
  }
  
  // Include it as HTML attribute, e.g. <input type="submit" value="$dynamic_string">.
  function _drupal_render_attribute(DynamicStringWrapper $dynamic_string) {
    $dynamic_string->setOutputFormat('text/html; x-drupal-context: attribute');
    // Escape attribute delimiters and type-specific content, e.g. URL protocol.
  }
  

  A token value could simply be wrapped into a child TokenStringWrapper having simply @value as hard-coded string pattern.
  In this scenario @value and :url placeholders would determine the value's semantics instead of the sanitization logic: the former would indicate any plain string, while the second would indicate a URL string. This way, depending on the output format, we would know whether escaping the value or sanitizing the URL protocol is needed, for instance.

Well, IMHO for email subjects we should be able to strip all tags.

I think this is not enough: if the string was "HTML-escaped" previously, stripping tags would have no effect and lots of bogus < and > could creep into the mail subject.

Just discussed this with plach, I'd thought that #2569485: Add AttributeSafeStringInterface and UriAttributeSafeStringInterface was a competing approach to this, but actually I think we should use both.

You get a $translated_string object.

The object has a ->renderAsHtmlAttribute() method.

This strips tags (to avoid <em> tags either being invalidly not escaped, or uglily escaped in an HTML attribute.

It also encodes entities to avoid XSS.

And returns an AttributeSafeStringInterface.

You can then pass an AttributeSafeStringInterface into AttributeString, so that it doesn't end up getting run through Html::escape() when it's already in the right format.

If we only did renderAsPlainText() then that might be OK for an e-mail subject but it's not necessarily OK for an HTML attribute.

If we only do AttributeSafeStringInterface then you can still end up with either escaped or unescaped HTML tags in attributes.

If we do both all cases are covered and it's clear what should be used for what.

I'll experiment a bit with this, although it would be better to get #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list in first.

Here is a first stab, just to get an idea of how things could look like. This includes also #2557113-185: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list, the interdiff is the new code.

So right now the first argument to t() is literal HTML and does not get escaped or filtered again (since it's marked as a SafeString). We also allow translated strings to have HTML in them.

We should probably make that more explicit than it currently is in the documentation, https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi... doesn't really make it clear that the first argument is HTML at all, except saying don't put variables in there.

Given in this issue we're talking about three different formats of returned string - HTML + plain text + HTML attribute, we need to figure out what that looks like.

Let's say we start with the following string:

t('The &lt;em&gt; tag makes your text look like <em>"this"</em>.')

1. HTML:

Nothing happens, you get this back:

The &lt;em&gt; tag makes your text look like <em>"this"</em>.

Which will look like

The <em> tag makes your text look like "this".

in the browser (i.e. a select option or the title tag of an image or whatever).

2. Plain text:

plach suggested html_entity_decode(strip_tags($string)); The other option would be factoring out https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Mail!MailFormatHe...

That would return:

The <em> tag makes your text look like "this".

Fine for e-mail subject lines and similar.

3. HTML attribute.

For this, I think we'd want to take the plain text string, then Html::escape() it, so:

Html::escape(html_entity_decode(strip_tags($string)));

That gets us:

'The <em> tag makes your text look like "this".

Which will look like:

The <em> tag makes your text look like "this".

In the browser.

Having typed that out makes me wonder the following (assuming the above is what we want, it might not be):

If we can get a plain text output from t(), then we can pass that plain text to Attributes, and AttributeString would Html::escape() it - and maybe that's enough of an API for passing the results of t() to attributes (which Views currently does).

Then that same example with arguments:

t('The @tag makes your text look like @result', ['@tag' =>'<em>', '@result' => SafeString::create('<em>"this"</em>']);

1. HTML:

First argument is escaped, second argument is marked as a SafeString so is not escaped. Return is correct HTML.

2. Plain text:

First argument is not escaped- this is fine.

Second argument is ... we could strip_tags() safe string arguments but erggh.

I think it'd be easier to get the HTML string first, then apply the same html_entity_decode(strip_tags($string)) to that. Gets us the same result regardless of whether the HTML is in the string or replacements then.

3. Attributes, as before we just Html::escape() the plain text value.

Html::escape(html_entity_decode(strip_tags($string)));

Hmm, not sure this is secure for all attributes, we may still need some special cases for other attributes such as on* then, and think about what to do about attributes that are not wrapped in double quotes.

2. Plain text:

The Html::escape(html_entity_decode(strip_tags($string))); might be fine for email subjects but the htmlToText helper is much nicer for longer text, I also quite like some of the things in https://github.com/soundasleep/html2text that we could be doing in htmlToText too.

Hmm, not sure this is secure for all attributes, we may still need some special cases for other attributes such as on* then, and think about what to do about attributes that are not wrapped in double quotes.

So at the moment we just don't support passing user-entered content to on* (and never should). But if we wanted to provide a way to encode them properly I think we need a new issue for that - would need to be applied to Attributes and Xss::attributes() too.

The Html::escape(html_entity_decode(strip_tags($string))); might be fine for email subjects but the htmlToText helper is much nicer for longer text

Yes I think that's an open question.

The output of t() will eventually be used in HTML context, but something else will escape it, such as drupal_attributes() or form_select_options().

So for me #2 is the tricky one, which #43 attempts to unpick, and I think plach's patch is compatible with what's in #43.

The problem being is that the output of t(), with the same strings and context, could be used as either an attribute value or an HTML fragment, certainly the Views info stuff is like that - and those need two different strategies.

I'd also add an example #6, which is the reverse problem of #1:

6. You thought you were creating a string for drupal_set_message() (or any HTML output), but someone put it into the subject of an e-mail instead.

#43 allows us to handle that case too.

This implements #43 - #44 and provides unit tests for that. I will start looking at test failures as soon as #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list is committed, since I don't want to waste time chasing every iteration. Most of the failures are unit tests that just need to be adapted to the new API.

Done for tonight

Here's a version of #49 including only the parts added here, except for some small bits in TranslationInterface and TranslationManager.

Added missing PHP docs

Rebased on top of #2557113-225: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list.

I really like this approach. Re-uploaded the review.txt patch in #56 since #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list is now in.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -119,34 +130,37 @@ public function getOptions() {
+   * TODO
    *
-   * @return string
-   *   The translated string.
+   * This should be an injected factory, likely a plugin manager.
+   *
+   * @return \Drupal\Core\OutputFormatter\OutputFormatterInterface
    */

Working on a service / plugin manager for replacing placeholders based on content type.

In this patch...
1. Added a plugin manager with @OutputFormatter annotation to manage the different kinds of output formatters.
2. Moved the three output formatters to the Plugin/OutputFormatter directory

Needs tests for the new classes.

This patch looks like it's off track in terms of scope and what it's doing. Let's make it as small as possible - ideally just doc and no API changes

Thanks, some copy/paste issues:

1. +++ b/core/lib/Drupal/Core/OutputFormatter/OutputFormatterManager.php
   @@ -0,0 +1,119 @@
   + * Contains \Drupal\Core\Block\BlockManager.
   

2. +++ b/core/lib/Drupal/Core/OutputFormatter/OutputFormatterManager.php
   @@ -0,0 +1,119 @@
   + * Manages discovery and instantiation of block plugins.
   + *
   + * @todo Add documentation to this class.
   + *
   + * @see \Drupal\Core\Block\BlockPluginInterface
   

3. +++ b/core/lib/Drupal/Core/OutputFormatter/OutputFormatterManager.php
   @@ -0,0 +1,119 @@
   +   * Constructs a new \Drupal\Core\Block\BlockManager object.
   

This patch looks like it's off track in terms of scope and what it's doing. Let's make it as small as possible - ideally just doc and no API changes

I'm not sure what API changes you are referring to, can you expand on that? Also, I don't really think we are going to address this issue just with documentation.

Please update the issue summary before posting any more patches.

I can't even tell if this is going in the right direction. I think adding more complexity to t() and the rest of the API here might be the wrong thing.

From the issue summary I was hoping this would be mostly a documentation issue - instead it's looking light a significant API change.

@plach - option #2 in the current issue summary suggests it can mostly be a docs issue. It's not clear to me when/if that option was discarded.

Sorry, we are currently going a different way, see #33 and #37, I will update the issue summary ASAP.

Discussing with alexpott at MOB. We need a conversation in person about broadening this into to a generic (but as simple as possible) system to allow us to render any SafeString with a context-relevant formatter.

This is still critical, and it blocks #2571673: Convert Views t() usage where it is used as an attribute value, which is also critical, even if you don't personally like the solution.

The problem is very much there, and does not 'need more info'.

Discussed more with pwolanin and alexpott at the sprint.

I don't think we should use plugins for this, @alexpott suggested just a method that takes a formatter and returns the string in the format, that seems plenty for extensibility to me. It'll be an interface with one method.

There's still a bit of discussion as to whether when we format for an attribute, do we return a plain text string that we then escape again, or return an escaped string and communicate to AttributeString and elsewhere that it doesn't get escaped again (via AttributeFormattedStringInterface or whatever which we don't have yet). However hopefully this clarifies exactly what the need is with the updated issue summary.

I read through the entire issue, and had the same remarks/questions/doubts about using plugins as #73. But catch posted it 10 minutes before I did :P

@pwolanin and @catch confirmed that we are now going with option 3 in the issue summary. That's why he tried to strike through the rest in #76, but failed miserably, because <del> is not able to strike through block-level elements :P

really more like #2

really more like #2

After further discussion just having an interface and classes with a static method is the simplest possible way to solve this problem.

Here's a starting patch showing the general idea.

Added some tests

@pwolanin @dawehner:

Can we restore the test cases using placeholders that were introduced in #49?

Working on this

1. +++ b/core/lib/Drupal/Component/Utility/AttributeValueOutput.php
   @@ -0,0 +1,26 @@
   +   * @param $string|SafeStringInterface
   
   +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,23 @@
   +   * @param $string|SafeStringInterface
   
   +++ b/core/lib/Drupal/Component/Utility/PlainTextOutput.php
   @@ -0,0 +1,26 @@
   +   * @param $string|SafeStringInterface
   

   Missing FQCN

2. +++ b/core/lib/Drupal/Component/Utility/AttributeValueOutput.php
   @@ -0,0 +1,26 @@
   \ No newline at end of file
   
   +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,23 @@
   \ No newline at end of file
   
   +++ b/core/lib/Drupal/Component/Utility/PlainTextOutput.php
   @@ -0,0 +1,26 @@
   \ No newline at end of file
   

   Missing newline

More tests and docs

Fixed test failures.

+++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
@@ -0,0 +1,25 @@
+/**
+ * Common interface for output strategies.
+ */
+interface OutputStrategyInterface {

Probably this interface should provide more details about what output strategies are, when to use them and how.

1. +++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
   @@ -0,0 +1,29 @@
   + * Implements an output strategy to be used to format strings to be used as
   

   Would be good to avoid 'to be used' twice in the sentecne.

   "Implements an output strategy used to format strings for HTML attribute values."?

2. +++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
   @@ -0,0 +1,29 @@
   +     return Html::escape(PlainTextOutput::renderFromHtml($string));
   

   We can skip the Html::escape() - Attributes/AttributeString will do that. I think that's what we decided this afternoon.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlAttributeValueOutputTest.php
   @@ -0,0 +1,71 @@
   +    $output = HtmlAttributeValueOutput::renderFromHtml($markup);
   

   Even if we don't have the separate output strategy for attributes vs. plain text, we could have an integration test with AttributeString to ensure it looks right after escaping?

+1 to #91 - I had discussed this with @plach earlier today and the conclusion was a "fancier" version of the plain text output for larger text could be a nice non-critical followup here. SimplePlainTextOutput and FormattedPlainTextOutput sound great to me

Some docs nits mostly...

1. +++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
   @@ -0,0 +1,29 @@
   + * Contains \Drupal\Component\Utility\PlainTextOutput.
   + */
   ...
   +class HtmlAttributeValueOutput implements OutputStrategyInterface {
   

   Contains \Drupal\Component\Utility\HtmlAttributeValueOutput

2. +++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
   @@ -0,0 +1,29 @@
   +   * @param $string|\Drupal\Component\Utility\SafeStringInterface
   +   *   An HTML string or a any object that can be cast to string.
   

   "or a any" :)

3. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,25 @@
   + * @file
   + * Contains \Drupal\Component\Utility\OutputStrategyInterface.
   ...
   +interface OutputStrategyInterface {
   

   OutputStrategyInterface doesn't really explain for me what this does. Maybe OutputEscapeStrategyInterface or OutputFormatStrategyInterface. Or perhaps just OutputFormatInterface.

So what's the plan for implementation of these on SafeStrings? The IS is not very clear on this. Are we adding a new :: renderAsFormat(OutputStrategyInterface $strategy) method to SafeStringInterface...?

Smal changes here and there.

Not working on this atm...

+1 for the SimplePlainTextOutput to make it possible to create more plaintext output strategies

I think all of these just return a simple string, so I don't think we should return a HtmlAttributeValueOutput or anything else for any of these.

+++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
@@ -0,0 +1,32 @@
+   * @param $string|\Drupal\Component\Utility\SafeStringInterface

I put this here originally, but actually we can accept any object that implements __toString
Not sure how to note that in the @param

+++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
@@ -0,0 +1,32 @@
+    // @todo Conver the result to AttributeSafeStringInterface, see
+    //   https://www.drupal.org/node/2569485

Typo here, but I also think the comment isn't right - we may use it with AttributeSafeStringInterface but every class implementing this interface should return a string.

+++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextOutputTest.php
@@ -0,0 +1,70 @@
+    $safe_string = $this->prophesize(SafeStringInterface::class);

Why not just create a SafeString object? I don't see the value of a mock here.

Making some further changes

I'm going to try to fix the test fails now.

+++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
--- /dev/null
+++ b/core/lib/Drupal/Component/Utility/PlainTextSimpleOutput.php

+++ b/core/lib/Drupal/Component/Utility/PlainTextSimpleOutput.php
@@ -0,0 +1,37 @@
+ * Contains \Drupal\Component\Utility\PlainTextSimpleOutput.
...
+class PlainTextSimpleOutput implements OutputStrategyInterface {

+++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextSimpleOutputTest.php
@@ -0,0 +1,64 @@
+class PlainTextSimpleOutputTest extends UnitTestCase {
...
+    $output = SimplePlainTextOutput::renderFromHtml($markup);

should be SimplePlainTextOutput

mis-named classes used in the code.

The PlainTextSimple / PlainTextFormatted was deliberate as SimplePlainText seemed more confusing but I don't care much about one or the other. If we're going to go back to SimplePlainText let's rather mention FormattedPlainText in the @todo as well.

I think we'll have to revert the SafeString change as SafeString is in Core\Render - which we're not supposed to refer to in Component?

Silly me - the DrupalComponentTest fails if you do that. Back to using mocks, plus fix another class name use.

1. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,35 @@
   + * Output strategies assist in transforming unsanitized HTML strings into
   + * strings that are appropriate for a given context (i.e. plain-text, HTML
   + * attributes), through performing the relevant sanitization and formatting.
   

   I'm still convinced by the comment about unsanizited. If you pass in something like t('Foo

2. +++ b/core/lib/Drupal/Component/Utility/PlainTextSimpleOutput.php
   @@ -0,0 +1,37 @@
   + * @todo Provide a PlainTextFormattedOutput strategy that transforms HTML
   + *   into formatted plain text for use in the email body and long texts.
   

   Do we need to come up with the email given that we already have \Drupal\Core\Mail\MailFormatHelper::htmlToText

@dawehner yes, 1 is confusing, let me see about rewriting that.

As to 2, earlier in the issue a PlainTextFormattedOutput option came up (to be added in a followup). As far as I can see this would be no different from MailFormatHelper, so we could just move the logic from MailFormatHelper to PlainTextFormattedOutput as the formatted plain text could be used in more contexts than just email.

As to 2, earlier in the issue a PlainTextFormattedOutput option came up (to be added in a followup). As far as I can see this would be no different from MailFormatHelper, so we could just move the logic from MailFormatHelper to PlainTextFormattedOutput as the formatted plain text could be used in more contexts than just email.

Do you think this is needed as part of this issue?

Do you think this is needed as part of this issue?

I think this should be a followup. Created #2573009: Provide a PlainTextFormattedOutput output strategy.

Some thoughts for now:

1. +++ b/core/lib/Drupal/Component/Utility/HtmlAttributeValueOutput.php
   @@ -0,0 +1,33 @@
   + * Use this when rendering a given HTML string into an HTML attribute value,
   + * such as select list options. Never use this to render strings into
   + * "style"/"on*" attributes, or attributes that are not wrapped in quotes.
   ...
   +  public static function renderFromHtml($string) {
   +    return Html::escape(PlainTextSimpleOutput::renderFromHtml($string));
   +  }
   

   Didn't we say select lists are special from other attributes?

   I'd also say there are use cases for including tags in a select list, for example if you are choosing different HTML tags for output of a field.

2. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,35 @@
   +<?php
   +/**
   ...
   + * Contains \Drupal\Component\Utility\OutputStrategyInterface.
   + */
   

   Minor: Blank line needed above docblock.

3. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,35 @@
   + * appropriate for a given context (i.e. plain-text, HTML attributes), through
   ...
   +   * a given output context (i.e. plain-text email subjects, HTML attribute
   

   Minor: I think these i.e. should both be e.g.,.

4. +++ b/core/lib/Drupal/Component/Utility/PlainTextSimpleOutput.php
   @@ -0,0 +1,37 @@
   + *   into formatted plain text for use in the email body and long texts.
   

   What does long texts mean here?

Fixed minors (#121.2 and #121.3).

Re #107 #108, #110: Sorry, didn't know there had been a decision to change the names. Patch looks good.

Given our previous discussion with @stefan.r I think we should add explicit UI test coverage for select form elements with quotes in there and HTML just to see what we exactly should do.

I'm actually not sure about options elements being special. <option>&lt;</option> in a select does /not/ double escape for me.

Discussed this patch with @catch earlier today and we didn't see the need of escaping HTML attributes - merely turning them into plain text is enough. I do think it makes sense to have a dedicated class for them though, even if they only wrap the plain text one.

If select elements really are special let's address them in a followup as I don't think they're blocking any criticals whereas this one is. Let's get this patch in?

Select options in a followup is fine with me.

I don't understand the last change. The help text says "Use this when rendering a given HTML string into an HTML attribute value"

If people are putting it into an attribute value (or if we wire this up to a Twig filter) it needs to be escaped.

@pwolanin I had discussed this with @catch and the idea was not to do any sanitization in these output strategies and arrange this with Twig instead. Which would need updated docs if that's what we want to do.

Alternatively maybe we could sanitize and then mark as safe (for output into attributes).

re #132 why would we need to escape when outputting into a plain text context? It'd be for output into (non-URL/style/on*) attributes in any case, right?

And wouldn't that attribute value get double escaped whenever Twig has a go at that string?

@stefan.r - in the twig context I was assuming you'd use this as a filter, not let Twig do the default autoescaping.

@pwolanin OK can you double check with @catch what we want to do here then?

@pwolanin

Regardless of what we decide, plain text should not be HTML-escaped. I think the change was applied to the wrong strategy class.

Oh, huh - yes. I'm a little tired

If we let Twig don't we have to add knowledge about the strategy to AttributeString since that's what we use mostly.

I don't think we should do that here. The Views case where t() is put into an attribute can do plain text then put that string into AttributeString which does the escaping.

Anything beyond that is major followup for me not release blocking. And we shouldn't add any strategies we can't use in core at all here either.

Ok, removing the HTML attribute strategy altogether. We can add it back later if needed.

Also improved docs a bit.

1. +++ b/core/lib/Drupal/Component/Utility/PlainTextSimpleOutput.php
   @@ -0,0 +1,31 @@
   + *   into formatted plain text for use in the email body and CLI. See
   

   Nit: into fits the previous line

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
   @@ -140,9 +140,6 @@ public function render() {
   -    // @todo https://www.drupal.org/node/2509218 Note that the argument
   -    //   replacement is not stored so that different sanitization strategies can
   -    //   be used in different contexts.
   

   Why was this @todo removed in #88?

On this

@lauriii

Why was this @todo removed in #88?

Because we changed approach: we no longer automatically apply the output strategy from the ::render() method, instead we apply it to its return value.

This looks good for me now :) Thanks @plach!

I think we should be done here.

Working on a CR

RTBC++

CR at https://www.drupal.org/node/2574697

Discussed with @alexpott and we agreed PlainTextOutput is a better name for the strategy we are adding here, PlainTextFormattedOutput is still a good name for the upcoming advanced strategy.

And now with patch!

Created #2574723: Figure out whether we need a dedicated output strategy for select elements.

Fails on bot are PHP Fatal error: Uncaught exception 'ReflectionException' with message 'Class Drupal\Tests\Component\Utility\PlainTextOutputTest does not exist'

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
--- /dev/null
+++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextOutputTest.php

+++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextOutputTest.php
@@ -0,0 +1,68 @@
+ * Contains \Drupal\Tests\Component\Utility\PlainTextSimpleOutputTest.

due to a mismatch between class and filename.

Working on #159, and whatever else @alexpott finds during review.

Quick rename so tests can run while review proceeds.

PlainTextFormattedOutput makes no sense to me.

How can something be both plain and formatted? Contrast with the text field types we have: — you must choose, you can't have both.

Can somebody enlighten me?

+++ b/core/lib/Drupal/Component/Utility/PlainTextOutput.php
@@ -0,0 +1,31 @@
+ * @todo Provide a PlainTextFormattedOutput strategy that transforms HTML into
+ *   formatted plain text for use in the email body and CLI. See
+ *   https://www.drupal.org/node/2573009.

I'm too am confused by this comment. I thought this patch would give as the tools necessary to do #2572597: Replace !placeholder with @placeholder in mail code. Tbh I don't get why we have to do this - surely MailFormatHelper::htmlToText() just does what it does and this is great.

1. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,36 @@
   + * appropriate for a given context (e.g. plain-text), through performing the
   

   s/plain-text/plain text/

2. +++ b/core/lib/Drupal/Component/Utility/OutputStrategyInterface.php
   @@ -0,0 +1,36 @@
   + * relevant formatting. No santization is applied.
   

   s/santization/sanitization/

3. +++ b/core/lib/Drupal/Component/Utility/PlainTextOutput.php
   @@ -0,0 +1,31 @@
   + * Provides an output strategy for transforming HTML into simple plain text.
   

   What is "simple" plain text? Isn't plain text always "simple"?

   I'm betting this is related to the "formatted plain text", but that doesn't mean this is clear. I think it's fine to omit.

4. +++ b/core/lib/Drupal/Component/Utility/PlainTextOutput.php
   @@ -0,0 +1,31 @@
   + * @todo Provide a PlainTextFormattedOutput strategy that transforms HTML into
   + *   formatted plain text for use in the email body and CLI. See
   + *   https://www.drupal.org/node/2573009.
   

   Discussed with @jhedstrom, and now #162 is answered. I still think the name doesn't make sense, but we can discuss that further/refine it in the follow-up issue. I will comment there in a minute.

5. +++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextOutputTest.php
   @@ -0,0 +1,68 @@
   +   * @param array $args
   

   s/array/string[]/

6. +++ b/core/tests/Drupal/Tests/Component/Utility/PlainTextOutputTest.php
   @@ -0,0 +1,68 @@
   +   *   none.
   

   s/none/the empty array/

EDIT: to be clear, I fixed all my nits.

Ok, per #163, we need to deal with the mail thing here.

I think PlainTextFormattedOutput is an extremely confusing name. So let's try to find something better. This is the relevant code:

  /**
   * Transforms an HTML string into plain text, preserving its structure.
   *
   * The output will be suitable for use as 'format=flowed; delsp=yes' text
   * (RFC 3676) and can be passed directly to MailManagerInterface::mail() for sending.
   *
   * We deliberately use LF rather than CRLF, see MailManagerInterface::mail().
   *
   * This function provides suitable alternatives for the following tags:
   * <a> <em> <i> <strong> <b> <br> <p> <blockquote> <ul> <ol> <li> <dl> <dt>
   * <dd> <h1> <h2> <h3> <h4> <h5> <h6> <hr>
   *
   * …
   */
  public static function htmlToText($string, $allowed_tags = NULL) {

So it is transforming HTML into plain text, while preserving the HTML structure and just mapping it to a syntax defined in RFC 3676. That RFC is titled The Text/Plain Format and DelSp Parameters.

So it's actually transforming text/html to text/plain.

Suggested names based on this so far: StructuredPlainTextOutput, MailPlainTextOutput, PlainTextMailOutput.

Second, let's look at that RFC for inspiration:

3.  The Problem

   The Text/Plain media type is the lowest common denominator of
   Internet email, with lines of no more than 998 characters (by
   convention usually no more than 78), and where the carriage-return
   and line-feed (CRLF) sequence represents a line break (see [MIME-IMT]
   and [MSG-FMT]).

   Text/Plain is usually displayed as preformatted text, often in a
   fixed font.  That is, the characters start at the left margin of the
   display window, and advance to the right until a CRLF sequence is
   seen, at which point a new line is started, again at the left margin.
   When a line length exceeds the display window, some clients will wrap
   the line, while others invoke a horizontal scroll bar.

   Text which meets this description is defined by this memo as "fixed".

Suggested names based on this: PreformattedPlainTextOutput, MonospacedPlainTextOutput.

Conclusion: pick one of these names:

1. StructuredPlainTextOutput
2. MailPlainTextOutput
3. PlainTextMailOutput
4. PreformattedPlainTextOutput
5. MonospacedPlainTextOutput

I think PlainTextMailOutput is the best one.

Discussed with @stefan.r and @alexpott, and decided the @todo could be removed entirely, and that this is rtbc assuming it goes green.

Eh, ok. Can we document here why?

I've transplanted my commment at #165 to #2573009-3: Provide a PlainTextFormattedOutput output strategy.

re #167 the reasoning is that the new class isn't strictly needed as part of this fix, so an @todo is premature, but the new class can still be discussed in #2573009: Provide a PlainTextFormattedOutput output strategy.

Thanks everyone - this looks a great solution. Committed 70bad3e and pushed to 8.0.x. Thanks!

Too slow old testbot, too slow.

:)

This change attempts to introduce a custom concept of "output strategies", not a utility. Why was the code added to Utility?

@sun:

hey :)

My original patch was providing an OutputFormatter namespace, @pwolanin asked for a simplification of the approach and since both Html and SafeMarkup live in Utility, that felt like a good place also for PlainTextOutput.

Btw, I think it would useful if SafeMarkup implemented OutputStrategyInterface, providing a ::renderFromHtml() method implementation just casting its input to string. That would allow to pass it to methods receiving any output strategy as input.