Problem/Motivation

If you have some safe strings and you want to safe_join them in a template and pass that as a @placeholder for translation, they get escaped even though they are safe. safe_join actually breaks the Markup object wrapper.

Why this should be an RC target

Without this patch any Markup instance passed to the |safe_join() Twig filter will be unintentionally escaped.

Disruptions: None, the intended result of this is to concatenate strings safely and return them joined by a separator and each item escaped if an item is not already a Markup instance.

Proposed resolution

Make the safe_join filter return a Markup object.

Remaining tasks

  1. Patch
  2. Tests
  3. Review

User interface changes

Less things getting accidentally escaped.

API changes

safe_join will return a TwigMarkup/Markup object rather than a string.

Data model changes

None

Why this should be an RC target

Expectations of how safe_join works with MarkupInterface will be way off and this is not disruptive.

Files: 
CommentFileSizeAuthor
#61 2579091_mod_dependencies_lists.png19.38 KBDuaelFr
#60 make_safe_join_twig-2579091-56-reroll.patch8.17 KBjoelpittet
#56 interdiff.2579091.52.56.txt413 bytesDuaelFr
#56 make_safe_join_twig-2579091-56.patch8.12 KBDuaelFr
#52 interdiff.2579091.50.52.txt1.33 KBDuaelFr
#52 make_safe_join_twig-2579091-52.patch8.18 KBDuaelFr
#50 interdiff.2579091.48.50.txt1.86 KBDuaelFr
#50 make_safe_join_twig-2579091-50.patch9.51 KBDuaelFr
#48 make_safe_join_twig-2579091-48.patch8.32 KBDuaelFr
#45 make_safe_join_twig-2579091-45.patch8.11 KBlauriii
#39 make_safe_join_twig-2579091-39.patch5.9 KBjoelpittet
#39 interdiff.txt799 bytesjoelpittet
#31 make_safe_join_twig-2579091-31.patch5.88 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,704 pass(es). View
#31 interdiff.txt1.52 KBjoelpittet
#30 make_safe_join_twig-2579091-30.patch6.27 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 66,977 pass(es), 26,521 fail(s), and 9,246 exception(s). View
#30 interdiff.txt6.29 KBjoelpittet
#22 2579091-22.patch16.93 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 66,200 pass(es), 25,929 fail(s), and 8,588 exception(s). View
#22 interdiff.txt1.91 KBdawehner
#12 make_safe_join_twig-2579091-12.patch2.19 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,282 pass(es). View
#12 make_safe_join_twig-2579091-12-test-only.patch1.16 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,258 pass(es), 1 fail(s), and 0 exception(s). View
#2 make_safe_join_twig-2579091-2.patch1.04 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,624 pass(es). View

Comments

Cottser created an issue. See original summary.

Cottser’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,624 pass(es). View
joelpittet’s picture

This seems like the appropriate take on this. RTBC after tests in place:)

Cottser’s picture

Status: Needs review » Needs work
dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -534,10 +535,10 @@ public function renderVar($arg) {
   public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
-    return implode($glue, array_map(function($item) use ($env) {
+    return Markup::create(implode($glue, array_map(function($item) use ($env) {
       // If $item is not marked safe then it will be escaped.
       return $this->escapeFilter($env, $item, 'html', NULL, TRUE);
-    }, $value));
+    }, $value)));
   }
 
 }

Given the domain I'm curious whether \Twig_Markup would be more correct to use here.

Cottser’s picture

That would be great but \Twig_Markup doesn't implement MarkupInterface so it doesn't really solve the problem I don't think.

Edit: Unless we allow \Twig_Markup wherever we allow MarkupInterface…

dawehner’s picture

