Problem/Motivation

Noticed in #2226493: Apply formatters and widgets to Node base fields. Right now you get warnings and errors.

Right now {% trans %} only allows strings to be passed in and using render arrays is not possible. Being able to use render arrays in node.html.twig would allow us to simplify template_preprocess_node().

Proposed resolution

Change this line:
$compiler->string($token->getAttribute('placeholder'))->raw(' => ')->subcompile($token)->raw(', ');
in TwigNodeTrans.php

API changes

Allows common values to be rendered in Trans tag.

Before

After

CommentFileSizeAuthor
#61 trans_does_not-2334319-61.patch5.3 KBCottser
#57 trans-support-render-array-2334319-57.patch5.37 KBjoelpittet
#57 interdiff.txt2.18 KBjoelpittet
#57 trans-support-render-array-2334319-57-test-only-fail.patch4.66 KBjoelpittet
#50 trans_does_not-2334319-50.patch5.81 KBandypost
#38 before-patch.png34.64 KBjoelpittet
#38 After_Patch.png33.69 KBjoelpittet
#33 interdiff.txt3.17 KBDenchev
#33 trans_does_not-2334319-33.patch5.81 KBDenchev
#31 interdiff.txt2.03 KBDenchev
#31 trans_does_not-2334319-31.patch5.05 KBDenchev
#29 interdiff.txt1.21 KBDenchev
#29 trans_does_not-2334319-29.patch4.67 KBDenchev
#25 interdiff.txt1.41 KBDenchev
#25 trans_does_not-2334319-25.patch4.78 KBDenchev
#23 interdiff.txt533 bytesDenchev
#23 trans_does_not-2334319-23.patch4.78 KBDenchev
#20 interdiff.txt4.25 KBDenchev
#20 trans_does_not-2334319-20.patch4.26 KBDenchev
#16 core-trans_render_array_support-2334319-16.patch858 bytesDenchev
#16 interdiff.txt861 bytesDenchev
#14 core-trans_render_array_support-2334319-14.patch870 bytescalebtr
#8 core-trans_render_array_support-2334319-8.patch873 bytesDenchev
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,196 pass(es), 4 fail(s), and 0 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joelpittet’s picture

Issue tags: +D8MI

Translation is trying to avoid dealing with complex objects. There are comments to that effect. Can we render the render array before providing it the trans block's context?

It's intended to work as the t() does as much as possible and same for t() it won't render a Renderable Array. If we do need to support render arrays in trans (which I kinda hope we don't). Maybe we should do that at the t() level?

joelpittet’s picture

Is this the error you are getting BTW?

TwigTransTokenParser:105?

throw new \Twig_Error_Syntax(sprintf('The text to be translated with "trans" can only contain references to simple variables'), $lineno);

Or am I wrong in that assumption?

Berdir’s picture

Nope, I got warnings from checkPlain(), so it apparently it is passing it right through.

I don't really agree with your argumentation :) t() does not and IMHO should not treat render arrays specially because nothing on the PHP side does. There you need to know if you have a render array, an object or a string. You can't do 'some string' . $render_array anywhere.

However, isn't twig supposed to hide those differences? If it would be a different syntax inside a trans element then it might make sense, but it is the same {{ something }} that handles render arrays just fine outsided. @effulgentsia also assumed that it would work, see his comments in the referenced issue.

joelpittet’s picture

So you do agree with my argument:) That's what I'm saying :P We shouldn't treat render arrays specially. I just mentioned that an option, though I don't agree it's the right way and would be undone when we go Renderable Objects in the future.

Twig does hide those difference by finding print statements {{ var }} and wrapping that from print $var; to print twig_render_var($var); Which internally finds render arrays and calls drupal_render() on them.

But inside trans block it was prevented from using complex objects and filters because it was tricky for whomever was writing it to deal with all the cases.

It is a bit confusing as those tokens look like print, so I agree it's a bit confusing. Actually a good person to get some context on this would me @Mark Carver. Let's see if he can shed some light.

Cottser’s picture

Fabianx’s picture

All we need to do here is replace t($x) with t(twig_render_var($x)) in the output.

I am not worried about the performance impact anymore, which would be the only reason _not_ to do this ...

This should be a simple search+replace patch and some little work on tests.

Denchev’s picture

FileSize
873 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,196 pass(es), 4 fail(s), and 0 exception(s). View

Here is a start.

Denchev’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: core-trans_render_array_support-2334319-8.patch, failed testing.

Wim Leers’s picture

Is this still actually a problem? I'm not sure I fully understand what the issue is about.

Cottser’s picture

Yeah definitely needs an IS update. I think the issue is that if fluffy_kittens is a render array, this doesn't work:

{% trans %}
  {{ fluffy_kittens }} are great.
{% endtrans %}
Fabianx’s picture

#12 is correct.

calebtr’s picture

Status: Needs work » Needs review
FileSize
870 bytes

Per IRC chat with joelpittet, I'm changing #8 to remove the ::escapeFilter method and use ::renderVar() instead

Status: Needs review » Needs work

The last submitted patch, 14: core-trans_render_array_support-2334319-14.patch, failed testing.

Denchev’s picture

Status: Needs work » Needs review
FileSize
861 bytes
858 bytes

Fixed wrong argument.

Status: Needs review » Needs work

The last submitted patch, 16: core-trans_render_array_support-2334319-16.patch, failed testing.

joelpittet’s picture

Issue tags: +rc target triage
xjm’s picture

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.

Denchev’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
4.25 KB

So so far i removed some unneeded arguments that were left over from escapeFilter() and added test for render array. Looking for some feedback on improving this.

Denchev’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 20: trans_does_not-2334319-20.patch, failed testing.

Denchev’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
533 bytes

Lets see what this breaks.

slashrsm’s picture

Status: Needs review » Needs work

Few comments. 1. and 2. from my IRC discussion with @chx.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -497,6 +497,10 @@ public function renderVar($arg) {
    +    if ($arg instanceof MarkupInterface) {
    +      return $arg;
    +    }
    +
         // Optimize for scalars as it is likely they come from the escape filter.
         if (is_scalar($arg)) {
           return $arg;
    
    if (is_scalar($arg) || $arg instanceof MarkupInterface) {
    

    dont break the scalar optimization

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
    @@ -67,10 +67,9 @@ public function compile(\Twig_Compiler $compiler) {
    -      $compiler->string($token->getAttribute('placeholder'))->raw(' => ')->subcompile($token)->raw(', ');
    +      $compiler->string($token->getAttribute('placeholder'))->raw(' => $this->env->getExtension(\'drupal_core\')->renderVar(')->subcompile($token)->raw('), ');
    

    can we use " instead of \' ?

  3. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
    @@ -67,10 +67,9 @@ public function compile(\Twig_Compiler $compiler) {
         $compiler->raw(')');
    -
         // Write any options passed.
         if (!empty($options)) {
    

    Let's not introduce unrelated changes.

Denchev’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
1.41 KB

Fixes from #24

dawehner’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -498,7 +498,7 @@ public function renderVar($arg) {
     // Optimize for scalars as it is likely they come from the escape filter.
-    if (is_scalar($arg)) {
+    if (is_scalar($arg) || $arg instanceof MarkupInterface) {
       return $arg;
     }
 

This is super odd ... given that this invalidates a lot of documentation of the method. Now the __toString() method is no longer called

dawehner’s picture

Ah actually the behaviour is right, that interface is already markup, so it should not be cast to a string, as this looses information.

What about doing something like escapeFilter() which is :

    // Keep Twig_Markup objects intact to support autoescaping.
    if ($autoescape && ($arg instanceof \Twig_Markup || $arg instanceof MarkupInterface)) {
      return $arg;
    }

so also check for \Twig_Markup?

slashrsm’s picture

Status: Needs review » Needs work
Denchev’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
1.21 KB

removed one more unrelated change and added \Twig_Markup

dawehner’s picture

IMHO a dedicated test for render($MarkupInterfaceObject),/code> would be nice

Denchev’s picture

FileSize
5.05 KB
2.03 KB

Added test for markup.
I found later TwigExtensionTest where i could have added the check, don't have enough time now.

dawehner’s picture

MH, that feels wrong, I thought more about using render_var() directly?

Denchev’s picture

Removed the last test for markup and added a new one that directly tests if renderVar is returning the markupinterface obj.

joelpittet’s picture

Title: {% trans %} does not support render array placeholders » {% trans %} does not support render array and MarkupInterface valued placeholders
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thank you @Denchev

Has tests for renderable arrays and FormattableMarkup (for MarkupInterface).

I'll add a change record.

joelpittet’s picture

andypost’s picture

@joelpittet please provide example what will work in CR because now it confusing

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: trans_does_not-2334319-33.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
33.69 KB
34.64 KB

@andypost That example worked when I tried the CR example. Which part is confusing?.
Before:

After:

I just chucked the snippets in bartik's bartik_preprocess_node() and node.html.twig.

andypost’s picture

Issue summary: View changes

RTBC++

@joelpittet sorry, looks I misread "s/now/not" - thanx a lot for screens, added to summary

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Issue tags: -rc target triage +Needs change record, +Needs Documentation

Given that #2226493: Apply formatters and widgets to Node base fields got fixed without this I'm not sure this is a bug. As far as I can see we should only put this into 8.1.x. The patch seems to be missing documentation and a change record.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record, -Needs Documentation

https://www.drupal.org/node/2615198 was just missing the reference to this issue so I added that.

Asked @alexpott what he meant about needing documentation and he clarified that there is no update to API documentation. Setting NW for that.

xjm’s picture

I was also wondering if it makes sense for markup objects and render arrays to be lumped together in this issue? It seems like in general we have an expectation that markup objects should be usable anywhere we would use a string in rendering, but that render arrays are another whole can of worms (which is why #2505497: Support render arrays for drupal_set_message() was not accepted).

Berdir’s picture

This issue started as support for render arrays, it existed before we had markup objects :)

Yes, render array are a different thing on the PHP side. But that's not true for Twig templates. You can do {{ whatever }} with anything, almost anywhere. Isn't it kind of the goal that themers don't have to know about render arrays?

I'm fine with pushing this to 8.1.x. But note that #2226493: Apply formatters and widgets to Node base fields had to use drupal_render() in preprocess to avoid this. That's very hard to understand for themers and it's also inefficient, we render those things all the time, even if we don't use them in the template. That said, I guess these days you could also use whatever|render but that's not exactly straight forward either :)

joelpittet’s picture

+1 to #43.

I'd like Twig to deal with render arrays when printing in all cases.

lauriii’s picture

+1 for making #43 in a follow-up!

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

We have CR, latest patch applies still so I think this is RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Looking at this I still feel it is missing documentation - however looking at this the only place where {% trans %} is in core/lib/Drupal/Core/Language/language.api.php and I'm not sure this is appropriate for there. We're missing a place to put documentation for how Drupal integrates with Twig and our customisations - we should have a place to document things like |without and {% trans %}. So let's not try and add this here.

I guess we should also file a followup to remove the drupal_render() added to template_preprocess_node() in #2226493: Apply formatters and widgets to Node base fields.

I've thought a lot about security implications of this patch and I don't think it changes anything even though we've changed TwigExtension::renderVar(). That said I think we should improve the @return documentation of TwigExtension::renderVar(). Currently it is:

   * @return mixed
   *   The rendered output or an Twig_Markup object.

I think it should it is either a scalar, MarkupInterface or Twig_Markup.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

andypost’s picture

Issue tags: +Needs reroll
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.81 KB

Still need to address #47

agoradesign’s picture

If this change will be committed, is there a chance to push it to 8.1.x too? It seems, that Entity API will need this work -> https://github.com/fago/entity/pull/38. If yes, than I guess only before rc1 in 2 weeks...

I don't know, if there's a possible workaround within Entity API. I only fear the following scenario:

Commerce 2.x (that will be a very important D8 module) depends on 8.1.x as well as on Entity API 1.x. Currently, the pull request for making Entity API 1.x fit for 8.1.x, only passes tests, when the patch from this issue is applied => neverending patching and patch re-rerolling until 8.2.x is released

Berdir’s picture

As commented over there, the fact that the entity module relied on this is a bug, a bug that is fixed in the Pull request that you linked.

It would still be very nice to see this fixed, but this does not block entity or commerce in any way.

agoradesign’s picture

Thanks 4 clarifying. I've mixed up the two patches/issues you mentioned in the PR. @bojanz already told me in a Commerce issue thread. However, I agree that this one looks also very useful in general.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

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 review » Needs work

Here's a review

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -504,7 +504,7 @@ public function renderVar($arg) {
    -    if (is_scalar($arg)) {
    +    if (is_scalar($arg) || $arg instanceof MarkupInterface || $arg instanceof \Twig_Markup) {
    

    This line shouldn't be needed at all. In the line below if (is_object($arg)) { condition it should be covered by the __toString() check that is already in there which covers both the interface and the Twig_Markup.

  2. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
    @@ -24,6 +24,16 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
    +        '#prefix' => '<b>',
    

    nit: can we put <strong> for html5?

joelpittet’s picture

Here's the changes I mentioned in #56, had to change the test because it was expecting the object though renderVar will render it as the method name suggests.

Tested locally and it seems to work, let the testbot double check.

The last submitted patch, 57: trans-support-render-array-2334319-57-test-only-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 57: trans-support-render-array-2334319-57.patch, failed testing.

joelpittet’s picture

Hmm I'll dig in deeper into the fails, it looked like that was needed, thanks for automated test coverage.

Cottser’s picture

Status: Needs work » Needs review
FileSize
5.3 KB

Just attempting a reroll of #57, no changes.

Status: Needs review » Needs work

The last submitted patch, 61: trans_does_not-2334319-61.patch, failed testing.

joelpittet’s picture

Sorry I fell in a rabbit hole and then saw a bunny. Thanks for picking this up @Cottser

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

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