Problem/Motivation

#1825952: Turn on twig autoescape by default introduced autoescape and with it a mechanism to mark strings as safe. This works great as long as render arrays are used or only t() functions, but it begins to be a problem once user input is involved that needs to be concatenated or modified in some way:

Consider the following code:

$build['#markup'] = SafeMarkup::set('<span class="x">' . $my_maybe_secure_value . '</span>');

The problem here is that $my_maybe_secure_value might not be known if its secure at the moment. A way to mitigate that is to use:

$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::check($my_maybe_secure_value) . '</span>');

but that NEEDS to be done or a XSS will happen. We also discourage direct usage of SafeMarkup::set() as it is marked as internal only function, so we need something better.

This can get complicated very fast:

          $summary_escaped = '';
          $separator = '';
          foreach ($summary as $summary_item) {
            $summary_escaped .= $separator . SafeMarkup::escape($summary_item);
            $separator = '<br />';
          }
          $field_row['settings_summary'] = array(
            '#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . $summary_escaped . '</div>'),
          );

and escaping gets to be something that is difficult, discouraged and an uneasy task.

We could use for this particular example (if SafeMarkup::escape supported arrays - it does not):

          $summary_escaped = SafeMarkup::escape($summary);
          $field_row['settings_summary'] = array(
            '#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . implode("<br />", $summary_escaped) . '</div>'),
          );

which is way shorter, but still uses SafeMarkup::set directly, which module authors should never think about.

This would not a problem if the whole section used a template as we then would be getting the autoescape for free and the BIG and BEST solution is to move all those little markup things into a larger encapsulating template.

However we need something in between for until more code can be converted.

Proposed resolution

This issue proposes to introduce a new #type called twig_inline that allows safe creation of markup without calling SafeMarkup::set() directly. This also decreases the chance of single tags getting marked as safe as render arrays are usually just passed up un-rendered to the top page level.
(at least once the SafeMarkup::set() within drupal_render uses the $recursive flag)

Before:

$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::escape($possible_unsafe_var) . '</span>');

After:

$build['string'] = array(
  '#type' => 'inline_template',
  '#template' => '<span class="x">{{ var }}</span>',
  '#context' => array(
    'var' => $possible_unsafe_var,
  ),
);

This also has the advantage that we store less #markup within render arrays, which we also should discourage. While before the render array was only changeable with either re-creating the value or using str_replace, now the template can theoretically still be changed in larger encompassing templates or preprocess functions, which is a slight theming win, too. For forms a simple form_alter() can also be enough.

The more complicated example looks then like:

          $field_row['settings_summary'] = array(
            '#type' => 'inline_template',
            '#template' => '<div class="field-plugin-summary">{{ summary|safe_join("<br />") }}</div>',
            '#context' => array('summary' => $summary),
          );

safe_join is needed here which does automatic escaping before implode - comparable to the SafeMarkup::check() originally done. We had been debating to directly have |join always escape and be a safe filter, but that discussion is still ongoing.

As already discussed in the Motivation section the best would be to move as much of those to larger encompassing structures and templates, but this is probably the best we can do in the short term.

Remaining tasks

* Reviews and discussions.

User interface changes

* None.

API changes

* Addition of #type => 'twig_inline'.

Original report by Fabianx

I wondered if we should allow simple twig inline templates:

Before:

$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::escape($possible_unsafe_var) . '</span>');

After:

$build['string'] = array(
  '#type' => 'inline_template',
  '#template' => '<div>{{ var }}</div>',
  '#context' => array(
    'var' => $possible_unsafe_var,
  ),
);

That gives us autoescape for free and is compiled and cached to disk like normal templates, so pretty performant and not more difficult to use than t().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

A couple of questions:

Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

#1:

> Would that compile the string to disk and just load it using some hash?

Yes!

> In case it cannot be compiled is there any other way that twig does not have to parse the string on each corresponding request?

It is compiled to disk, so this is not a concern.

> OT: Did we considered #2300737: Register twig service as part of core.services.yml before ?

I don't think we had considered it. I guess it can just be moved over?

dawehner’s picture

Status: Active » Needs review
FileSize
6.46 KB

