Attempting to render empty Twig "trans" tags (i.e., {% trans %}{% endtrans %} causes a fatal exception like the following:

An exception has been thrown during the compilation of a template ("Attribute "data" does not exist for Node "Twig_Node".") in "{# inline_template_start #}{% trans %}{% endtrans %}".

This is because of a call to \Twig_Node::getAttribute() for a "data" attribute that doesn't exist in the case of an empty tag. The simple solution is to test for the attribute before requesting it. Patch to follow.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

TravisCarden created an issue. See original summary.

TravisCarden’s picture

Here's a failing test and the fix that makes it pass.

The last submitted patch, 2: empty_twig_trans-2698141-2-failing-test.patch, failed testing.

ChuChuNaKu’s picture

I was able to review this patch and it worked for me. To reproduce the error I opened core/themes/classy/templates/layout/html.html.twig and added the following before the closing body tag

{% trans %}{% endtrans %}

I then refreshed the homepage of a local fresh installation of Drupal 8 and I received the error mentioned in the ticket description.

I then applied the patch and refreshed the homepage and the error went away.

ChuChuNaKu’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review, +Needs framework manager review

Discussed with @Cottser and @alexpott. We are not actually sure this fix is correct, because an empty translatable string isn't actually valid, so informing the developer of that is good. We might be able to improve the error handling, but OTOH the exception in the summary does seem pretty clear about what the template error is. But supporting an empty string (which is what the current patch does) is probably not correct.

Also moving to 8.2.x, because changes to error handling generally should be minor version targets. (In general, changing when an exception is or isn't thown may disrupt contrib/custom code that responds to that exception.)

TravisCarden’s picture

@xjm: I can definitely see the point that an empty translatable string doesn't make sense. Nevertheless, an uncaught exception, and consequently an HTTP 500, seems extreme for something with such negligible side effects. I personally encountered it while testing suggestions for an incomplete template, where the white screen of death needlessly derailed me from the task at hand to debugging something that turned out not to be an issue at all. That seems like an undesirable developer experience, though I can also see the argument that the exception message is pretty clear to the developer who reads it. I think that if it were me I would still "fix" it, but I gladly defer to the judgment of the subsystem maintainer. :)

dawehner’s picture

Yeah its one of those things where failing for me is totally fine, but what isn't fine is hiding it, because well, someone did a mistake when creating the template.
When you run a production site with a broken template, you have failed somehow on QA at some point.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review +Needs followup

I'm totally agreeing with both points but I'm siding with #7 from @TravisCarden for this issue. Derailing the developer experience is not an acceptable. We really should commit this patch as is and look at a more generic exception catching in templates (where that should/can happen will likely derail this immediate problem) in a follow-up issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Derailing the developer experience is not an acceptable.

I think that we derail the developer experience by not exposing that the template is invalid and needs fixing. What we need to ensure is that the exception thrown is helpful and leads the developer to quickly identify and fix the template (inline or otherwise).

TravisCarden’s picture

So the current exception thrown says this:

An exception has been thrown during the compilation of a template ("Attribute "data" does not exist for Node "Twig_Node".") in "{# inline_template_start #}{% trans %}{% endtrans %}".

That's fairly clear because it's a tiny inline template, but in most situations it won't be so obvious because you'll have a large(r) template file, and in either case you have to know the internals of Twig_Node to know what a non-existent "data" attribute means. Perhaps we can throw a more helpful exception--one that just comes right out and says you've got an empty {% trans %} tag.

TravisCarden’s picture

What about something like this? This throws the same exception but changes the message to be like the following:

{% trans %} tag cannot be empty in "{# inline_template_start #}{% trans %}{% endtrans %}"

The last submitted patch, 12: empty_twig_trans-2698141-12-failing-test.patch, failed testing.

TravisCarden’s picture

Oops. I made a typo in one of the assertion messages: "thew" instead of "threw".

ChuChuNaKu’s picture

Status: Needs review » Reviewed & tested by the community

I tested and reviewed the patch against the latest version of Drupal 8.2. As stated above the same exception is thrown but the message has now been changed to:

Twig_Error_Syntax: {% trans %} tag cannot be empty in "core/themes/classy/templates/layout/html.html.twig" in Drupal\Core\Template\TwigNodeTrans->compileString() (line 173 of core/lib/Drupal/Core/Template/TwigNodeTrans.php).

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

#14 incorporates #10's review, so removing the "Needs framework manager review" tag. I also think the patch incorporates what was requested in #9 as a follow-up, so removing the "Needs followup" tag.

+++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
@@ -92,6 +92,30 @@ public function testTwigTransTags() {
+    try {
+      /** @var \Drupal\Core\Render\RendererInterface $renderer */
+      $renderer = \Drupal::service('renderer');
+      $renderer->renderPlain($elements);

It makes sense why the ->renderPlain() needs to happen in a try, but the construction of the renderer should be outside of the try, because it is not this test's job to handle the case of an inability to construct the renderer. "Needs work" for that, but otherwise this patch looks good to me.

effulgentsia’s picture

Actually removing the tags mentioned in #16.

effulgentsia’s picture

Title: Empty Twig {% trans %} tag causes fatal exception » Empty Twig {% trans %} tag causes unhelpful fatal exception
TravisCarden’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
870 bytes

I can deal with that. :)

Status: Needs review » Needs work

The last submitted patch, 19: empty_twig_trans-2698141-19.patch, failed testing.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Oops. User error. :)

Status: Needs review » Needs work

The last submitted patch, 21: empty_twig_trans-2698141-21.patch, failed testing.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
794 bytes

Oh my goodness. Focus, self, focus...

TravisCarden’s picture

Status: Needs review » Reviewed & tested by the community

And given @effulgentsia's pre-approval of the change in #16, I'll just move this straight to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1dc4ba8 and pushed to 8.2.x. Thanks!

  • alexpott committed 1dc4ba8 on 8.2.x
    Issue #2698141 by TravisCarden, ChuChuNaKu, effulgentsia: Empty Twig {%...

Status: Fixed » Closed (fixed)

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