Remove SafeMarkup::set() from AllowedTagsXssTrait

1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   + * Defines an object that passes safe strings through the Filter system.
   

   s/Filter/Field/

2. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +   * Returns a list of tags allowed by AllowedTagsXssTrait::fieldFilterXss().
   

   C/P remnant.

3. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +    return array('a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img');
   

   Nit: [].

4. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
   +        // @todo convert to markup using the tag list.
   
   +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
   @@ -35,7 +35,8 @@ public function viewElements(FieldItemListInterface $items) {
   +      // @todo convert to markup using the tag list.
   

   These are the two only places that will use #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter. The IS made it sound like many places would use what's introduced there.

5. +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
   @@ -80,12 +81,14 @@ public function summaryName($data) {
   +      // Filtered an escaped string will not change it.
   
   +++ b/core/modules/views/src/Plugin/views/argument/ListString.php
   @@ -72,12 +73,14 @@ public function summaryName($data) {
   +      // Filtered an escaped string will not change it.
   

   Parse error.

Fixed fatal error in Drupal\options\Tests\OptionsFieldUITest.

Just getting this green again for the moment.

edit:

+++ b/core/modules/file/file.field.inc
@@ -7,8 +7,10 @@
 use Drupal\Component\Utility\Html;

and this is unused now as well

And some nits:

1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +    return ['a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img'];
   

   Double space after ins

2. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +   * @param $option
   +   * @return $this|static
   +   *   If the case transformation does not affect the string the same object is
   

   Missing documentation, $this is not a type

3. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +        break;
   ...
   +        break;
   ...
   +        break;
   

   Missing newlines

Addressed the points by @stefan.r in #9 #10.

+++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
@@ -0,0 +1,113 @@
+   * @param $option
+   * @return $this|static
+   *   If the case transformation does not affect the string the same object is

Missing documentation, $this is not a type but its getting returned as a safe String.

1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
   @@ -0,0 +1,113 @@
   +  /**
   +   * @param $option
   +   * @return $this|static
   +   *   If the case transformation does not affect the string the same object is
   +   *   returned. If it does, then a new object is returned, preserving
   +   *   immutability.
   +   */
   

   Docs need to be improved, as #12 also said.

2. +++ b/core/modules/file/file.field.inc
   @@ -7,8 +7,10 @@
   +
    /**
   

   Unnecessary whitespace addition.

3. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
   +        // @todo convert to markup using the tag list.
   +        $elements[$delta] = array('#markup' => FieldFilteredString::create($output));
   
   +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
   @@ -35,7 +36,8 @@ public function viewElements(FieldItemListInterface $items) {
   +      // @todo convert to markup using the tag list.
   +      $elements[$delta] = array('#markup' => FieldFilteredString::create($item->value));
   

   I guess these make this issue blocked on #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter? This can still continue in the mean time though.

4. Finally, tests are needed for the caseTransform() stuff, per #2.

Looking at this as part of the Acquia Build Week Hackathon

Addressed comments on #14.

Thanks @Mariano!

Note that it's helpful to provide interdiffs when updating existing patches in the d.o patch-based workflow to make it easier to review the changes. I've attached one here. :)

1. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
   @@ -0,0 +1,101 @@
   +  public function testCreate($string, $expected, $instance_of_check = FALSE) {
   ...
   +    $data[] = [
   +      '', '',
   +    ];
   

   The gain of making the third parameter optional is very small. Let's just be explicit and require FALSE to be specified?

2. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
   @@ -0,0 +1,101 @@
   +    $data[] = [
   +      'teststring', 'teststring', TRUE
   +    ];
   

   This looks a bit strange. Let's do either:

   $data[] = [$x, $y];
   

   or

   $data[] = [
     $x,
     $y,
   ];
   

   But not this in-between variant. Either of the two would be consistent with the rest of core.

3. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
   @@ -0,0 +1,101 @@
   +
   +  /**
   +   * @covers: ::allowedTags
   +   */
   +  public function testAllowedTags() {
   +    $expected = ['a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img'];
   +
   +    $this->assertSame($expected, FieldFilteredString::allowedTags());
   +  }
   +
   +  /**
   +   * @covers: ::displayAllowedTags
   +   */
   +  public function testdisplayAllowedTags() {
   +    $expected = '<a> <b> <big> <code> <del> <em> <i> <ins> <pre> <q> <small> <span> <strong> <sub> <sup> <tt> <ol> <ul> <li> <p> <br> <img>';
   +
   +    $this->assertSame($expected, FieldFilteredString::displayAllowedTags());
   +  }
   

   This is just asserting the same exact hardcoded list. That's not worth testing.

4. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
   @@ -0,0 +1,101 @@
   +  public function testCaseTransform($option, $test_string, $expected) {
   ...
   +  public function providertestCaseTransform() {
   

   Perfect :)

Fair point. I don't feel very strongly about it. Let's let a committer decide then :)

Dear committer, see #20.3 — I think those are useless tests, especially testAllowedTags(). If you think this is useful, keep it, if you think it's not, remove them.

Excellent! Just one thing:

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
+        // @todo convert to markup using the tag list.

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
@@ -35,7 +36,8 @@ public function viewElements(FieldItemListInterface $items) {
+      // @todo convert to markup using the tag list.

This @todo is still left and can now actually be done.

(I forgot about that when RTBC'ing in #23 — sorry.)

Yep, exactly :)

Failing on:

    $this->assertRaw('<strong>Test with an upload field.</strong>');

This should actually be escaped AFAICT?

As should the next assertion (which is not failing because the test already failed on the above:

    $this->assertRaw('<em>Test with a non upload field.</em>');

Such noob :P

back to RTBC

+++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
@@ -0,0 +1,80 @@
+ *
+ * @internal
+ *   This object is marked as internal because it should only be used in the
+ *   Filter module on strings that have already been been filtered and sanitized
+ *   in \Drupal\filter\Plugin\FilterInterface.
+ *
+ * @see \Drupal\Core\Render\SafeString
+ */
+final class FieldFilteredString implements SafeStringInterface, \Countable {
+  use SafeStringTrait;
+

These docs are copy and pasted from the filter tips patch I think?

Needs updating, and also needs to apply to the usages across field modules and views, which makes this a bit less @internal.

which makes this a bit less @internal.

Hrm good point. It's still "relatively" @internal to the Field module, but given that it's now used by the options, image and views module, that's indeed less the case. It's still all places where "Field-specific stuff" is happening.

Wondering what Alex Pott thinks about this.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
@@ -75,8 +75,8 @@ public function viewElements(FieldItemListInterface $items) {
-        $prefixes = isset($settings['prefix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['prefix'])) : array('');
-        $suffixes = isset($settings['suffix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['suffix'])) : array('');
+        $prefixes = isset($settings['prefix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['prefix'])) : array('');
+        $suffixes = isset($settings['suffix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['suffix'])) : array('');

Wow, good catch. Why was this not causing test failures before?

stefan_r answered #38 in IRC:

12:59:39 <stefan_r> as to the AllowedXssTrait one, why would the previous code trigger a test failure? it's essentially the same, except instead of calling the wrapper we call the method directly
13:01:26 <stefan_r> ie AllowedXssTrait::fieldFilterXss() just wraps FieldFilteredString::create()

Of course. We're basically just keeping AllowedXssTrait::fieldFilterXss() for BC. Should we deprecate it?

EDIT: deprecate for 9, of course.

The whole trait is deprecated in the patch, we could copy that @deprecated notice to all its methods if needed?

#40: oops, missed that. My bad.

Back to RTBC.

+++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
@@ -0,0 +1,63 @@
+    // HTML will be normalised.

normalize. Fixed on commit, for some definitions of fixed.