This turned out to be easier than thought, tbh.

Status: Needs review » Needs work

The last submitted patch, 4: 2289999.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.4 KB
3.93 KB
YesCT’s picture

YesCT’s picture

chx’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
FileSize
1.99 KB
8.29 KB

I like Yoda conditions and the De Morgan's laws as much as the next guy but I rather prefer not having them in core. That said, this is ready, my changes are cosmetic. Also this blocks an uncounted number of issues so bumping a bit.

chx’s picture

Status: Reviewed & tested by the community » Needs work

However, this doesn't create the render element in the IS so with a heavy heart resetting to CNW.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.07 KB
3.07 KB

There we go.

chx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Lovely!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2289999.patch, failed testing.

tstoeckler’s picture

+++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
@@ -25,6 +25,14 @@ public function testInlineTemplate() {
+    $this->assertEqual($environment->renderInlineTemplate('test-with-context {{lama}}', array('lama' => 'muuh')), 'test-with-context muuh');

Shouldn't the test be doing a drupal_render()?

That said also having a test that tests renderInlineTemplate() seems like a good idea, as well.

star-szr’s picture

This is awesome! Can we use Twig coding standards in the test please? Spaces inside the curly braces, {{ llama }} instead of {{llama}} :)

Test failure is unrelated I think, had it on another patch last night.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.18 KB
2.69 KB

Thanks @tstoeckler, cottser
It is impressive how much you can do wrong.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Well, none of us caught that from looking at it, so.... ....yay test coverage! :-)

Fabianx’s picture

This is wonderful!!! Thanks so much dawehner!

RTBC + 1

Do we need a change notice for this? Update an existing one?

catch’s picture

Status: Reviewed & tested by the community » Needs review

Won't this break if a site uses read-only PhpStorage and builds Twig templates as part of a build step? That's been proposed as one of the options for dealing with preventing writing PHP in production, not sure how you'd get past this (except for executing all the code that renders a template inline).

star-szr’s picture

FileSize
10.18 KB
671 bytes

Small coding standards fix. Somewhat crossposting with #19, I don't know the answer to that.

Fabianx’s picture

#19:

I think there are several possibilities:

a) Detect all twig inline templates, similar to how we can find t() strings - good DX, difficult but manageable build script; good performance.
b) Instead of cache to disk, cache to memcache or similar for inline templates if storage is read-only. Won't be as fast due to missing APC support, but still faster than compiling - good DX, less performance
c) Register inline templates centrally and use an index into that table - worse DX, but easy to find.

I am for a) as '#template' => 'abc', should not be too difficult to find within source files.

dawehner’s picture

Do we need a change notice for this? Update an existing one?

We do have one already: https://www.drupal.org/node/2306083

c) Register inline templates centrally and use an index into that table - worse DX, but easy to find.

Once we require this, it seems to be not far away from just writing a full template, tbh.

I think there are several possibilities:

Does that mean we don't have to care about that for core now? This would be a problem of people which don't want to use PhpStorage, which is part of core already.

chx’s picture

Status: Needs review » Needs work

> This would be a problem of people which don't want to use PhpStorage,

No, these people would use phpstorage but probably not the mtime variant -- for them the writing happens on staging and then the generated files get deployed to production so that production doesn't need writing PHP files.

But catch missed a crucial step here: we do not have anything to ensure all templates get generated on staging. That's left as an exercise to such developers already . They will likely need to visit a lot of pages or somehow get Twig to compile the templates. Now this mythical script will need to search for #template as well. My hand is up writing this script provided it doesn't need to happen this month.

Setting to CNW for adding a comment on renderInlineTemplate to not use it. You can always achieve what you want with a render array plus #template . Also to comment that #template must be a constant much like t() as Fabianx notes. Once #1617948: [policy for now] New standard for documenting form/render elements and properties happens we can put that comment on the element too.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.45 KB
797 bytes

Is this documentation good enough?

chx’s picture

Status: Needs review » Reviewed & tested by the community

That will do for now then when the new standard comes I will move it in place with an example.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Yes PhpStorage but read-only is what I should have written.

