Views render pipeline is escaping CustomBooleanTest

This will reduce the number of fails (somewhat..)

1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
   @@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
   +    $this->drupalGet('test_page_display_200');
   

   This was returning 404

2. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
   @@ -22,7 +22,23 @@ class TaxonomyFieldAllTermsTest extends TaxonomyTestBase {
   +  public static $testViews = array('taxonomy_all_terms_test', 'test_page_display');
   

   So I added `test_page_display`

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
@@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
+    // @todo Fix this.
+    $this->testViewsHandlerAllTermsField();

Can you please explain why `@todo` here. Everything starting with `test` itself is a test and is executed by the system. Why are you executing them explicitly.

Patch with fixes in #2348729: Convert theme_views_view_field to twig

Don't we seriously need some profiling on this issue?

1. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
   @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        $separator = Xss::filterAdmin($this->options['separator']);
   ...
   +          '#template' => '{{ items | safe_join(separator) }}',
   

   Oh so you have to ensure that the $separator is safe even if you use a template? This somehow feels counter intuitive, to be honest.

2. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
   @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +
   +      return \Drupal::service('renderer')->renderRoot($build);
   

   Don't you want a renderPlain here?

This fixes point 2 in in #14. I'm not clear on what the next step is for point 1. Just remove the XSS:filterAdmin()? Is that needed because it's used both in the template and the context below that? I'm not quite sure what's going on there.

1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php
   @@ -43,6 +59,27 @@ public function testViewsHandlerAllTermsField() {
    
   +  public function testViewsHandlerAllTermsFieldWithTemplate() {
   

   Its a bit odd that this kind of test coverage lives in a file related with taxonomy term fields and not some other test more specific test class

2. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
   @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        $separator = Xss::filterAdmin($this->options['separator']);
   +        $build = [
   +          '#type' => 'inline_template',
   +          '#template' => '{{ items | safe_join(separator) }}',
   +          '#context' => ['separator' => $separator, 'items' => $items],
   

   This is the same as Field.php does, so this is fine.

3. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
   @@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +
   +      return \Drupal::service('renderer')->renderPlain($build);
   

   Are you sure we need renderPlain() and cannot just use render()?

3. I used renderPlain because you suggested it. If we use straight up render, don't we need to define a render context? I don't fully understand how that works, but thought I spotted that when I was working on the page title block.

+++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
@@ -78,17 +79,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        $build = array(

Can we get this in short array syntax?

+++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-field.html.twig
@@ -0,0 +1,11 @@
+Use posts instead of twigs to protect your llamas from escaping the field.

+1

This adds the short array syntax. That's the only feedback I felt I could take action on right now.

Time to review again, and if it's looking good, move forward?

Rerolling

Not entirely sure of this re-roll... pls have a look -=)

The #30 patch was failing to apply against latest HEAD:

patching file core/modules/views/src/Plugin/views/field/FieldPluginBase.php
Hunk #1 FAILED at 1308.


Here's a reroll.

Please ignore #35, it was missing one of the files. Here's a with the previously missing file.

I've been digging around different views tests and found out that these escaping issues needs a bit of testing planning which I think somebody more experienced should take a look (I'm also quite new to SafeMarkup). There is a test \Drupal\views\Tests\ViewsEscapingTest which could be abstracted.

I also think that this may be more child issue of #2297711: Fix HTML escaping due to Twig autoescape rather than #2348729: Convert theme_views_view_field to twig, should we relink this?

I'm gonna drop on this and leave this to somebody who knows lots of about testing and lots of about SafeMarkup and let myself move on to work on another issue :)

Shouldn't the tests-only patch fail?

1. I manually tested the patch, ensuring that a) markup is not escaped (as expected) and b) markup is still admin filtered (as expected).
2. Reviewed the code, and it looks sane to me, just 2 small issues (but neither should hold up this patch I think):

   1. +++ b/core/modules/views/src/Plugin/views/field/Boolean.php
      @@ -118,7 +119,8 @@ public function render(ResultRow $values) {
      +      $custom_value = $value ? UtilityXss::filterAdmin($this->options['type_custom_true']) : UtilityXss::filterAdmin($this->options['type_custom_false']);
      +      return ViewsRenderPipelineSafeString::create($custom_value);
      

      Nitpick: I would defer UtilityXss::filterAdmin() until the return statement (to avoid having it twice, since we are doing it for $custom_value regardless of value).

   2. +++ b/core/modules/views/tests/themes/views_test_theme/templates/views-view-field.html.twig
      @@ -0,0 +1,11 @@
      + * The reason for this template is to override the theme function provided by
      + * views.
      

      Should this explain *why* we want to override the theme function for this test (to force usage of twig auto-escaping on rendering the field values, so we can ensure there is no double-escaping going on), or is this not necessary?

Removing the manual testing tag since manual testing was done in #60.

Manual testing and review completed, all issues addressed, RTBC.

Committed f26b065 and pushed to 8.0.x. Thanks!