Replace !placeholder with @placeholder for non URLs in t() in Views, except for t() output that is used as an attribute value

mmmh

I can reproduce the double escaping manually. Just did the steps from the test and kaboom!

Didn't have time to figure out this more right now. I might take a look on this later if this haven't been solved by then :)

Would it be helpful or not helpful to break this up into "Views" and "Views UI"?

Whoever ends up getting started on this first, make whatever judgment call you want to make on #19. We can always split or merge the patches later if need be. The important thing is to get started.

Promoting to Critical for the same reason as #2566503-15: [meta] Replace remaining !placeholder for Non-URL HTML outputs only.

* Double escaping on admin/structure/views
* Views edit page looks great, both with aggregation enabled and disabled
* Escaping of tags on modal titles (might be HEAD)
* Deleting a display / view works fine
* Custom "administrative title"s for fields/filters are double escaped
* display title is getting xss filtered
* Administrative comment is escaped once, and just once

1. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -185,7 +185,7 @@ public function adminLabel($short = FALSE) {
        $title = ($short && isset($this->definition['title short'])) ? $this->definition['title short'] : $this->definition['title'];
   -    return $this->t('!group: !title', array('!group' => $this->definition['group'], '!title' => $title));
   +    return $this->t('@group: @title', array('@group' => $this->definition['group'], '@title' => $title));
   

   @group is escaped once, yeah, same with title. Perfect

2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -578,7 +578,7 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL, array $c
   -          $list[$id] = $this->t('!type language selected for page', array('!type' => $name));
   +          $list[$id] = $this->t('@type language selected for page', array('@type' => $name));
   
   @@ -588,7 +588,7 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL, array $c
   -            $list[$id] = $this->t('!type language selected for page', array('!type' => $type['name']));
   +            $list[$id] = $this->t('@type language selected for page', array('@type' => $type['name']));
   

   These are machine names, so this is totally fine

3. +++ b/core/modules/views/src/Plugin/views/area/HTTPStatusCode.php
   @@ -48,7 +48,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
        array_walk($options, function($title, $code) use(&$options) {
   -      $options[$code] = $this->t('@code (!title)', array('@code' => $code, '!title' => $title));
   +      $options[$code] = $this->t('@code (@title)', array('@code' => $code, '@title' => $title));
   

   Looks perfect

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1702,7 +1702,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -            '#markup' => $this->t('You may also adjust the !settings for the currently selected row style.', array('!settings' => $this->optionLink(t('settings'), 'row_options'))),
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected row style.', array('@settings' => $this->optionLink(t('settings'), 'row_options'))),
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected row style.', array('@settings' => $this->optionLink(t('settings'), 'row_options'))),
   

   Duplicated code

5. +++ b/core/modules/views/views.api.php
   @@ -531,7 +531,7 @@ function hook_field_views_data_alter(array &$data, \Drupal\field\FieldStorageCon
        'base' => $entity_type->getBaseTable(),
        'base field' => $entity_type->getKey('id'),
   -    'label' => t('!field_name', array('!field_name' => $field_name)),
   +    'label' => t('@field_name', array('@field_name' => $field_name)),
   
   @@ -588,7 +588,7 @@ function hook_field_views_data_views_data_alter(array &$data, \Drupal\field\Fiel
   -    'label' => t('!field_name', array('!field_name' => $field_name)),
   +    'label' => t('@field_name', array('@field_name' => $field_name)),
   

   Follow up: pointless t() call

6. +++ b/core/modules/views/views.views.inc
   @@ -449,7 +449,7 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
          $data[$table_alias][$field_alias]['aliases'] = $aliases;
   -      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
   +      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: @also.', array('@also' => implode(', ', $also_known)));
        }
   
   @@ -550,7 +550,7 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
            $data[$table_alias][$column_real_name]['aliases'] = $aliases;
   -        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
   +        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: @also.', array('@also' => implode(', ', $also_known)));
   

   Don't we loose the safety information again here, especially because $also_known is an array of t() resulsts

7. +++ b/core/modules/views/views.views.inc
   @@ -497,12 +497,12 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
        if (count($field_columns) == 1 || $column == 'value') {
   -      $title = t('@label (!name)', array('@label' => $label, '!name' => $field_name));
   +      $title = t('@label (@name)', array('@label' => $label, '@name' => $field_name));
          $title_short = $label;
   

   General idea, could we disable the serialization of unsafe TranslateWrapper instances?

8. +++ b/core/modules/views_ui/src/Form/BreakLockForm.php
   @@ -79,7 +79,7 @@ public function getDescription() {
   -    return $this->t('By breaking this lock, any unsaved changes made by !user will be lost.', array('!user' => drupal_render($username)));
   +    return $this->t('By breaking this lock, any unsaved changes made by @user will be lost.', array('@user' => drupal_render($username)));
   
   +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -139,14 +139,14 @@ public function form(array $form, FormStateInterface $form_state) {
   -        '!user' => drupal_render($username),
   -        '!age' => $this->dateFormatter->formatTimeDiffSince($view->lock->updated),
   +        '@user' => drupal_render($username),
   +        '@age' => $this->dateFormatter->formatTimeDiffSince($view->lock->updated),
   ...
            '#attributes' => array('class' => array('view-locked', 'messages', 'messages--warning')),
   -        '#children' => $this->t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="@url">break this lock</a>.', $lock_message_substitutions),
   +        '#children' => $this->t('This view is being edited by user @user, and is therefore locked from editing by others. This lock is @age old. Click here to <a href="@url">break this lock</a>.', $lock_message_substitutions),
            '#weight' => -10,
   

   Does anyone mind opening up a follow up replacing that with $user->getUsername() which is IMHO the right usage here?

9. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -1131,10 +1131,10 @@ public function getFormBucket(ViewUI $view, $type, $display) {
                if ($group_info['groups'][$gid] == 'OR') {
   -              $store[$pid]['#link'] = $this->t('!link    OR', ['!link' => $store[$pid]['#link']]);
   +              $store[$pid]['#link'] = $this->t('@link    OR', ['@link' => $store[$pid]['#link']]);
                }
                else {
   -              $store[$pid]['#link'] = $this->t('!link    AND', ['!link' => $store[$pid]['#link']]);
   +              $store[$pid]['#link'] = $this->t('@link    AND', ['@link' => $store[$pid]['#link']]);
   

   Checked that, this works, beside the previous mentioned double escaping

.

Don't we loose the safety information again here, especially because $also_known is an array of t() resulsts

Yes, we do. This is a fine example of how complex it can get with nested t()s. Below is an excerpt of existing code that is leading up to the t('Also known as: . We have three levels of nested t()s each with placeholders.

if (...) {
  $title = t('@label (!name)', array('@label' => $label, '!name' => $field_name));
}
else {
  $title = t('@label (!name:!column)', array('@label' => $label, '!name' => $field_name, '!column' => $column));
}

foreach (...) {
  $also_known[] = t('@group: @field', array('@group' => $group_name, '@field' => $title));
}

$data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));

So here is a thing, I think all beside the last t() call are actually pointless, IMHO.

I always thought that t calls with only placeholders like t('@label (!name), ... where useful for right to left languages. I checked Arabic and Hebrew, but both don't change the order of the placeholders in this kind of strings. They do change the order when text and placeholders are combined (but not always). I also checked for languages with different character sets (Russian, Japanese, Hindi) to verify if they use ':' and '(' unchanged. And they do. So, in the examples I have found, these placeholder-only strings don't need to be translated.

@Sutharsan
You indeed had the same idea as I had in the ViewsBlock conversion issue. Same problems and finally nobody actually remember why, beside tricking out safemarkup.

So that's true except for at least French, which does:

Something : something else.

Instead of

Something: something else.

With RTL the actual directionality of the browser should cover something like t('@label (!name)'), but I think we need to cover the colon translatability, or use something other than a colon.

So yeah, well the only problem is then

$data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));

which we could convert to inline templates, which meh, would slow down the cold cache (no idea about the numbers, but this scales with the amount of fields you have in the system).

working on fails, assuming this time they are real

Escaping of tags on modal titles (might be HEAD)

is #2207247: Dialog titles double escaped for views handlers and delete forms

first a reroll

Fixed the nits but couldn't fix the fails.

It looks like the second drupalGet in the UITestBase is hitting the parent site instead of the child, the 404 seen is from the parent site. Which explains the 404 - because the view doesn't exist.

If I enable the view on the parent site and add the block and configure it as per the test - the test passes.

Which indicates definitely hitting the parent site. But can't see anything in the patch that would effect that.

$data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));

which we could convert to inline templates, which meh, would slow down the cold cache

For the implode() part, do we not have a way to implode() safe strings and have the result be safe other than via a inline_template? If not, then yes, this needs an inline template, and perhaps we need a separate issue for having a PHP version of safe_join?

For the .= ' ' . part, might that not also depend on language? Should we fix that by making the t() broader, such as:

$primary_name = $data[$table_alias][$column_real_name]['help'];
$data[$table_alias][$column_real_name]['help'] = t('@primary_name Also known as: @also.', array('@primary_name' => $primary_name, '@also' => ...)));

Also, re #40, should we pull out things that require more thought into a separate issue, and only deal with the more straightforward ones here?

Also, re #40, should we pull out things that require more thought into a separate issue, and only deal with the more straightforward ones here?

+1 good idea!

Working on test failures...

The test was failing because we were trying to put an HTML-escaped string containing HTML-sensitive characters (<test>) in an HTML attribute. This is wrong as in that context we should not apply HTML markup sanitization:

<input type="submit" value="&lt;test&gt;">

That obviously cannot work and makes text assertions fail. I think we need to address #2509218: Ensure that SafeString objects can be used in non-HTML contexts first to fix those, the attached patch reverts the related changes for now.

Just in case anyone were wondering: unescaped < and > are legal characters inside attributes:

#48: oh wow!

So, the following source is legal, according to https://validator.w3.org/#validate_by_input:

<!DOCTYPE html>
<html>
<head lang="en">
  <title>Test</title>
</head>
<body>
  <h1 title="0 < 1 even for very big values of 0">Good morning, Vietnam!</h1>
  <p title="<span title=">Yo, Victor!</p>
</body>
</html>

However, as correctly pointed out by @stefan_r, XML will break. And the following is needed to make the validator happy:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE html
    PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head lang="en">
  <title>Test</title>
</head>
<body>
  <h1 title="0 &lt; 1 even for very big values of 0">Good morning, Vietnam!</h1>
  <p title="&lt;span title=">Yo, Victor!</p>
</body>
</html>

The code above works as intended (even in its HTML5 flavor) so I'd say I was wrong and we can just escape < and > and change the test assertions. Trying that.

[...] so I'd say I was wrong and we can just escape < and > and change the test assertions. Trying that.

Nope, code is actually double-escaped so we need #2509218: Ensure that SafeString objects can be used in non-HTML contexts.

Let's limit this issue to non-attribute-value output then. Retitling.

Cool, its progress!

For the implode() part, do we not have a way to implode() safe strings and have the result be safe other than via a inline_template?

@catch reminded me this morning that the preferred way to do this is:

'@also' => drupal_render([
  '#theme' => 'item_list',
  '#items' => $also_known,
  '#context' => ['list_style' => 'comma-list'],
])

Unless there's reasons why this is a bad idea for this particular part of Views (e.g., if the output is for an attribute value, or performance concerns about full rendering in this part of the pipeline).

Note that as a separate issue, I think we should consider allowing render arrays themselves as values of '@' placeholders, instead of requiring the caller to early render.

Sorry. Unintentional title change in #54.

@effulgentsia
We do have a couple of inline templates in core, should we then also use a proper render array instead?

+++ b/core/modules/views/views.views.inc
@@ -449,7 +449,7 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
-      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
+      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: @also.', array('@also' => implode(', ', $also_known)));
...
-        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
+        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: @also.', array('@also' => implode(', ', $also_known)));

Are we sure this isn't creating double escaping? If it's not, why isn't it?

It will probably indeed, see some of my earlier reviews. We though should also add some tests, just to be sure.

We do have a couple of inline templates in core, should we then also use a proper render array instead?

I guess we'd need separate issues to evaluate each one case by case. template_preprocess_search_result() and Field::renderItems() seem like possible candidates, though they don't use "," as the separator, so maybe that's why they don't use "comma-list"?

I'm not entirely sure where this is rendered out. Was assuming Content ID = also known as nid in the views UI would be it but that looks broken in head and this patch...

I could be wrong about the location of this Also known as concatenation. We can add double escape assertion if we can find where this alias help is rendered.

Yeah, it though would be on a configurable field, like the node body.

Steps to manually reproduce that:

Rename the label of the body field on articles to "Description". Once done the following thing appears:

Given that we talk about field labels, I don't think we will ever ever have HTML there

Even if someone renames a boolean field to Toggle <em> tag then the em tag should be treated as plain text and escaped, so I think that's fine.

Haven't reviewed all the rest of the patch yet so just leaving RTBC for now.

1. +++ b/core/modules/views/views.views.inc
   @@ -449,7 +449,7 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   -      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
   +      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: @also.', array('@also' => implode(', ', $also_known)));
   

   $also_know is an array of already escaped string - this is ripe for double escaping.

2. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -1131,10 +1131,10 @@ public function getFormBucket(ViewUI $view, $type, $display) {
            foreach ($contents as $key => $pid) {
              if ($key != $last) {
                if ($group_info['groups'][$gid] == 'OR') {
   -              $store[$pid]['#link'] = $this->t('!link    OR', ['!link' => $store[$pid]['#link']]);
   +              $store[$pid]['#link'] = $this->t('@link    OR', ['@link' => $store[$pid]['#link']]);
                }
                else {
   -              $store[$pid]['#link'] = $this->t('!link    AND', ['!link' => $store[$pid]['#link']]);
   +              $store[$pid]['#link'] = $this->t('@link    AND', ['@link' => $store[$pid]['#link']]);
                }
              }
   

   What is happening here?

$also_know is an array of already escaped string - this is ripe for double escaping.

As said earlier, the content will never have HTML.

Is there a follow-up for the t() return passed into Attributes that was split out of this patch? I can't find one mentioned anywhere, or in the queue.

As said earlier, the content will never have HTML.

What about quotes and ampersands?

What about quotes and ampersands?

Well, do you use them for field labels?

Tried to make some progress.. Maybe not the nicest solution ever

Ha, we have no tests at all for this functionality, this is for sure.

You run into the following error message with your approach:

User error: "group" is an invalid render array key in Drupal\Core\Render\Element::children() (line 102 of core/lib/Drupal/Core/Render/Element.php).
Drupal\Core\Render\Element::children(Array, 1)
Drupal\Core\Render\Renderer->doRender(Array, )
Drupal\Core\Render\Renderer->render(Array)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1)
__TwigTemplate_ac0bf388ce0f20d419e13e2b577986458872fa03a93dd170e625037c541cc147->doDisplay(Array, Array)
Twig_Template->displayWithErrorHandling(Array, Array)
Twig_Template->display(Array)
Twig_Template->render(Array)
Drupal\Core\Template\TwigEnvironment->renderInline('{% for item in items %}{% if not loop.first %}, {% endif %}{{ item }}{% endfor %}', Array)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func(Array, Array)
Drupal\Core\Render\Renderer->doRender(Array, 1)
Drupal\Core\Render\Renderer->render(Array, 1)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\Render\Renderer->renderPlain(Array)
views_field_default_views_data(Object)
views_views_data()
call_user_func_array('views_views_data', Array)
Drupal\Core\Extension\ModuleHandler->invoke('views', 'views_data')
Drupal\views\ViewsData->getData()
Drupal\views\ViewsData->get('node_field_data')
Drupal\views_ui\Controller\ViewsUIController->edit(Object, NULL)
call_user_func_array(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1)
Drupal\Core

This here is another classical place where you ideally want to construct just a string, because at that point in time its not yet meant for generating HTML. The level of outputting will be done much latter, so I actually think we should avoid to escape it here. We construct the string and then later could just deal with it.

Added some docs to the descape method. Thanks @dawehner for the added test coverage.

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -393,4 +393,8 @@ public static function escape($text) {
   +  public static function descape($text) {
   

   It's a bit sad that we need this but it'll solve our label conundrum :)

2. +++ b/core/modules/views/src/ViewsData.php
   @@ -10,6 +10,7 @@
   +use Drupal\Core\Cache\NullBackend;
   

   unneeded?

3. +++ b/core/modules/views/views.views.inc
   @@ -452,12 +453,8 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +      // Decode the entities, as they might have been escaped before.
   
   @@ -558,12 +555,8 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +        // Decode the entities, as they might have been escaped before.
   

   Might have been, or have been? The t() string has @placeholders, so unless those are safe (I don't think they are here), they'll be escaped.

4. +++ b/core/modules/views/views.views.inc
   @@ -452,12 +453,8 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: ') . Html::descape(implode(', ', $also_known));
   

   We have the comma list template for this, can't we use that?

5. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -393,6 +393,15 @@
   +   * Decodes HTML entities back to characters.
   

   Let's mention in the long description how it's the inverse of self::escape()?

   Should we also mention use cases along with a warning not to misuse this?

6. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -393,6 +393,15 @@
      public static function descape($text) {
   

   Let's add a unit test in HtmlTest

#72.3: It might change at some point and then we would have to change the comment
#72.4: It used to be implode and because of the fact that is concatenated into a string there is no real benefit to use the template
Fixed rest of #72.

Hmm, files lost again?

I think this is RTBC, asked @kgoel to have a final look at this and document in HtmlTest how they're inverse functions by doing some Html::descape(Html::escape((x))) == x

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -393,4 +393,19 @@ public static function escape($text) {
   +  public static function descape($text) {
   +    return htmlspecialchars_decode($text, ENT_QUOTES | ENT_SUBSTITUTE);
   

   Did we consider naming this method unescape instead? curl and JavaScript either have or used to have unescape (see below) and descape seems to be our own invention.

   http://curl.haxx.se/libcurl/c/curl_easy_unescape.html
   https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

2. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -393,4 +393,19 @@ public static function escape($text) {
   +   * @param $text
   +   *   The input text
   

   Minor: Missing period on the second line.

3. +++ b/core/modules/views/src/Tests/FieldApiDataTest.php
   @@ -26,9 +27,20 @@ protected function setUp() {
   +    // Attach the same to a different bundle with a different label.
   

   Minor: Attach the same what? In other words, I think we should say "same field" here.

4. +++ b/core/modules/views/views.api.php
   @@ -487,7 +487,7 @@ function hook_field_views_data(\Drupal\field\FieldStorageConfigInterface $field_
   -      'label' => t('image from !field_name', array('!field_name' => $field_storage->getName())),
   +      'label' => t('image from @field_name', array('@field_name' => $field_storage->getName())),
   
   @@ -531,7 +531,7 @@ function hook_field_views_data_alter(array &$data, \Drupal\field\FieldStorageCon
   -    'label' => t('!field_name', array('!field_name' => $field_name)),
   +    'label' => $field_name,
   
   @@ -588,7 +588,7 @@ function hook_field_views_data_views_data_alter(array &$data, \Drupal\field\Fiel
   -    'label' => t('!field_name', array('!field_name' => $field_name)),
   +    'label' => t('@field_name', array('@field_name' => $field_name)),
   

   Probably minor: Why is the second label not translated? Might relate to #25.5.

+1 for unescape instead of descape

1. +++ b/core/modules/views/views.api.php
   @@ -588,7 +588,7 @@ function hook_field_views_data_views_data_alter(array &$data, \Drupal\field\Fiel
   -    'label' => t('!field_name', array('!field_name' => $field_name)),
   +    'label' => t('@field_name', array('@field_name' => $field_name)),
   

   This t() should be removed, we are writing an assert that makes this unpossible:)

2. +++ b/core/modules/views/views.views.inc
   @@ -449,7 +453,8 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: ') . Html::descape(implode(', ', $also_known));
   

   This seems like a bunch of processing, (translate in __toString on TranslationWrapper, implode, de-escape. But worse is the : feels like it's missing context in the 'Also known as: t()'. Can we put that back in t('Also known as: @also')?

Fixed all the things except #80.2 which requires quite a few changes to make work without double escaping problems

Thank you for cleaning up the other issues. @lauriii #80.2 is a bit of a hard problem.

1. +++ b/core/modules/views/views.views.inc
   @@ -449,7 +453,8 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
   +      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: ') . Html::unescape(implode(', ', $also_known));
   

   I do like the function balancing out Html::escape() but I wonder if this issue needs to be the one to add it.

   Maybe this help should be built up some other way?

2. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
   @@ -512,6 +512,14 @@ td.group-title {
   +.views-ui-display-tab-setting > span {
   +  margin-left: 0.5em; /* LTR */
   +}
   +[dir="rtl"] .views-ui-display-tab-setting > span {
   +  margin-left: 0;
   +  margin-right: 0.5em;
   +}
   

   Can we get @lewisnyman to have a quick look at this CSS? May need a class instead of targeting the span tag.

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

   Should operators be translated, would they change since they are programming terms? They seem to me not to be translated because they won't change between language.
   Should we use a render array here to concatenate or inline template?

Should operators be translated, would they change since they are programming terms? They seem to me not to be translated because they won't change between language.

It feels like views is not a programming UI, but its something for sitebuilders. I think keeping the translatability is crucial.

I couldn't find the follow-up so added a stub #2571673: Convert Views t() usage where it is used as an attribute value.

+++ b/core/modules/views_ui/css/views_ui.admin.theme.css
@@ -512,6 +512,14 @@ td.group-title {
+.views-ui-display-tab-setting > span {
+  margin-left: 0.5em; /* LTR */
+}
+[dir="rtl"] .views-ui-display-tab-setting > span {

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -1130,12 +1130,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 <span>@operator</span>', ['@link' => $store[$pid]['#link'], '@operator' => $operator]);

That looks alright! Even you might think, this is not really underspecified CSS.

Addressing #89.1

We had the same problem in #2296929: Remove system_requirements() SafeMarkup::set() use and #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages(). Regardless of whether the patterns used there were the best possible fix, maybe instead of introducing yet another pattern let's just go with the same pattern here? It's a bit sad that we can't do t('something: @further_context') anymore, but we didn't do that in the linked issue either.

The unescape looks nice, but as it's not needed if we use markup arrays and the comma list template, that could go into its own issue if we want to do this instead.

+++ b/core/modules/views/views.views.inc
@@ -422,7 +419,17 @@
+      'help' => [
+        [
+          '#markup' => t('Appears in:'),
+          '#suffix' => ' ',
+        ],
+        [
+          '#theme' => 'item_list',
+          '#items' => $bundles_names,
+          '#context' => ['list_style' => 'comma-list'],
+        ]
+      ],

@@ -453,8 +460,18 @@
+        '#prefix' => '. ',
+        '#markup' => t('Also known as:'),
+        '#suffix' => ' ',
+      ];
+      $data[$table_alias][$field_alias]['help'][] = [
+        '#theme' => 'item_list',
+        '#items' => $also_known,
+        '#context' => ['list_style' => 'comma-list'],
+        '#suffix' => '.',
+      ];
+
     }
 
     $keys = array_keys($field_columns);
@@ -530,7 +547,17 @@

@@ -530,7 +547,17 @@
         'group' => $group,
...
+            '#markup' => t('Appears in:'),
+            '#suffix' => ' ',
+          ],
+          [
+            '#theme' => 'item_list',
+            '#context' => ['list_style' => 'comma-list'],
+            '#items' => $bundles_names,
+          ]
+        ],
       );
 
       // Go through and create a list of aliases for all possible combinations of
@@ -555,10 +582,17 @@

@@ -555,10 +582,17 @@
       }
       if ($aliases) {
         $data[$table_alias][$column_real_name]['aliases'] = $aliases;
-        // Decode the entities, as they might have been escaped before.
-        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: ') . Html::unescape(implode(', ', $also_known));
+        $data[$table_alias][$column_real_name]['help'][] = [
+          '#prefix' => ' ',
+          '#markup' => t('Also known as:'),
+          '#suffix' => ' ',
+        ];
+        $data[$table_alias][$column_real_name]['help'][] = [
+          '#theme' => 'item_list',
+          '#items' => $also_known,

I think its fundamental wrong what we do here. We don't want to generate HTML here.

@dawehner well then the problem is we use t() in a plain-text context, as t() is currently designed to work with markup. Which brings us back to #2509218: Ensure that SafeString objects can be used in non-HTML contexts.

Let's discuss with @alexpott & @xjm when we get to the CCIB

I found no additional remarks beyond #95. I think this is ready once it's green.

Just discussed this with Alex Pott, he's +1 to splitting off #95 into a separate issue. Created that issue: #2571935: Fix use of !placeholder for imploding in views.views.inc.

+++ b/core/modules/views/views.views.inc
@@ -460,18 +449,7 @@
-
+      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));

@@ -582,17 +550,9 @@
-          '#context' => ['list_style' => 'comma-list'],
-        ];
+        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
       }

Should we just add a test so we know exactly how the "expected" behaviour is?

@dawehner yes, maybe in #2571935: Fix use of !placeholder for imploding in views.views.inc as I don't think the current patch touches that part anymore?

Yeah good point. Let's get this in, once it is green!

Thanks drupalCI!

Committed 3f689d7 and pushed to 8.0.x. Thanks!

Changing to fixed, as it's already committed, and the test failed because the patch couldn't be applied.

This patch contains a lot of bugs.

1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1544,7 +1544,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected access restriction.', array('@settings' => $this->optionLink($this->t('settings'), 'access_options'))),
   

   This is incorrect code. This is correct way of doing this:

   t('You may also adjust the <a href=":settings">settings</a> for the currently selected access restriction.', ...
   

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1582,7 +1582,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected cache mechanism.', array('@settings' => $this->optionLink($this->t('settings'), 'cache_options'))),
   

   Same bug

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1654,7 +1654,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected style.', array('@settings' => $this->optionLink($this->t('settings'), 'style_options'))),
   

   Same again

4. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1702,7 +1702,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected row style.', array('@settings' => $this->optionLink($this->t('settings'), 'row_options'))),
   

   Same again

5. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1803,7 +1803,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected style.', array('@settings' => $this->optionLink($this->t('settings'), 'exposed_form_options'))),
   

   Same again

6. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1839,7 +1839,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +            '#markup' => $this->t('You may also adjust the @settings for the currently selected pager.', array('@settings' => $this->optionLink($this->t('settings'), 'pager_options'))),
   

   Same again

@hass
You are right about the fact that we do it wrong in views, but this issue about having a straight conversion to fix XSS issues, not about fixing views.
Do you mind open up a new issue?