+   * On top of that you have to ensure that the template string is not dynamic
+   * but just an ordinary static php string.

This needs a 'why'.

I can live with the 'you have to do some extra work to compile inline templates' but can we please open an explicit follow-up. Compiling a bunch of stuff that you know lives in a finite number of places is a much easier task than this.

Fabianx’s picture

Proposal for extending that comment:

+   * but just an ordinary static php string,

+   * because there may be installations using read-only PHPStorage that want to generate all possible twig templates as part of a build step.
+   * So it is important that an automated script can find the templates and extract them. This is only possible if the template is a regular string.
Fabianx’s picture

Issue summary: View changes
dawehner’s picture

Assigned: Fabianx » Unassigned
Status: Needs work » Needs review
FileSize
10.75 KB
1.04 KB

I really liked the suggestion from Fabianx, so here is a patch with it.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Filed a child issue as catch asked; now both his comment and his issue is in place, let's try it one more time!

andypost’s picture

+++ b/core/modules/system/src/Tests/Theme/TwigEnvironmentTest.php
@@ -0,0 +1,46 @@
+    $element = array();
+    $element['test'] = array(
+      '#type' => 'twig_inline',
+      '#template' => 'test-with-context {{ lama }}',
+      '#context' => array('lama' => 'muuh'),
+    );
+    $this->assertEqual(drupal_render($element), 'test-with-context muuh');

auto-escape is on now, suppose test should check that too

Dries’s picture

It's not clear what the real reason for this change is. Is it performance? Is it DX? (I personally find the 'Before' easier to understand/read/write than the 'After'.)

I'd love to see a couple of real use cases in the patch; some conversions in the patch would make it easier to evaluate.

chx’s picture

Status: Reviewed & tested by the community » Needs work
RainbowArray’s picture

I'm really not a fan of this.

We've been doing a lot of work to convert theme functions to use Twig templates instead. That has some great benefits for themers, because they can find the appropriate Twig template with Twig debug and copy it into their theme if they want to override the markup.

With this, markup will be back in functions once again, and not necessarily in a place where it's possible to override the markup. That means themers digging through PHP in order to find out if there's any way to override markup. It's extraordinarily annoying right now when you find markup that's hard-coded in some contrib module in a way that's nearly impossible to override without hacking the module. This seems like it would make it even more likely for that to happen.

The particular example is with a render array, so I may be completely off base here, as that should be something a themer can modify, although probably only with digging into PHP. And there may well be other instances of markup making its way directly into render arrays rather than going through a template. So I might be completely off-base. If I am, please ignore me. Just sharing my initial impression.

chx’s picture

Are you ready to mandate a separate Twig template then for every instance of #prefix and #suffix in core or similar?

RainbowArray’s picture

Like I said, I might not be aware of everything that's going on. If there's already other markup not in file-based Twig template, then my concerns are moot.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.7 KB
2.96 KB

Converted two places: One which is pretty straightforward: views UI, and one which can replace a foreach loop

Fabianx’s picture

Status: Needs review » Needs work

#32: I will update the issue summary.

#34: You have a point here, but the problem is that so much HTML is still hardcoded in core, but it is difficult to just move that to larger encapsulation templates with ease and we need something that is still discouraged, but not as difficult or unsafe to use as SafeMarkup::set(). I'll explain in the issue summary.

#37:

+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -404,14 +404,10 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
-            '#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . $summary_escaped . '</div>'),
+            '#type' => 'twig_inline',
+            '#template' => '<div class="field-plugin-summary">{{ summary|join("<br />") }}</div>',
+            '#context' => array('summary' => $summary),

This should be using safe_join however, to do the same as the loop above.

marcvangend’s picture

As a site-building themer with development skills (doing a bit of everything), I am in favor of this patch.

In reply to mdrummond's concerns: Developers who don't care about templates, overrides and doing things The Drupal Way™, can already use '#markup' => '<div>foo</div>'. I agree we don't want to see that kind of code in core or contrib, but discarding this patch will not stop anyone from doing it wrong. Meanwhile the patch will reduce the chance that markup-in-code introduces security issues, so that's a win.

