Double escaped string on Available translation update page

I think that you can use an inline template:

From:

$string = t('My string');

To:

$string = array(
  'data' => array(
    '#type' => 'inline_template',
    '#template' => '{{ text }}',
      '#context' => array(
        'text' => t('My string'),
    ),
  )
);

Marking for Amsterdam. The patch is reviewable by experienced Drupal(contrib) people who know t(). Other tasks can be done by everyone (like updating the issue summary for the committers, because it has changed direction).

I could reproduce the bug in my local environment. After applying the last patch, the bug is resolved.
See screenshot:

The path follows the coding standards.

Since the core has changed, the patch doesn't apply anymore.
Re-rolled against latest head.

Oh, I just found this issue :(

We solved the same thing at https://www.drupal.org/node/2349749

Difference is: we used an inline_template.

I am not sure, which solution is the better way to go.

Just tested this solution with patch #10 locally, it worked and successfully solved the issue.

We looked into possible solutions in https://www.drupal.org/node/2349749 and actually i prefer this solution since it seem easier.

Those errors seem random. Re-testing...

Adding parent issue relationship.

OK; since this patch was already needs-review in #10 (the failure was indeed 'random' at busy Drupalcon A'dam) and people agree that this is the best solution...

... I did a quick review and can set RTBC, I think. Patch still applies with minimal offset + fuzz, and with a 2-line patch that should be OK.

OK I'll remember to reroll patches with minimal offset + fuzz next time. For the rest; as #18.

(checked things are still the same.)

+++ b/core/modules/locale/locale.pages.inc
@@ -125,7 +125,7 @@ function template_preprocess_locale_translation_update_info(array &$variables) {
-        $releases[] = t('@module (@version).', array('@module' => $update['name'], '@version' => $version)) . ' ' . $update['info'];
+        $releases[] = t('@module (@version). !info', array('@module' => $update['name'], '@version' => $version, '!info' => $update['info']));

I'm not sure why we are using t() here.

Using String::format()

Using String::format is a good change. I was about to RTBC this but only thing I'm concerned still is whether we can use multi line array syntax while setting parameter for the function call or not.

+++ b/core/modules/locale/locale.pages.inc
@@ -60,7 +61,10 @@ function template_preprocess_locale_translation_update_info(array &$variables) {
-        $releases[] = t('@module (@date)', array('@module' => $update['name'], '@date' => format_date($update['timestamp'], 'html_date')));
+        $releases[] = String::format('@module (@date)', array(
+          '@module' => $update['name'],
+          '@date' => format_date($update['timestamp'], 'html_date'),
+        ));

Why would String::format() not do the same escaping as t() does? I don't see how this is fixing the issue?

BTW the multiline array syntax is no problem.

We are not translating anything here. We are showing some strings in placeholder. I do not think we should use t() for that, String::format() would suffice in this case. That is what @alexpott told in #23 and we talked about this in IRC.

That does not answer my question. '@module (@date)' should escape the module name and date for output the same as it would in t() that you are removing. Switching from t() to String::format() should not change how the values are escaped and therefore should not fix this bug. Or what am I missing?

Hi, I have installed some modules in my Drupal environment and applied the patch. It appears to solve the problem, although I also don't understand why, Gábor.

Screenshot attached

This is to answer @Gábor's question of how the bug is being fixed.

-        $releases[] = t('@module (@date)', array('@module' => $update['name'], '@date' => format_date($update['timestamp'], 'html_date')));
+        $releases[] = String::format('@module (@date)', array(
+          '@module' => $update['name'],
+          '@date' => format_date($update['timestamp'], 'html_date'),
+        ));

This change is out-of-scope of this issue. It's ok to commit this too, but this string was escaped correctly before and after the patch.

-        $releases[] = t('@module (@version).', array('@module' => $update['name'], '@version' => $version)) . ' ' . $update['info'];
+        $releases[] = String::format('@module (@version). !info', array(
+          '@module' => $update['name'],
+          '@version' => $version,
+          '!info' => $update['info'],
+        ));

The change that fixes this issue is here, by moving the "!info" part into String::format (or t(), makes no difference). The double-escaping happened because only the '@module (@version).' part of the string was being marked as safe in t(), and when the whole string was printed, it was unrecognized and escaped again.

So I needed to go one step further in understanding. String::format() adds the string to the SafeMarkup storage, so it will not be escaped later. So bringing strings under that storage protects them from being escaped later again. Agreed with the RTBC then.

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 f758884 and pushed to 8.0.x. Thanks!

Superb, thanks all!

@herom++ Nice explanation. Thanks!!

I don't think this works anymore, seeing #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument landed.

Removed tag by mistake.

The issue reported is still gone with the committed patch. See image.

That surprised me given #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument, but all right :)