Improve FormattableMarkup documentation

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *   - %variable: Use when @variable would be appropriate, but the placeholder
   +   *     value will also be wrapped in <em> tags with a class. As with
   

   The grammar here sounds a bit funny

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *     @variable, do not use within the "<" and ">" of an HTML tag, such as in
   +   *     HTML attribute values. Doing so is a security risk.
   ...
   -   *     As with @variable, do not use this within HTML attributes, JavaScript,
   -   *     or CSS. Doing so is a security risk.
   

   Is there a reason we are moving this?

3. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   -   *       $string = "%output_text";
   -   *       $arguments = ['output_text' => 'text output here.'];
   -   *       $this->placeholderFormat($string, $arguments);
   +   *       $this->placeholderFormat('Prefix %output_text', ['%output_text' => 'text output.']);
   

   +1

4. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   -   *   - :variable: Return value is escaped with
   +   *   - :variable: Use when the return value is to be used as a URL value of an
   +   *     HTML attribute, for example the href attribute in an a tag.
   +   *     Return value is escaped with
   ...
   -   *     using the "href" attribute, ensuring the attribute value is always
   -   *     wrapped in quotes:
   +   *     using the "href" attribute, ensuring the value is always wrapped in
   +   *     quotes.
   

   This change may not be needed, in the parent issue it was decided to only support the "href" attribute?

5. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -179,6 +169,28 @@ public function jsonSerialize() {
   +   *     @todo Add some advice and stronger warnings.
   +   *       https://www.drupal.org/node/2569041
   +   *
   

   Just to be clear, what is this about and how is this related?

6. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -179,6 +169,28 @@ public function jsonSerialize() {
   +   *   In $string, only HTML attributes that take a URL as the value, are
   +   *   supported via the special ":variable" placeholder. For example, with the
   +   *   "href" attribute, the ":variable" placeholder allows simple links to be
   +   *   inserted:
   

   This change may not be needed, in the parent issue it was decided to only support the "href" attribute?

7. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -179,6 +169,28 @@ public function jsonSerialize() {
   +   *     - Secure: @code <a href=":variable">link text</a> @endcode
   +   *     - Secure: @code <a href=":variable" title="static text">link text</a> @endcode
   +   *     - Secure: @code <a href=":variable">@variable</a> @endcode
   

   +1 to less of the $this->placeholderFormat noise. Should we keep the explanations that were removed in the first hunk of the patch?

8. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -179,6 +169,28 @@ public function jsonSerialize() {
   +   *       - Insecure: @code <img src=:variable />@endcode
   

   This change may not be needed, in the parent issue it was decided to only support the "href" attribute?

Comments #42 and #50 in the parent. Other attributes than href are untested, and the docs state to only use formattablemarkup for simple markup. Core has no use case for any other attributes, and contrib can be added as people ask for it.

moving to docs component, right?

I think maybe this isn't done, but I took a look through the docs as they are and have some comments. This is confusing stuff.

Overall: I think we should say something, somewhere (not sure where), about the general strategy for all these new formatting objects. Which I think is: when you are formatting some text, you need to always use the proper objects and placeholders to format anything that comes from the database (which is usually unsanitized) or user input (definitely unsanitized). If you do so, everything should be escaped exactly once, not zero times (security risk) or multiple times (ugly).

This text doesn't quite get that across to me. Some detailed notes follow, but that is the general gist.

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -32,30 +32,9 @@
   - * - Variable placeholders should not be used within the "<" and ">" of an
   ...
   + * - The result of casting this object to a string should not be used within the
   

   This is totally changing the meaning of this section of docs. Is that right?

   Because before it was basically saying:

   Within the string you are formatting with placeholders, don't use placeholders in the part that is between < and > in an HTML tag.

   And now it says something totally different:

   Don't use the output of formatting this entire thing between < and > of an HTML tag.

   I don't think that is the right change to make?

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *   - @variable: Use as the default choice for anything displayed on the
   +   *     site. Do not use within the "<" and ">" of an HTML tag, such as in HTML
   +   *     attribute values. Doing so is a security risk.
   

   I don't get it. Why is this a security risk? It is supposedly sanitized -- just below it says it is sanitized. So why is this a security risk? The security risk would only happen if you also used the wrong object to sanitize that placeholder.

3. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *     - A MarkupInterface object, the object will be cast to a string and not
   +   *       sanitized.
   

   Yes, but that is presumably because the MarkupInterface object has previously been properly sanitized. If it was the right object and right placeholders to begin with, you should be OK?

4. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *       To force sanitization of a MarkupInterface object, cast the
   +   *       replacement value to a string, since it is not known what strategy a
   +   *       MarkupInterface object uses for sanitization.
   

   But... The point is that you don't want to double-escape tags. If you use the right type of string formatting/sanitizing class, you should be OK right?

   Also I had to read this at least twice to understand it. Can it say something like:

   ... instead of passing a MarkupInterface object directly as the placeholder replacement value, cast it to a string first.

5. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *   - %variable: Use when @variable would be appropriate, but the placeholder
   +   *     value will also be wrapped in <em> tags with a class. As with
   

   how about "... but you want the placeholder value to be wrapped in em tags..." or something like that? Confusing as it is.

6. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *     - The : placeholder should be used for HTML attribute values, but is
   +   *       insecure not to have quotes around the attribute value:
   

   I thought it said above that it's only for href attributes, not for all attributes?

7. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *     - Only the "href" attribute is supported. So, even other attributes
   +   *       which take a URL value are unsupported:
   +   *       - Insecure: @code <img src=":variable" />@endcode
   

   ok so this contradicts it. Better not to have contradictory text in the docs.

I am working on this.

Addressed https://www.drupal.org/node/2580505#comment-10451213 5, 6 and 7 in this patch.

I still don't know about this patch.

a) It cuts out a huge amount of documentation. Is this somewhere else? If so we should at least point there.

b) It makes a statement about security that I still do not understand.

Addressed #23 1 and 2.

re #16

2) I'm not sure an attribute value is an example that's easy to see why it's dangerous. But, let's assume an href. I can supply a value like this which is fully escaped but still allows me to execute arbitrary javascript when the link is clicked:

<a href="javascript:eval(String.fromCharCode(97,108,101,114,116,40,34,35,34,41));">click me</a>

If we assume tag name, or an attribute name, they are even more trivial to exploit. This is why we have the special ":" placeholder for URLs to strip dangerous protocols in URL attribute values.

3) This is basically the replacement for the ! placeholder in D7 - we want some markup to pass through. This should have some more warnings perhaps.