Dries: To me, the main advantage of this patch is the DX when writing custom code. In D7, #markup has proven to be a really useful tool in my custom modules. I use it for rapid prototyping (just whack the HTML mockup in there, so the client can see it in the demo, and refactor later), for quick fixes (sometimes every minute counts), or simply when you're certain that your output does not need to be overridden. This patch would allow me to combine that with the advantages of twig.

Fabianx’s picture

Issue summary: View changes

I updated the issue summary to include more motivation and discussion of the various points.

star-szr’s picture

The example in #37 (Field UI) makes it very clear to me the benefit of this. Nice one @dawehner :)

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -679,8 +679,8 @@ public function renderPreview($display_id, $args = array()) {
-              SafeMarkup::set('<strong>' . t('Query') . '</strong>'),
...
+              array('data' => array('#type' => 'twig_inline', '#template' => '<strong> {% trans "Query" %} </strong>')),

@@ -693,14 +693,14 @@ public function renderPreview($display_id, $args = array()) {
-                SafeMarkup::set('<strong>' . t('Other queries') . '</strong>'),
+                array('data' => array('#type' => 'twig_inline', '#template' => '<strong> {% trans "Other queries" %} </strong>')),
...
-              SafeMarkup::set('<strong>' . t('Title') . '</strong>'),
+              array('data' => array('#type' => 'twig_inline', '#template' => '<strong> {% trans "Title" %} </strong>')),

I don't think {% prints anything, I think for all these since they are short you want something like:

{{ "Query"|t }}

Also we shouldn't add additional whitespace inside the strong tags for these, it wasn't there before.

star-szr’s picture

…well we usually use single quotes, so {{ 'Query'|t }} :)

LewisNyman’s picture

Are you ready to mandate a separate Twig template then for every instance of #prefix and #suffix in core or similar?

Isn't this what template suggestions or twig blocks are for?

The first objective of #2289511: [meta] Results of Drupalcon Austin's Consensus Banana is to move mark up into templates and out of PHP. Maybe it needs to be possible to do this to cover custom module edge but we shouldn't encourage this in core or contrib because this causes massive pain, requiring themers to wade through php and learn a load of Drupalisms to change markup.

star-szr’s picture

Issue summary: View changes

From the issue summary:

This would not a problem if the whole section used a template as we then would be getting the autoescape for free and the BIG and BEST solution is to move all those little markup things into a larger encapsulating template.

However we need something in between for until more code can be converted.

This is a stepping stone and way better than #markup IMO.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.7 KB
2.01 KB

It does work without the print statement:

    protected function doDisplay(array $context, array $blocks = array())
    {
        // line 1
        echo "<strong> ";
        echo t("Other queries", array());
        echo " </strong>";
    }

    public function getTemplateName()
    {
        return "<strong> {% trans \"Other queries\" %} </strong>";
    }
The first objective of #2289511: [meta] Results of Drupalcon Austin's Consensus Banana is to move mark up into templates and out of PHP. Maybe it needs to be possible to do this to cover custom module edge but we shouldn't encourage this in core or contrib because this causes massive pain, requiring themers to wade through php and learn a load of Drupalisms to change markup.

https://www.drupal.org/node/2289999#comment-8994155 also explained why this should be useful in general during development.

Fabianx’s picture

#45: Could you still address using safe_join instead of join? Thanks! (explained in #38)

chx’s picture

FileSize
13.7 KB
732 bytes
LewisNyman’s picture

This is a stepping stone and way better than #markup IMO.

Agreed. Thumbs up as a stepping stone.

dawehner’s picture

Great that you agreed.

@Fabianx
It is up to you to RTBC again.

star-szr’s picture

#45, thanks for that. Didn't know that :) I double checked #2040089: Support for Drupal 8 twig %trans% template translatables and it supports that syntax so that works.

RainbowArray’s picture

Thumbs up to RTBC.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC

tim.plunkett’s picture

+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -404,14 +404,10 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
-            '#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . $summary_escaped . '</div>'),
+            '#type' => 'twig_inline',
+            '#template' => '<div class="field-plugin-summary">{{ summary|safe_join("<br />") }}</div>',
+            '#context' => array('summary' => $summary),

Can we get some clarification that this is an optional change, and not everyone will be forced into this? What horrible DX...

alexpott’s picture

Issue tags: +Needs change record

Also I think we need a change record for this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
dawehner’s picture

as written in IRC I disagree with making #safe_markup ... we should better discourage people from writing unsecure code, always,
but rather encourage to write proper templates.
Based on this for me it would be fine with having no #safe_markup functionality or something similar at all.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Edit: nevermind, I forgot my place. It is not mine to argue anything any more.

chx’s picture

Status: Reviewed & tested by the community » Needs work
joelpittet’s picture

I really like a bunch of things in this patch. Nice work @Fabianx!

Don't want to derail this too much but was wondering your thoughts on something a bit more inline with the way twig does inline templates?

print Drupal::service('twig')->renderInline('Hello {{ name }}!', array('name' => 'FabianX'));

Do these inline templates need to go through the drupal_render pipeline?

FYI I'm referencing/plugging my quick loader swap hack from the Twig lab in Austin:
https://github.com/DrupalTwig/axe/blob/master/src/Controller/AxeControll...

And would love to avoid any new #type/#theme/render array if possible:)

joelpittet’s picture

#59 never mind #59 I just realized I can do what I was asking...

print \Drupal::service('twig')->renderInlineTemplate('test-with-context {{ lama }}', array('lama' => 'muuh'));

And do share the DX concerns but mostly like I said before, I don't like #type. But this looks like an improvement to me:

+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -404,14 +404,10 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, Ent
         if (!empty($summary)) {
-          $summary_escaped = '';
-          $separator = '';
-          foreach ($summary as $summary_item) {
-            $summary_escaped .= $separator . SafeMarkup::escape($summary_item);
-            $separator = '<br />';
-          }
           $field_row['settings_summary'] = array(
-            '#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . $summary_escaped . '</div>'),
+            '#type' => 'twig_inline',
+            '#template' => '<div class="field-plugin-summary">{{ summary|safe_join("<br />") }}</div>',
+            '#context' => array('summary' => $summary),
             '#cell_attributes' => array('class' => array('field-plugin-summary-cell')),
           );

@FabianX could I propose renderInlineTemplate be named renderInline()? Because render() expects a template and I'd like to use that method directly(selfish i know) for some fun. And 'twig_inline' be renamed to 'inline_template'?

No worries if you disagree, just thought I'd ask.

chx’s picture

What this patch offers (not because I want to disagree but because it seems this is not clear to others): since the template+variable is not fused into a string, the "clean 1/3" from #2289511: [meta] Results of Drupalcon Austin's Consensus Banana can write a theme which alters #template to not contain div etc.

Of course, everything in a template is better. But, see #38 on the feasibility.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.71 KB
6.38 KB

@FabianX could I propose renderInlineTemplate be named renderInline()? Because render() expects a template and I'd like to use that method directly(selfish i know) for some fun. And 'twig_inline' be renamed to 'inline_template'?

Done

Fabianx’s picture

Status: Needs review » Needs work

I am happy with the rename, we still need a change record.

#60: You can do whatever you please, but please bear in mind the translation tools might need other changes to also work with ->renderInline() - that is why we discourage direct usage at the moment.

There is two possibilities for the simpler cases to be _clean_ for themers if one does not write a clean template from the beginning.

$build['x'] = array(
  '#prefix' => SafeMarkup::set('<div>'),
  '#markup' => t('abc'),
  '#suffix' => SafeMarkup::set('</div>'),
$build['x'] = array(
  '#type' => 'inline_template',
  '#template' => '<div>{{ var }}</div>',
  '#context' => array(
    'var' => $var,
  ),
);

Anything else will not allow a themer to change the output properly, but we still want to discourage usage of SafeMarkup::set as much as possible.

So if someone says this is 'horrible DX' or "don't like this", they need to fix their module to work properly with templates, because what they had before was horrible TX (themer experience)!

If you do it right from the start, you will never need to use this and writing object oriented templates and using suggestions should not be more difficult than writing object oriented classes.

Summary

This approach is secure by default, more themer friendly than before, achieves the 1/3 of clean-markup-themers goal and you will never need to use this if you do it right.

It is a stepping stone.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Here is one: [#2311123]

Fabianx’s picture

Issue summary: View changes
joelpittet’s picture

Issue tags: -Needs change record

RTBC++ thank you @Fabianx and thanks @dawehner for the change record. I tweaked the title and linked it up. Pity change records don't magically convert to urls like issues do. Here's the link.

https://www.drupal.org/node/2311123

dawehner’s picture

It was a pleasure to write the change record ;)

chx’s picture

Similarly to rewrite it ;)

Fabianx’s picture

Thanks, chx!

tstoeckler’s picture

Not sure how relevant this is but AFAIK the following snippet in #63 is incorrect:

$build['x'] = array(
  '#prefix' => SafeMarkup::set('<div>'),
  '#markup' => t('abc'),
  '#suffix' => SafeMarkup::set('</div>'),

I'm pretty sure that the SafeMarkup call is in fact not needed for #prefix and #suffix. See also line 3279 of common.inc in drupal_render()

Fabianx’s picture

#70, you are right - I missed that, but that was probably an oversight as we either:

a) debate whether #prefix, #suffix should be considered safe similar to #markup vs. #safe_markup
b) Add a SafeMarkup::check() on it during drupal_render()

We will probably address as part of "Check and document all SafeMarkup" calls in core, though, so for now this leaves another option - if #markup is checked currently in drupal_render() - if its not that's a security issue again again in the issue summary and another reason why "inline_template" is just safer to use as it guarantees the escaping in an unobtrusive way.

Fabianx’s picture

As per #70, code would currently need to be:

$build['x'] = array(
  '#prefix' =>'<div>',
  '#markup' => SafeMarkup::check($possibly_unsafe_var),
  '#suffix' => '</div>',

as we currently do not escape #markup. (@see #2273925: Ensure #markup is XSS escaped in Renderer::doRender()) and #prefix, #suffix.

I did just in my example, expect it to be safe, so I fell into the security trap as well, which as already said is another good point for 'inline_template' and #markup being escaped by default. If even core devs that wrote the autoescape part, can't get this right (aka me in this case) ...

andypost’s picture

The prefix/suffix primary used in forms to add wrapping DIV with ID to allow core's ajax work.
IMO this example is more confusing then the issue.

Can someone explain how this would help to fix all double-escape issues that postponed on this one?
Would be great to provide example based on #2309929: HTML double-escaping in field forms

effulgentsia’s picture

Assigned: Unassigned » Dries

Since this is RTBC again, assigning the commit evaluation to Dries due to #32, but that doesn't need to stop additional feedback and responses by others in the meantime.

catch’s picture

I think Dries' concerns were adequately addressed in the remaining comments, and this is blocking all the double-escaping issues at the moment, so I'd suggest we unassign on Monday if there's no feedback from Dries by then.

Dries’s picture

Assigned: Dries » Unassigned

Thank you for the sample conversions and the excellent issue summary and change record. I agree that removing usages of SafeMarkup::set() and required calls to SafeMarkup::check() is a DX win, and helps people avoid security bugs from forgetting to SafeMarkup::check().

I don't think that inline Twig strings inside of PHP code is good DX, but it seems like everyone here agrees that the solution to that is to move that markup to template files in followups as we're able to, and that works for me.

I haven't reviewed the changes to TwigEnvironment in detail, so I'm unassigning myself so that catch or alexpott can commit this once they've done that.

Anonymous’s picture

What if I don't use Twig? Changing templates is one thing but changing the code is something totally different. Though I understand why this is needed I don't think this is a good solution:

$build['string'] = array(
  '#type' => 'inline_template',
  '#template' => '<span class="x">{{ var }}</span>',
  '#context' => array(
    'var' => $possible_unsafe_var,
  ),
);

How about something in this direction:

$build['string'] = array(
  '#type' => 'inline_template',
  '#template' => '<span class="x">[mytoken] or [mytoken|t]</span>',
  '#engine' => 'twig', // <-- value would be taken from active theme, no need to set this manually
  '#context' => array(
    'mytoken' => $possible_unsafe_var,
  ),
);

+ parser for each template engine that, in case of Twig, would change '<span class="x">[mytoken] or [mytoken|t]</span>' into '<span class="x">{{ mytoken }} or {{mytoken|t }}</span>' and in case og phpengine to '<span class="x">mytoken or t(mytoken)</span>'.

dawehner’s picture

@ivanjaros
This is not the point. The twig services will be always available in the DIC. You don't and actually can't directly replace this snippets inside your theme.
Beside from that, building an abstraction over template engines is well described in http://xkcd.com/927/ . You could also easily build a parser for twig and convert it
to your wanted language instead of inventing a new one.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 76608ff and pushed to 8.x. Thanks!

diff --git a/core/lib/Drupal/Core/Template/TwigEnvironment.php b/core/lib/Drupal/Core/Template/TwigEnvironment.php
index 6c2f673..d8da1ca 100644
--- a/core/lib/Drupal/Core/Template/TwigEnvironment.php
+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -47,7 +47,7 @@ public function __construct(\Twig_LoaderInterface $loader = NULL, $options = arr
 
     // Ensure that twig.engine is loaded, given that it is needed to render a
     // template because functions like twig_drupal_escape_filter are called.
-    require_once 'core/themes/engines/twig/twig.engine';
+    require_once DRUPAL_ROOT . '/core/themes/engines/twig/twig.engine';
 
     // Set twig path namespace for themes and modules.
     $namespaces = array();
diff --git a/core/modules/field_ui/src/DisplayOverviewBase.php b/core/modules/field_ui/src/DisplayOverviewBase.php
index f7ca879..9ceddab 100644
--- a/core/modules/field_ui/src/DisplayOverviewBase.php
+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -8,7 +8,6 @@
 namespace Drupal\field_ui;
 
 use Drupal\Component\Plugin\PluginManagerBase;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Entity\Display\EntityDisplayInterface;
diff --git a/core/modules/views_ui/src/ViewUI.php b/core/modules/views_ui/src/ViewUI.php
index c6aafa1..faa7507 100644
--- a/core/modules/views_ui/src/ViewUI.php
+++ b/core/modules/views_ui/src/ViewUI.php
@@ -8,7 +8,6 @@
 namespace Drupal\views_ui;
 
 use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\Timer;
 use Drupal\Component\Utility\Xss;
 use Drupal\Core\Form\FormStateInterface;

Fixed on commit.

  • alexpott committed 76608ff on 8.0.x
    Issue #2289999 by dawehner, Cottser | Fabianx: Add an easy way to create...
Gábor Hojtsy’s picture

I see there was minor discussion of how this is compatible with translations. Looks like both the inline_template and the renderInline() would need specific support in potx. We are not looking at PHP files for Twig translatable text of course. So we need reliable ways to identify these template fragments and then parse them as Twig. See #2315329: Support for Drupal 8 inline twig translatable, help appreciated there in figuring out what are the possible forms. The change notice explains two forms: https://www.drupal.org/node/2311123, can we rely on such specific things as an array defined all at once with type key before the template key? Ie. if you define it the other way around it will not be translatable? Consider we need to statically parse this PHP code to then statically parse the Twig code. See? Discussion should happen in #2315329: Support for Drupal 8 inline twig translatable, thanks again.

effulgentsia’s picture

The change notice explains two forms

Specifically, the change notice says:

The recommended way is to use the render element but you can also service directly, if unavoidable: \Drupal::service('twig')->renderInline('{{ var }}', array('var' => $possible_unsafe_var))

But, the PHPDoc for renderInline() says:

Warning: You should use the render element 'inline_template' together with the #template attribute instead of this method directly.

So, what's an example of an "unavoidable" situation that the change notice is referring to that would justify violating the PHPDoc of the method? If there is one, we should probably add that information to the PHPDoc and change record. If there isn't, then that's one less bit of work for #2315329: Support for Drupal 8 inline twig translatable.

dawehner’s picture

Meh, I just didn't wanted to write the same string twice. The situation feels similar to #attached and _drupal_add_css.

larowlan’s picture

Status: Fixed » Closed (fixed)

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