{% trans %} does not support render array and MarkupInterface valued placeholders

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?

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?

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.

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.

This problem came up in #2450993-140: Rendered Cache Metadata created during the main controller request gets lost.

Here is a start.

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

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 %}

#12 is correct.

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

Fixed wrong argument.

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.

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.

Lets see what this breaks.

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.

Fixes from #24

+++ 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

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?

removed one more unrelated change and added \Twig_Markup

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

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

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

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

Thank you @Denchev

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

I'll add a change record.

Here's a CR: https://www.drupal.org/node/2615198

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

@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.

RTBC++

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

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.

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.

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).

+1 to #43.

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

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

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

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.

Still need to address #47

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

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.

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?

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.

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

Just attempting a reroll of #57, no changes.

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

Any update on this issue?

Looks like the latest patch failed for 8.4.

Comment removed. Sorry, wrong issue.

Rerroll of #61

Referencing the "drupal_core" extension by its name (defined by getName()) is deprecated since 1.26 and will be removed in Twig 2.0. Use the Fully Qualified Extension Class Name instead.

Attempting to fix these errors...

It's not possible still to use |function on a variable within trans...

Re-roll #71.

Rerolled #82's patch on D9.5 and fixed tests (I hope)
(This patch applies on 9.4 too)

Rerolled on 10.1.x

I should have trusted @webflo in the first place :)
Bringed back the code I removed in #86 and fixed tests accordingly.

The attached patch is for people that needs this for D9 right now.

Tried to fix the failed test of patch #89.