On /admin/reports/updates/update page, "Reccomended version" is double escaped as seen in screenshot:

double-escape-update

Proposed resolution

Check parent issue's guidelines (twig inline templates)

Files: 
CommentFileSizeAuthor
#38 twig_double_escaping_on-2349773-38.patch2.58 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,148 pass(es). View
#9 interdiff_4-9.txt2.47 KBZekvyrin
#9 interdiff_8-9.txt569 bytesZekvyrin
#9 core-twig_double_escaping_available_updates-2349773-9.patch2.31 KBZekvyrin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,555 pass(es). View
#10 Selection_002.png21.43 KBsubhojit777
#10 Selection_003.png23.06 KBsubhojit777
#18 core-twig_double_escaping_available_updates-2349773-18.patch2.35 KBmirom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,714 pass(es). View
#21 core-twig_double_escaping_available_updates-2349773-21.patch2.57 KBmirom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,855 pass(es). View
#23 interdiff.txt2.2 KBmirom
#30 core-twig_double_escaping_available_updates-2349773-30.patch2.57 KBmirom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,855 pass(es). View
#30 interdiff.txt2.06 KBmirom
#33 core-twig_double_escaping_available_updates-2349773-33.patch2.57 KBmirom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,856 pass(es). View
#33 interdiff.txt2.06 KBmirom

Comments

Zekvyrin’s picture

Issue summary: View changes

I'm working on a patch for the issue

Zekvyrin’s picture

Status: Active » Needs review
FileSize
1.41 KB

I'm uploading a patch for this. I used Twig inline_template render elements (option two from parent issue's recommendations).

$recommended_version is supposed to be safe, as every possibly unfiltered string passes either through an _l() or a t() function and is filtered there.

Zekvyrin’s picture

FileSize
33.34 KB

Also attaching and image for the fixed page:

Zekvyrin’s picture

FileSize
1.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,880 pass(es). View

For some reason I deleted the patch file (sorry)... reuploading..

iro’s picture

I did some manual testing and everything seems to work fine.

SteffenR’s picture

Status: Needs review » Reviewed & tested by the community

I also had a look on the issue - patch applies fine for me and the result is as expected.

I think we can set the issue to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
      $recommended_release = $project['releases'][$project['recommended']];
      $recommended_version = $recommended_release['version'] . ' ' . _l($this->t('(Release notes)'), $recommended_release['release_link'], array('attributes' => array('title' => $this->t('Release notes for @project_title', array('@project_title' => $project['title'])))));
      if ($recommended_release['version_major'] != $project['existing_major']) {
        $recommended_version .= '<div title="Major upgrade warning" class="update-major-version-warning">' . $this->t('This update is a major version update which means that it may not be backwards compatible with your currently running version.  It is recommended that you read the release notes and proceed at your own risk.') . '</div>';
      }

Can we convert this to an actual twig template rather than just adding html together. So it is something like

$template = "{{ release_version }} <a href="{{ release_link }}" title="{{ 'Release notes for'|t }} {{ project_title }}">{{ 'Release notes'|t }}</a>";
Zekvyrin’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,036 pass(es). View
2.17 KB

Hello Alex,

Thanks for the feedback & sample code. Helped me a lot. I've uploaded a new patch.

I have a few questions:
1) I'm still not sure if we can remove the drupal_render function from here. I tried and it didn't work.
2) Is it any way to call 't' filter with arguments in twig? For example we could use it to keep the same string as d7: 'Release notes for @project_title' .
3) I've noticed that there are some custom simple functions in twig about url generation. Can we implement something instead of using '<a>' in the template?

Zekvyrin’s picture

FileSize
2.47 KB
569 bytes
2.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,555 pass(es). View

Ok.. I should think a bit more like d7 sometimes.. I removed drupal_render from there.

uploaded new patch.

subhojit777’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
21.43 KB
23.06 KB

The code looks good, and follows @alexpott's suggestions. Tested the patch and it is working. Good work @Zekvyrin. Images attached for comparison.

subhojit777’s picture

Assigned: Zekvyrin » Unassigned
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -134,16 +134,26 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      $recommended_version = "{{ release_version }} (<a href=\"{{ release_link }}\" title=\"{{ 'Release notes for'|t }} {{ project_title }}\">{{ 'Release notes'|t }}</a>)";
...
+        $recommended_version .= "<div title=\"Major upgrade warning\" class=\"update-major-version-warning\">{{'This update is a major version update which means that it may not be backwards compatible with your currently running version.  It is recommended that you read the release notes and proceed at your own risk.'|t }}</div>";

There are numerous problems with this:

1. There is no way the translation extractor will find this. See #2315329: Support for Drupal 8 inline twig translatable for constructs the extractor can support. A random variable changed several ways and then later used in a render API construct will not suffice.

