Double-escape on Views filters

Probably Major.

We need to mark as secure and probably when creating the link.

Using inline_template to avoid SafeMarkup::set().

Oops, Sorry I forgot to comment that @dawhener told me to comment this change with FabianX.

Is this change intentional? I don't think this fits into the scope of this issue.

Let me explain, where this is coming from.

So the settings links are part of core/modules/views_ui/templates/views-ui-display-tab-setting.html.twig, which has the following twig code:
{{ settings_links|safe_join('<span class="label">&nbsp;|&nbsp;</span>') }}

The links which are part of this issue, are put into that safe_join, and are now, after this change, a render array, so we need be able to render both a string and a render array in there as well.

The inline template itself, looks fine, so all we need is a test

working on the test.

@pjonckiere thanks!

I think we need to create a FilterUITest and move the filter handler UI test there. So my test is different... also using test_filter_groups with this test.

The patch verifies if we have an array or not before escape.

1. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -1143,7 +1143,15 @@ public function getFormBucket(ViewUI $view, $type, $display) {
   -            $store[$pid]['#link'] .= '  ' . ($group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND'));
   +            $operator = ($group_info['groups'][$gid] == 'OR') ? $this->t('OR') : $this->t('AND');
   +            $store[$pid]['#link'] = array(
   ...
   +                'link' => $store[$pid]['#link']
   

   One way that would work without inline templates is:

   $store[$pid]['#link'] = SafeMarkup::format('!link    !operator', ['!link' => $store[$pid]['#link'], '!operator' => $operator]);
   

   Because link should be safe to output and operator is safe to output (as it came via $this->t), the whole thing should work.

2. +++ b/core/themes/engines/twig/twig.engine
   @@ -158,9 +158,14 @@ function twig_drupal_join_filter($value, $glue = '') {
   +      // Rendering the array in the case that we're using the inline_template.
   +      $output .= $separator . render($item);
   

   That's not the right fix.

   And I don't think we want to support that for drupal_join.

   This should be calling twig_drupal_escape_filter (which was recently moved to the twig extension).

   If we wanted to do that at all ...

@Fabianx thanks!

So let's try it then.

@pjonckiere sure!

Please add author attribution to @pjonckiere and @b0unty because they were working on the same bug but from a different issue issue. Detected in #14. :)

Added proposed commit message.

RTBC, looks great now.

That's a pretty ugly bug -- good find. Thanks for the test-only patch as well. I just have a couple questions about the patch:

1. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,75 @@
   +   * Tests that "Limit list to selected items" option is saved as expected when
   +   * editing the Content type filter from the UI.
   

   Minor: This should be only a single line of fewer than 80 characters, so we can take a few words out or move details to a separate paragraph.

2. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -1143,7 +1143,8 @@ public function getFormBucket(ViewUI $view, $type, $display) {
   -            $store[$pid]['#link'] .= '  ' . ($group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND'));
   +            $operator = $group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND');
   +            $store[$pid]['#link'] = SafeMarkup::format('!link    !operator', ['!link' => $store[$pid]['#link'], '!operator' => $operator]);
   

   Why not just two separate full t()? I think the word order here could be affected by the language, which means we should give translators control over it, right? Rather than translating "AND" and "OR" separately with no context.

Also, per #22, I added credit for reviewers, but @b0unty has not commented on the issue so will need to be added to the commit message manually as per the summary.

@xjm thanks for reviewing it :)

Well, maybe this is the way to give translators control over it...

Short array syntax.

It is odd that you can mark pretty much everything es safe by using t() in connection with SafeMarkup::format(). Honestly, I don't see the advantage over the previous solution of
using an inline template, in case there is one obvious one, please let's update the documentation .. Its not that we call that kind of code extremely often, it a rare operation on some admin UI.

@dawehner The problem with the inline_template is that, in this case, we cannot render correctly.

at core/themes/engines/twig/twig.engine

$output .= $separator . SafeMarkup::escape($item);

We need render() instead SafeMarkup::escape() to render correctly.

To be honest I'm not sure why in other places we can use the inline_template and not here... I need to investigate about it...

Maybe @FabianX knows more about it...

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -1143,7 +1143,8 @@ public function getFormBucket(ViewUI $view, $type, $display) {
-            $store[$pid]['#link'] .= '  ' . ($group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND'));
...
+            $store[$pid]['#link'] = SafeMarkup::format('!link', ['!link' => $this->t('@link @operator', ['@link' => $store[$pid]['#link'], '@operator' => $this->t('   @operator', ['@operator' => $operator])])]);

This is still translating the "or" and "and" twice and then formatting them again after that. We should just have one t() and no SafeMarkup::format() call. Like:

if ($whatever == 'OR') {
 $thing = $this->t('OR stuff');
}
else {
  $thing = $this->t('AND stuff');
}

No need to nest the calls; this makes it harder for translators.

@xjm Yes, makes no sense at all...

SafeMarkup::format

   * In most cases, you should use t() rather than calling this function
   * directly, since it will translate the text (on non-English-only sites) in
   * addition to formatting it.

I still think that this is an abuse of t() as string concatenation but well.

1. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -1143,7 +1143,8 @@ public function getFormBucket(ViewUI $view, $type, $display) {
   +            $operator = $group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND');
   +            $store[$pid]['#link'] = SafeMarkup::format('!link', ['!link' => $this->t('@link @operator', ['@link' => $store[$pid]['#link'], '@operator' => $this->t('   @operator', ['@operator' => $operator])])]);
              }
   

   Why can't we do the following...

     $operator = $group_info['groups'][$gid] == 'OR' ? $this->t('OR') : $this->t('AND');
     $store[$pid]['#link'] = SafeMarkup::format('@link    @operator ', ['@link' => $store[$pid]['#link'], '@operator' => $operator]);
   

   I think this is better because '!link    OR' is more likely to result in errors than translating 'OR'
   

2. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,78 @@
   + * Contains \Drupal\views\Tests\Handler\FilterUITest.
   

   Should be... Contains \Drupal\views\Tests\FilterUITest

@alexpott, did you read #26 and #32?

@xjm pointed out that '!link    OR' and '!link    AND' at least gives some context.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 6b8cc66 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views_ui/src/Tests/FilterUITest.php b/core/modules/views_ui/src/Tests/FilterUITest.php
index 53a2d92..4d8f5fe 100644
--- a/core/modules/views_ui/src/Tests/FilterUITest.php
+++ b/core/modules/views_ui/src/Tests/FilterUITest.php
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Contains \Drupal\views\Tests\Handler\FilterUITest.
+ * Contains \Drupal\views\Tests\FilterUITest.
  */
 
 namespace Drupal\views_ui\Tests;

Fixed on commit.

For the record, @dawehner and I discussed the use of the t() in IRC. The correct thing to do would be to make the whole thing a template, but that's out of scope for fixing this bug.

My perspective is isn't an "abuse" of t(); it's exactly what t() is for -- to translate a word in context. In this case the context is short, but it is still a relevant. Languages do not translate word-for-word, and the little bit of context will help.

IMO the actual "abuse" is using   for formatting in the first place, but having it inside a t() is better than not using t() with context for the link placeholder. (Edited for nonsense typos and clarity.)