4) I guess there are cases where double escaping is better than no escaping, or the markup object maybe have safe HTML but we need plain text.

#24
minor changes in patch about exceeding 80 characters.

not added any interdiff file.

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *     - A MarkupInterface object, the object will be cast to a string and
   +   *       not sanitized.
   ...
   +   *       To force sanitization of a MarkupInterface object, cast the
   +   *       replacement value to a string, since it is not known what strategy a
   +   *       MarkupInterface object uses for sanitization.
   

   This wording here still seems a bit confusing

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   -   *   - %variable: Use when the replacement value is to be wrapped in <em>
   -   *     tags.
   ...
   +   *     placeholder value to be wrapped in an em tag with a placeholder class.
   

   We're changing wrapped in <em> tags to wrapped in an em tag with a placeholder class, but isn't the previous wording clearer?

3. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
   +   *   - :variable: Use when the return value is to be used as a URL value of
   +   *     an HTML attribute. Only the "href" attribute is supported.
   

   Use when the placeholder value is to be used as the value of a "href" attribute.

4. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,39 +113,50 @@ public function jsonSerialize() {
       *     protocols using UrlHelper::stripDangerousProtocols(). Use this when
   ...
   +   *     using the "href" attribute, ensuring the value is always wrapped in
   

   The bit about the href attribute is repeated here.

5. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *   In $string, only the "href" HTML attribute, which takes a URL as the
   

   Not just in $string, maybe this could be more forceful about us not supporting any HTML attribute anywhere through any placeholder (just href).

6. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *     - It is insecure in $string to have @ or % placeholders within the "<"
   

   "@variable" or "%variable"

7. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *       - Insecure: @code <@variable>text</@variable> @endcode
   +   *       - Insecure: @code <a @variable>link text</a> @endcode
   +   *       - Insecure: @code <a href="@variable">link text</a> @endcode
   +   *       - Insecure: @code <a title="@variable">link text</a> @endcode
   +   *       - Insecure: @code <a href=":variable" title="@variable">link text</a>@endcode
   

   Should these have more explanation about why these are insecure (see the removed code on top of this patch)?

8. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *     - The : placeholder should be used for HTML "href" attribute values,
   

   ":variable" placeholder

9. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -180,6 +170,28 @@ public function jsonSerialize() {
   +   *       - Insecure: @code <img src=":variable" />@endcode
   

   This is not insecure, just unsupported :)

Status was wrong, per last review.

Reroll the Patch #26 also covers #27.

Reroll the Patch #26 also covers #27.

+ * - The result of casting this object to a string should not be used within

I had trouble reading this at first. It might read better was "The result from casting..."

+ * placeholder value to be wrapped in an em tag with a placeholder class.

Should the "em" here be capitalized? I'm not sure if there is a standard. Examples I checked around core are inconsistent.

Fixed the CS issue in patch #43.
142 | WARNING | Line exceeds 80 characters; contains 81 characters

Running 10.1 tests if they pass I'll mark RTBC.

Unfortunately the formatting needs a bit of work still:

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,35 +113,47 @@ public function jsonSerialize() {
       *       using \Drupal\Component\Utility\Html::escape().
   ...
   +   *       A call like:
   

   These lines can be wrapped together.

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,35 +113,47 @@ public function jsonSerialize() {
   +   *     - %variable: Use when @variable would be appropriate, but you want the
   +   *     placeholder value to be wrapped in an <em> tag with a
   

   Indentation is off here in the first line.

3. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,35 +113,47 @@ public function jsonSerialize() {
       *     @code
   ...
   +   *     $this->placeholderFormat('Prefix %output_text', ['%output_text' => 'text output.']);
       *     @endcode
   

   Indentation is off here.

4. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -134,35 +113,47 @@ public function jsonSerialize() {
   +   *     - :variable: Use when the return value is to be used as a URL value of
   

   Indentation is off here.

Easy fixes

Two more nits and a wider comment:

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -136,35 +115,47 @@ public function jsonSerialize() {
   +   *       using \Drupal\Component\Utility\Html::escape(). A call like:
   +   *
   +   *       @code
   

   Blank line isn't needed here.

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -136,35 +115,47 @@ public function jsonSerialize() {
   +   *     - :variable: Use when the return value is to be used as a URL value of
   +   *       an HTML attribute. Only the "href" attribute is supported.
   +   *     Return value is escaped with
   

   Indentation is still off here; the first two lines need to go back in by two spaces. Also it looks like this entire paragraph can be rewrapped at closer to 80 characters.

3. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -182,6 +173,31 @@ public function jsonSerialize() {
   +   *   In $string, only the "href" HTML attribute, which takes a URL as the
   +   *   value, is supported via the special ":variable" placeholder. With the
   +   *   "href" attribute, the ":variable" placeholder allows simple links to be
   +   *   inserted:
   

   This is a bit repetitive of the section above that mentions the :variable format. Should we combine the two?

Not the best technical writer. There a good tag to get some feedback on that?