2. The words "Major upgrade warning" are not translatable. Not a problem of this patch, but while you are at it.

3. The change from "Release notes for @project_title" to essentially "Release notes for" . $project_title is very bad for translation. Are you sure all languages that Drupal may be translated to will find this word order suitable? The goal of having the token to replace in the text is exactly to support different languages. Doing the string concatenation instead will not be compatible by several languages.

subhojit777’s picture

Assigned: Unassigned » subhojit777
Zekvyrin’s picture

@Gábor: I totally agree with 3, but I couldn't find a way to solve it.

I haven't found any example of 't' usage with variables and i was wondering if it can be done. (issue that added t as a twig filter: #2011442: Support for Drupal 8 twig t filter translatables ).
Check my comment #8 (question no.2)

I also still wondering if we can implement a way to use a function to generate urls in these strings, and still avoid twig's autoescape.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
lauriii’s picture

#14:

{{ 'Author: @author'|t({'@author' : author})  }}

OR

{% trans %}
  Author: {{ author }}
{% endtrans %}
mirom’s picture

Assigned: Unassigned » mirom
Issue tags: +Drupal Camp RS
mirom’s picture

FileSize
2.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,853 pass(es). View
2.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,714 pass(es). View

Posting patch. Solving 2. and 3. from #12.

mirom’s picture

Status: Needs work » Needs review
lauriii’s picture

Looks good to me. Fixes #12.2 and #12.3. What are we gonna do with #12.1?

mirom’s picture

FileSize
2.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,855 pass(es). View

Should this works for it?

lauriii’s picture

mirom’s picture

FileSize
2.2 KB

Here u r

subhojit777’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          'project_title' => $this->t('Release notes for @project_title',array('@project_title'=>$project['title'])),

Space missing after first parameter, and around array key value. Change it to this:
$this->t('Release notes for @project_title', array('@project_title' => $project['title']))

subhojit777’s picture

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        $recommended_version = "{{ release_version }} (<a href=\"{{ release_link }}\" title=\"{{ project_title }}\">{{ release_notes }}</a>)";

Can't we use l() function here.

mirom’s picture

#25 - can I pass title parameter to l() function?

subhojit777’s picture

lauriii’s picture

I dont think we are using l() function in a template or am I wrong?

subhojit777’s picture

ahh.. I get it. In that case we do not need l() function.

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        '#context' => [

I am not sure whether this is correct. I think #context should contain elements within array(), like this:

'#context' => array(
...
),
mirom’s picture

FileSize
2.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,855 pass(es). View
2.06 KB

It is probably my typo. Submitting correct patch and interdiff with #18.

subhojit777’s picture

  1. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        $recommended_version = "{{ release_version }} (<a href=\"{{ release_link }}\" title=\"{{ project_title }}\">{{ release_notes }}</a>)";
    

    Indentation not correct

  2. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          'project_title' => $this->t('Release notes for @project_title',array('@project_title' => $project['title'])),
    

    Space missing after first parameter *again* :)

@mirom Please change it to needs review after it is done

Gábor Hojtsy’s picture

This approach definitely fixes the translatability concerns, thanks a lot.

mirom’s picture

FileSize
2.06 KB
2.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,856 pass(es). View

@subhojit777 - hopefully fixed, first time i didn't see, what you mean by "space after first parameter"

Submitting correct patch and interdiff with #18.

mirom’s picture

Status: Needs work » Needs review
jibran’s picture

Issue tags: +SafeMarkup
joelpittet’s picture

Status: Needs review » Needs work

This needs a re-roll, because it's in a table it's quite difficult to move this markup to a template to so this approach looks like the most viable.

Couple of nitpicks as well:

  1. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $recommended_version = "{{ release_version }} (<a href=\"{{ release_link }}\" title=\"{{ project_title }}\">{{ release_notes }}</a>)";
    ...
    +        $recommended_version .= "<div title=\"{{ major_update_warning_title }}\" class=\"update-major-version-warning\">{{ major_update_warning_text }}</div>";
    

    Would be nice without as many quote escapes, could you swap the " with '.

    "<div title=\"{ to '<div title="{

  2. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -134,16 +134,29 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          'major_update_warning_text' => $this->t('This update is a major version update which means that it may not be backwards compatible with your currently running version.  It is recommended that you read the release notes and proceed at your own risk.'),
    

    There is a double space after the period in the sentence.

joelpittet’s picture

Edit: Removed comment (wrong issue)

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,148 pass(es). View

Rerolled and did the changes

subhojit777’s picture

Assigned: mirom » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
6.96 KB

Patch in #38 is working. And the code looks alright.

subhojit777’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 169fde0 on 8.0.x
    Issue #2349773 by mirom, Zekvyrin, subhojit777, lauriii: Twig Double...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.