Sure it doesn't, but you also doesn't need it, see core/lib/Drupal/Core/Template/TwigExtension.php:407

    if ($autoescape && ($arg instanceOf \Twig_Markup || $arg instanceOf MarkupInterface)) {
Cottser’s picture

That's not the problem this issue is trying to solve. It's trying to solve the interaction between Twig and SafeMarkup which ultimately leads to \Drupal\Component\Utility\SafeMarkup::isSafe():

  public static function isSafe($string, $strategy = 'html') {
    return $string instanceOf MarkupInterface;
  }

So to go with \Twig_Markup we'd need to update that as well and maybe other places. Then we are adding Twig stuff to Utility which I'm guessing is not OK.

dawehner’s picture

MH, well I don't get that. Where do we use the result of the twig level back outside of the level of twig?
Maybe it simply helps to write a test, but its not obvious for me why we need this. Do we use safe_join internally somehow?

Cottser’s picture

We want to use safe_join and be able to use it as a @placeholder. There is a failing patch at #2151109-52: Convert theme_system_modules_details() to Twig which kinda demonstrates the problem.

t() (t filter or trans tag) is one way that Twig things make their way outside of Twig.

lauriii’s picture

Assigned: Unassigned » lauriii

Writing tests for this

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,258 pass(es), 1 fail(s), and 0 exception(s). View
2.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,282 pass(es). View
dawehner’s picture

Huch, where do we use SafeJoin in that test?

lauriii’s picture

Copying code starting from TwigExtension line 175:

    $result = $twig_extension->safeJoin($twig_environment, $items, '<br/>');
    $this->assertEquals('&lt;em&gt;will be escaped&lt;/em&gt;<br/><em>will be markup</em><br/><strong>will be rendered</strong>', $result);

    // Ensure that result of safe join can be used in t() and string formatting
    // without double escaping.
    $result = new FormattableMarkup('Kittens @text', ['@text' => $result]);
    $this->assertEquals('Kittens &lt;em&gt;will be escaped&lt;/em&gt;<br/><em>will be markup</em><br/><strong>will be rendered</strong>', $result);

The last submitted patch, 12: make_safe_join_twig-2579091-12-test-only.patch, failed testing.

Cottser’s picture

I think it's just single escaping here, not double escaping.

Thanks for working on the tests @lauriii!

dawehner’s picture

Well, this almost sounds for me like TwigNodeTrans should use the same escaping strategy as normal twig and not use the one from t()? Well its tricky but doing things the wrong way, just because it works, potentially bites you on the longrun.

Cottser’s picture

@dawehner the t() filter does not go through TwigNodeTrans as far as I know so an approach like that would only get us so far. Other suggestions for solving this are welcome but right now it's a big WTF that safe_join actually loses safeness. Would adding our own TwigMarkup (or whatever you want to call it) that implements MarkupInterface make things any better?

lauriii’s picture

Issue tags: +blocker

This is blocking one of the last theme function conversions

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Tests in place and more kittens. @dawehner feel free to un-RTBC if you have a better solution but this one gets us to unblock some changes where we can use safe_join filter in the template more reliably.

The last submitted patch, 12: make_safe_join_twig-2579091-12-test-only.patch, failed testing.

dawehner’s picture

FileSize
1.91 KB
16.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 66,200 pass(es), 25,929 fail(s), and 8,588 exception(s). View

You know, I would try out something like this.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -16,6 +16,7 @@
@@ -534,10 +535,10 @@ public function renderVar($arg) {

@@ -534,10 +535,10 @@ public function renderVar($arg) {
    *   The strings joined together.
    */
   public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
-    return implode($glue, array_map(function($item) use ($env) {
+    return Markup::create(implode($glue, array_map(function($item) use ($env) {
       // If $item is not marked safe then it will be escaped.
       return $this->escapeFilter($env, $item, 'html', NULL, TRUE);
-    }, $value));
+    }, $value)));

Changed return value, let's document that.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2579091-22.patch, failed testing.

effulgentsia’s picture

I'm not so sure about #22. Maybe a code or issue comment can clarify?

Would adding our own TwigMarkup (or whatever you want to call it) that implements MarkupInterface make things any better?

Personally, I think a \Drupal\Core\Template\TwigMarkup that extends Twig_Markup and implements MarkupInterface would make a lot of sense.

The last submitted patch, 22: 2579091-22.patch, failed testing.

joelpittet’s picture

Still on board with #12. What is wrong with Markup and why does Twig need to be in the name?

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -148,8 +148,8 @@ public function getFunctions() {
+      new \Twig_SimpleFilter('t', [$this, 't'], array('is_safe' => array('html'))),
+      new \Twig_SimpleFilter('trans', [$this, 't'], array('is_safe' => array('html'))),

@@ -177,6 +177,10 @@ public function getFilters() {
+  public function t(\Twig_Environment $env, $string) {
+    return new \Twig_Markup(t($string), $env->getCharset());
+  }

@dawehner for that to work I thinkyou need to add:
'needs_environment' => TRUE

dawehner’s picture

Still on board with #12. What is wrong with Markup and why does Twig need to be in the name?

Well, you know its the interaction between layers. The twig layer should not have to pass its own internal objects to other systems, kind of similar to views, which should not use SafeString::create().

Personally, I think a \Drupal\Core\Template\TwigMarkup that extends Twig_Markup and implements MarkupInterface would make a lot of sense.

Sounds like a great suggestion even ideally you should not have to. Doing that consequently might require changes in quite some inner parts of twig.

joelpittet’s picture

@dawehner shouldn't have to change inner parts of Twig because all it's checks are for instanceof Twig_Markup. If you extend it should be fine.

The explanation:

Well, you know its the interaction between layers. The twig layer should not have to pass its own internal objects to other systems, kind of similar to views, which should not use SafeString::create().

Sounds like a recipe for cruft, because we already have quite a few SomethingStringInterface's laying about and recently added quite a few more. It feels like we are going down a messy path.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
6.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 66,977 pass(es), 26,521 fail(s), and 9,246 exception(s). View

Well let's see how this plays out... I removed the stuff to do with t() because it's a bit out of scope and t() already returns a MarkupInterface object.

joelpittet’s picture

FileSize
1.52 KB
5.88 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,704 pass(es). View

My unnecessary and wrong changes that parent class has already, removed.

The last submitted patch, 30: make_safe_join_twig-2579091-30.patch, failed testing.

The last submitted patch, 30: make_safe_join_twig-2579091-30.patch, failed testing.

joelpittet’s picture

Issue tags: +rc target triage

This produces unexpected results in the templates without this fix.

dawehner’s picture

Just a quick driveby review.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -541,14 +541,14 @@ public function renderVar($arg) {
+   * @return TwigMarkup

Should be "\Drupal\Core\Template\TwigMarkup"

joelpittet’s picture

@dawehner thanks, is that true if they are in the same namespace?

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary. "Unexpected results in the template" is a start, but kind of vague, and doesn't summarize the impact and disruption. :)

joelpittet’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
799 bytes
5.9 KB
joelpittet’s picture

Issue summary: View changes

Added RC target message to IS.

dawehner’s picture

I'm wondering whether we should have a test twig example as well?

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

I disagree with myself. This is not a bug of the integration between systems, but rather a problem in itself, so this means a unit test is the appropriate tool for the job.

lauriii’s picture

Rerolled & made this follow our BC policy about markup changes

Status: Needs review » Needs work

The last submitted patch, 45: make_safe_join_twig-2579091-45.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
8.32 KB

I just faced this issue while genuinely using the Zen theme that tries to use Components to build the page.
In the Drupal's html.html.twig file, it extends a component to render the <head> and pass it a title variable containing the safe_join'ed string we usually use. That string got escaped twice, reviving the good old &#039; issue if you have some quote in your title.

Just to see what happens, I rerolled #45 against 8.3.x.

Status: Needs review » Needs work

The last submitted patch, 48: make_safe_join_twig-2579091-48.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
9.51 KB
1.86 KB

It seems you were a bit too quick deleting some parts of that preprocess ;)
I also fixed the template in the stable theme.

Status: Needs review » Needs work

The last submitted patch, 50: make_safe_join_twig-2579091-50.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
1.33 KB

It seems that stable_preprocess_system_modules_details() does nothing more than template_preprocess_system_modules_details() now but breaking the tests by pre-rendering the required/missing modules.

The \Drupal\system\Tests\Module\DependencyTest test passes locally so let's see what the testbot think about the entire coverage.

GoZ’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigMarkup.php
@@ -0,0 +1,36 @@
+/**
+ * @file
+ * Contains \Drupal\Core\Template\TwigMarkup.
+ */
+

Class files should not have @file doc block.

Everything else seems OK. Thanks DuaelFr

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
9.44 KB
413 bytes

#53 Done

Please, ignore this patch.

GoZ’s picture

Status: Needs review » Needs work

Interdiff is wrong, and stable_preprocess_system_modules_details() is back in your patch

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
8.12 KB
413 bytes

My bad... I forgot to commit #52 on my local branch so I made my patch on #50. Stupid me.
That's fixed!

GoZ’s picture

Status: Needs review » Reviewed & tested by the community

Last patch passed and reviewed. Thx Duaelfr

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record for this. Plus are we sure about:

+++ b/core/modules/system/system.admin.inc
@@ -218,22 +217,11 @@ function template_preprocess_system_modules_details(&$variables) {
-      $requires = [
-        '#theme' => 'item_list',
-        '#items' => $module['#requires'],
-        '#context' => ['list_style' => 'comma-list'],
-      ];
-      $module['requires'] = $renderer->render($requires);
+      $module['requires'] = $module['#requires'];

+++ b/core/modules/system/templates/system-modules-details.html.twig
@@ -53,10 +53,10 @@
-                    <div class="admin-requirements">{{ 'Requires: @module-list'|t({'@module-list': module.requires }) }}</div>
+                    <div class="admin-requirements">{{ 'Requires: @module-list'|t({'@module-list': module.requires|safe_join(', ') }) }}</div>

This change. We're changing something from a proper html list to a comma separated list.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.17 KB

This looks like it just needs a CR. Rerolling.

DuaelFr’s picture

As @alexpott asked in #58 I'd like to ensure that we agree to replace these HTML lists to comma separated lists.

HTML lists are better for a11y because they allow to navigate from one item to the next easily. I'm not sure it's highly needed here but it's still better for people using a screen reader.

According to the answers here, I'll rewrite the patch to keep an HTML list in the modules dependencies, or not.

DuaelFr’s picture

Issue tags: +seville2017
DuaelFr’s picture

Issue tags: -seville2017 +DevDaysSeville

Messed with tags, sorry :/

lauriii’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record

I created change record for this. We still have to solve #58 & #61.