Commit Message

Issue #1927584 by Mark Carver, drupalninja99, penyaskito, Cottser, jenlampton, John Bickar, geoffreyr, ezeedub: Add support for the Twig {% trans %} tag extension

Latest patches

Complete patch: #109
Test only patch: #110

Problem/Motivation

In the node template, we currently have this:

<p class="submitted">{{ submitted }}</p>

Front-end developers despise this. What they want instead:

<p class="submitted">Submitted by {{ author }} on {{ date }}</p>

Unfortunately, this isn't translatable. We'd have to give them something like:

<p class="submitted">{{ "Submitted by !author on @date"|t({ '!author': author, '@date': date }) }}</p>

This syntax is extremely complex to scan. This also requires the front-end developer to know of the @ ! % "Drupalism" prefix of placeholders and what they do.

Proposed resolution

Twig has a better way to solve this problem. Add support for the Twig i18n trans tag to Drupal. It will translate the text using tokens with t() or format_plural() if the {% plural ... %} switch has been declared in the tag:

<p class="submitted">
{% trans %}
  Submitted by {{ author.name }} on {{ node.date }}
{% endtrans %}
</p>

Note: When using complex tokens (ie: arrays/objects) the placeholder name used for translation msgids is the key name in the token (after the . dot). In the above example, the msgid would read: Submitted by @name on @date, not Submitted by @author.name on @node.date.

Note: Filtering tokens inside the {% trans %} tag generally work. However, some of these filters may not work properly or at all. If you are not seeing the desired result or you are receiving fatal errors/WSOD you may need scale down what you are trying to do inside the {% trans %} tag. Create a new token outside of the tag with this filter applied:

{% set date = node.created|format_date('medium') %}
{% trans %}
  Node was created on {{ date }}.
{% endtrans %}

We can also extend the {% trans %} tag with a {% plural ... %} switch as well:

{% set count = comments|length %}
{% trans %}
  {{ count }} comment was deleted successfully.
{% plural count %}
  {{ count }} comments were deleted successfully.
{% endtrans %}

By default, all tokens are sent to t() or format_plural() with a @ prefix, to be escaped. If the value of that token needs to be passed through (!) or used as a placeholder (%), modify the token with these filters:

{% set string = '&"<>' %}
<div>
  {% trans %}
    Escaped: {{ string }}
  {% endtrans %}
</div>
<div>
  {% trans %}
    Pass-through: {{ string|passthrough }}
  {% endtrans %}
</div>
<div>
  {% trans %}
    Placeholder: {{ string|placeholder }}
  {% endtrans %}
</div>

View the expected translation msgid when templates use {% trans %} and twig_debug is enabled:

<!-- TRANSLATION: "Hello star.", PLURAL: "Hello @count stars." -->

Completed Tasks

  1. Add the Twig i18n extension
  2. Rewrite the i18n extension and override it in a way that it maps to our functions.
  3. Change the usage of gettext() to t() and format_plural().
  4. Create tests for the {% trans %} tag that maps to t() and format_plural() from templates.
  5. Ensure the {% trans %} tag works with String::format() (used by t() and format_plural())
  6. Ensure strings are translatable using .po files.
  7. Wish: Provide debug markup for which translation should be used.

Remaining Tasks (@todo after commit)

References

http://twig.sensiolabs.org/doc/extensions/i18n.html
http://twig.sensiolabs.org/doc/advanced.html#creating-an-extension
http://twig.sensiolabs.org/doc/advanced.html#overloading
https://github.com/fabpot/Twig-extensions

Examples of syntax:
#2004252: node.html.twig template
https://gist.github.com/jenlampton/4c7091f29b15f879aec5
https://gist.github.com/joelpittet/742f9ee5c9c6ac41c39e

Blocking Issues

#2004252: node.html.twig template
#2031883: Markup for: comment.html.twig

CommentFileSizeAuthor
#131 trans-output.png237.92 KBJeff Burnz
#115 drupal-add-support-for-the-1927584-115.patch23.08 KBmarkhalliwell
#115 interdiff.txt10.23 KBmarkhalliwell
#110 1927584-tests.patch12.27 KBmarkhalliwell
#109 drupal-add-support-for-the-1927584-107.patch23.17 KBmarkhalliwell
#109 interdiff.txt1.25 KBmarkhalliwell
#107 drupal-add-support-for-the-1927584-107.patch23.17 KBmarkhalliwell
#107 interdiff.txt1.25 KBmarkhalliwell
#104 drupal-add-support-for-the-1927584-104.patch23 KBmarkhalliwell
#104 interdiff.txt6.27 KBmarkhalliwell
#100 drupal-handle-trans-block-1927584-100.patch22.7 KBmarkhalliwell
#100 interdiff.txt6.96 KBmarkhalliwell
#99 1927584-complete-99.patch20.18 KBdrupalninja99
#99 interdiff.txt5.99 KBdrupalninja99
#91 1927584-tests.patch11.43 KBmarkhalliwell
#91 1927584-complete.patch21.28 KBmarkhalliwell
#89 drupal-handle-trans-block-1927584.86-89.txt3.14 KBpenyaskito
#89 drupal-handle-trans-block-1927584-89.patch20.06 KBpenyaskito
#86 drupal-handle-trans-block-1927584-86.patch20.87 KBmarkhalliwell
#86 interdiff.txt955 bytesmarkhalliwell
#82 Screen Shot 2013-07-10 at 12.08.09 AM.png31.93 KBmarkhalliwell
#81 drupal-handle-trans-block-1927584-81.patch20.67 KBmarkhalliwell
#81 interdiff.txt13.97 KBmarkhalliwell
#75 interdiff.txt25.58 KBmarkhalliwell
#74 drupal-handle-trans-block-1927584-74.patch17.63 KBmarkhalliwell
#68 drupal-handle-trans-block-1927584-68.patch25.15 KBmarkhalliwell
#68 interdiff.txt6.23 KBmarkhalliwell
#55 drupal-handle-trans-block-1927584-55.patch28.63 KBmarkhalliwell
#55 interdiff.txt27.64 KBmarkhalliwell
#53 1927584-53.patch27.52 KBstar-szr
#53 interdiff.txt720 bytesstar-szr
#50 drupal-handle-trans-block-1927584-50.patch29.72 KBmarkhalliwell
#50 interdiff.txt7.93 KBmarkhalliwell
#45 1927584-45.patch27.28 KBdrupalninja99
#42 1927584-42.patch962.07 KBdrupalninja99
#42 interdiff.txt1.92 KBdrupalninja99
#37 1927584-37.patch27.36 KBstar-szr
#37 interdiff.txt2.48 KBstar-szr
#34 drupal-twigtranstest-1927584-34.patch27.42 KBjenlampton
#34 interdiff.txt1.21 KBjenlampton
#24 drupal-twigtranstest-1927584-24.patch27.43 KBJohn Bickar
#12 1927584.patch27.43 KBgeoffreyr
#12 1927584-interdiff.txt23.2 KBgeoffreyr
#8 drupal-1927584.patch7.5 KBezeedub
#8 interdiff.txt458 bytesezeedub
#7 drupal-1927584.patch815 bytesezeedub
#4 twig_trans_block-1927584-4.patch6.58 KBezeedub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Issue tags: +Twig

Adding tag

joelpittet’s picture

Category: feature » task
ezeedub’s picture

Assigned: joelpittet » ezeedub
ezeedub’s picture

Here's a patch with the classes I think we need for this. Sorry, I punted on the test I was writing. To test this, I simply used the node.html.twig in the stark module (in the twig sandbox repo).

This works.

{% trans "Hello World" %}

This works.

{% trans %}
    Hello World.
{% endtrans %}

This doesn't work yet.

{% set name = 'Bob' %}
{% trans %}
    Hello {{ name }}.
{% endtrans %}

Since I got stuck on interpolating a variable, I also didn't yet look at handling plural.

Any variable I pass through trans this way gets renamed "render_var" and is either not set in TwigTemplate::getContextReference() or I'm not getting in there and should be... I'm wondering if there's a variable "name" attribute that I need to be setting/registering somewhere that I'm not doing..

Any insight appreciated!

ezeedub’s picture

Title: Add format_plural to twig filters » Handle trans block as Twig extension
disasm’s picture

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,131 @@
+	public function __construct(\Twig_NodeInterface $body, \Twig_NodeInterface $plural = null, \Twig_Node_Expression $count = null, $lineno, $tag = null)

Normally, I wait until a patch is a little more mature before nitpicking, but this one has so many whitespace errors, it's hard to follow what's going on in it. I only pasted one example, but almost every line in this class is indented wrong, and it's missing a newline at the end of the file.

ezeedub’s picture

Status: Active » Needs review
FileSize
815 bytes

Ah thx Sam, was updating eclipse when I did this.

ezeedub’s picture

FileSize
458 bytes
7.5 KB

Let's try that again including new files (trying out drush iq-submit).

geoffreyr’s picture

Assigned: ezeedub » geoffreyr
geoffreyr’s picture

Am going over the patch from comment 8 and trying to write some functional tests. Thinking I might use a modified .po for Lolspeak for the tests because it's easier to take liberties with.

My results are as follows:

{% trans "Hello sun." %}
This works.

{% trans %}
    Hello moon.
{% endtrans %}

This also works.

{% set place = 'Earth' %}
    Hello {{ place }}.

This is fine.

{% set name = 'stars' %}
{% trans %}
    Hello {{ name }}.
{% endtrans %}

This still doesn't work, it prints out an empty space where "stars" should go.

geoffreyr’s picture

Status: Needs review » Needs work

Current status: got trans block working with variable substitution and plural tag. format_plural still in progress.

geoffreyr’s picture

FileSize
23.2 KB
27.43 KB

OK, here's what I have so far. Keep in mind that it does NOT work completely yet, and it will still need work to get the bits that use format_plural within the trans blocks working correctly.

The reason why the interdiff is so big is because I had to create an extra class for the format_plural blocks, and I ended up trying to write some functional tests. Some of them WILL fail at this point.

joelpittet’s picture

Status: Needs work » Needs review

Test bot activate! Thank you @geoffreyr you were awesome at the sprints and great to meet you!

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, 1927584.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

#12: 1927584.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, 1927584.patch, failed testing.

geoffreyr’s picture

@joelpittet: Likewise, hope our paths cross again sometime soon!

Ok, the patch fails on the new tests that I introduced, but doesn't break anything that's already there. That's a good sign.
Might try setting the weights on the language settings during the test and see if that fixes anything. May need to translate the variables separately, but would have to check and see what t() does to make sure.

jenlampton’s picture

Priority: Normal » Major

Bumping this up to major since some of our other template conversions will be held up by trans :)

andypost’s picture

Issue tags: +D8MI

this could lead us to Language::FRONTEND which could be useful for theme layer as custom translations

andypost’s picture

Filed #2015575: Introduce Language::FRONTEND and start separate format_plural from theme and form to allow language able to detect a twig/template-render

Gábor Hojtsy’s picture

It would be important to take stock of all the changes proposed in this issue. Some things I see:

- a new syntax for t() different from what is already available
- introducing a syntax for format_plural
- a new syntax for placeholder strings in text, eg. 'Hello %star_numbers% stars.'; how does this map to/relate to/support the existing three types of placeholders %item, @item and !item

Additionally to this, it looks incorrect that no actual singular/plural pairs are present in the test .po file in the patch.

drupalninja99’s picture

This seems liek a bug "$this->assertText($out, $out);" line 80 in TwigTransFormatPluralTest.php.

jenlampton’s picture

Assigned: geoffreyr » Unassigned

Unassigning in hopes of getting someone more movement on this one.

jenlampton’s picture

Issue summary: View changes

task not feature

jenlampton’s picture

Issue summary: View changes

better why

jenlampton’s picture

Issue summary: View changes

this

John Bickar’s picture

Not setting to needs review yet b/c I want to test with simplytest.me first.

John Bickar’s picture

Status: Needs work » Needs review

Looks like it's failing these tests because the text is not being translated. Here's what gets output to the page at twig-theme-test/trans:

Hello sun.
Hello moon.
Hello star.
Hello 2 stars.
Hello Earth.

When it should be in LOLSpeak. (I think.)

John Bickar’s picture

Status: Needs review » Needs work

Did not mean to set to needs review.

drupalninja99’s picture

@John, you are correct, I get the same thing.

drupalninja99’s picture

I keep getting whitespace errors from https://drupal.org/files/drupal-twigtranstest-1927584-24.patch

<stdin>:250: trailing whitespace.
          $args = $n->getNode('arguments')->getNode(0); 
<stdin>:406: trailing whitespace.
          $args = $n->getNode('arguments')->getNode(0); 
warning: 2 lines add whitespace errors.
John Bickar’s picture

The patch in #24 doesn't fix the failing tests, nor even fix what I was trying to fix, so it probably shouldn't be used. By anyone. Ever :D

drupalninja99’s picture

I feel like there is a problem with some of the test theme / twig theme test functionality that is causing the tests to fail. I get these kind of errors when I try to enable the twig_theme_test theme.

Notice: Undefined index: twig_theme_test in drupal_theme_initialize() (line 115 of core/includes/theme.inc).
Notice: Trying to get property of non-object in _drupal_theme_initialize() (line 152 of core/includes/theme.inc).
Strict warning: Creating default object from empty value in _drupal_theme_initialize() (line 159 of core/includes/theme.inc).
Notice: Undefined property: stdClass::$name in _theme_load_registry() (line 380 of core/includes/theme.inc).
Notice: Undefined index: twig_theme_test in drupal_theme_initialize() (line 115 of core/includes/theme.inc).
Notice: Trying to get property of non-object in _drupal_theme_initialize() (line 152 of core/includes/theme.inc).
Strict warning: Creating default object from empty value in _drupal_theme_initialize() (line 159 of core/includes/theme.inc).
star-szr’s picture

twig_theme_test is a module, but I don't think there was ever a test theme by that name in Drupal 8. The previous test theme was called test_theme_twig but was renamed to just test_theme when Twig became the default theme engine. Not confusing at all, I know :). This change happened in #1806478: Make twig the default engine once all modules templates are converted from .tpl.php to .html.twig.

Edit: I got the test theme names wrong ;)

drupalninja99’s picture

K so that's prob one of the problems, I still see "->set('default', 'twig_theme_test')" in at least one of the Twig trans tests.

drupalninja99’s picture

OK so I can fix the theme issue. That is an easy fix.

I am also seeing in testTransBlocks() that $lolspeak_langcode that isn't being used. Is 'xx-lolspeak' supposed to be used instead of 'xx'?

// Enable lolspeak.
    $lolspeak_langcode = 'xx-lolspeak';
    $langcode = 'xx';
    $name = 'Lolspeak';
    $edit = array(
      'predefined_langcode' => 'custom',
      'langcode' => $langcode,
      'name' => $name,
      'direction' => '0',
    );
    $this->drupalPost('admin/config/regional/language/add', $edit, t('Add custom language'));
    $this->assertEqual($this->getUrl(), url('admin/config/regional/language', array('absolute' => TRUE)), 'Correct page redirection.');
    $this->assertRaw('"edit-languages-' . $langcode .'-weight"', 'Language code found.');

    // Assign Lolspeak to be the default language.
    $edit = array('site_default_language' => $langcode);
    $this->drupalpost('admin/config/regional/settings', $edit, t('Save configuration'));
    $this->assertEqual(language_default()->langcode, $langcode, $name . ' is the default language');

    // Import custom lolspeak .po file.
    $this->importPoFile($this->examplePoFile(), array(
      'langcode' => $langcode,
      'customized' => TRUE,
    ));
jenlampton’s picture

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

@drupalninja99 I do think something is wrong with that test. You're right in that it's never using the lolspeak langcode, and it probably should. In both tests. Can you create a follow-up issue under translation or wherever that test is, that points out this potential problem? We shouldn't fix it in this patch.

rerolled fixing the test theme name, hopefully this will get us a bit closer.

jenlampton’s picture

Issue summary: View changes

symbols

jenlampton’s picture

Anyone know how to test this? Can you add a note to the issue summary for our reviewers? :)

jenlampton’s picture

Issue summary: View changes

added another gist example :)

Status: Needs review » Needs work

The last submitted patch, drupal-twigtranstest-1927584-34.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

dream markup usage

star-szr’s picture

Status: Needs work » Needs review
FileSize
2.48 KB
27.36 KB

Just a patch to move things along slightly, @jenlampton and I were both looking at this last night.

  • Fixes the test exceptions and a couple test failures - the patch will still have a few failures.
  • Fixes up tabs and trailing whitespace (trailing whitespace removal not shown in interdiff).

Status: Needs review » Needs work

The last submitted patch, 1927584-37.patch, failed testing.

drupalninja99’s picture

@Cottser, this looks like it's on the right track. Has anyone been able to actually manually get the test translation page to work?

drupalninja99’s picture

This line "lolspeak_langcode = 'xx-lolspeak';" is not being used, can it be removed from the test?

star-szr’s picture

@drupalninja99 - yes, I don't see why not.

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans_invalid.html.twigundefined
@@ -0,0 +1,24 @@
+	{% trans %}

I missed a tab in this file :/

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
962.07 KB

I fixed a couple of easy problems. The first 2 tests that were failing were wrong bc they didn't match the po file. I fixed the discrepancies with the "OH"/"O" confusion that was causing those tests to fail (I think). I uncommented out the 2 tests that weren't being called and fixed the syntax to match what it should actually be testing.

When I run these locally they still fail but I think we are getting slightly closer here. I still feel like there is likely a problem with the actual test itself.

I was able to go through the tests manually to see if the translation works. On this page "/xx/twig-theme-test/trans" this is the translations I got:

OH HAI SUNZ
OH HAI TEH MUUN
Hello star.
Hello 2 stars.
O HAI THAR Earth

So confusingly it only is partially working. I don't know why Hello star didn't translate.

Status: Needs review » Needs work

The last submitted patch, 1927584-42.patch, failed testing.

joelpittet’s picture

@drupalninja99 re #42 the interdiff looks fine just the patch wasn't rebased against origin/8.x. File size is near 1MB vs 27.36K from the previous.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
27.28 KB

Okay sorry, here let's try that again

Status: Needs review » Needs work

The last submitted patch, 1927584-45.patch, failed testing.

jenlampton’s picture

This is looking great, thanks @drupalninja99! The only tests that are failing are the new TwigTrans tests. I'm not sure why though, can someone take a deeper look?

drupalninja99’s picture

Seems like all of the tests are failing to me:

"Hello sun." translation tag was successfully translated. Other TwigTransFormatPluralTest.php 82 Drupal\system\Tests\Theme\TwigTransFormatPluralTest->testTransBlocks()
"Hello moon." translation block was successfully translated. Other TwigTransFormatPluralTest.php 86 Drupal\system\Tests\Theme\TwigTransFormatPluralTest->testTransBlocks()
"Hello star." translation block with variable was successfully translated. Other TwigTransFormatPluralTest.php 91 Drupal\system\Tests\Theme\TwigTransFormatPluralTest->testTransBlocks()
"Hello {{ count }} stars." translation block with variable was successfully translated. Other TwigTransFormatPluralTest.php 95 Drupal\system\Tests\Theme\TwigTransFormatPluralTest->testTransBlocks()
"Hello {{ name }}." translation block with variable was successfully translated. Other TwigTransFormatPluralTest.php 100 Drupal\system\Tests\Theme\TwigTransFormatPluralTest->testTransBlocks()

markhalliwell’s picture

Yeah, I've been looking at this too. Jump on irc?

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
7.93 KB
29.72 KB

Ok. This patch will still fail, but I was able to clean up quite a bit of the testing issues we were having. Basically the test environment and a lot of assertions needed to be refactored for locale support. I'll post a review of my interdiff.txt to explains some of the changes here.

markhalliwell’s picture

I know this may seem like a lot of changes, but this should allow better debugging. They all pass except these two still:

"Hello star." translation block with variable was successfully translated.
"Hello {{ count }} stars." translation block with variable was successfully translated.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -8,8 +8,6 @@
-use Drupal\test_theme\ThemeClass;
-use Drupal\Core\Language\Language;

Removed unused classes.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -31,55 +33,34 @@ public static function getInfo() {
+    // Add test language for translation testing.
+    $this->addTestLanguage();

Created this method and moved it int the setUp() method for the test. It helps ensure that they xx language is setup properly before any tests are made.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -31,55 +33,34 @@ public static function getInfo() {
-    $this->drupalLogout();

Do we really need to logout? I debated this back and forth for a while. Do we need a different test to check this type of difference. Is there a difference?

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -31,55 +33,34 @@ public static function getInfo() {
+    $this->drupalGet('twig-theme-test/trans', array('language' => language_load('xx')));
-    $this->drupalGet('twig-theme-test/trans');

This was probably the main culprit. The page wasn't redirecting to the correct language so the translated assertions would fail.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -89,24 +70,22 @@ function testTransBlocks() {
     $this->assertText(
-      'O HAI <em class="placeholder">2</em> STARZZZZ',
+      'O HAI 2 STARZZZZ',
       '"Hello {{ count }} stars." translation block with variable was successfully translated.'
     );
     $this->assertText(
-      'O HAI THAR <em class="placeholder">Earth</em>',
+      'O HAI THAR Earth',
       '"Hello {{ name }}." translation block with variable was successfully translated.'
     );

assertText strips out HTML tags. This isn't why these are failing though. I still can't figure out why these two are failing, but I think it has something to do with the twig plural modifier in the trans block that interacts with format_plural().

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -89,24 +70,22 @@ function testTransBlocks() {
-  function testFormatPluralBlocks() {
-    config('system.theme')
-      ->set('default', 'test_theme')
-      ->save();
+  function testTwigFormatPluralBlocks() {

Moved this into the setUp() method. Renamed it so it better reflects the test name.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -122,6 +101,39 @@ function testFormatPluralBlocks() {
+    // Reset the static cache of the language list.
+    drupal_static_reset('language_list');

This needed to be added, seems that the static cache was also the culprit. Caused the language to still revert to en after setting the default language to xx.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -139,29 +151,33 @@ function importPoFile($contents, array $options = array()) {
-"Project-Id-Version: Drupal 8\n"
-"MIME-Version: 1.0\n"
-"Content-Type: text/plain; charset=utf-8\n"
-"Content-Transfer-Encoding: 8bit\n"
-"Plural-Forms: nplurals=2; plural=(n!=1);\n"
+"Project-Id-Version: Drupal 8\\n"
+"MIME-Version: 1.0\\n"
+"Content-Type: text/plain; charset=UTF-8\\n"
+"Content-Transfer-Encoding: 8bit\\n"
+"Plural-Forms: nplurals=2; plural=(n > 1);\\n"

The new lines needed to be double escaped.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -139,29 +151,33 @@ function importPoFile($contents, array $options = array()) {
-msgid "Hello %star_numbers% stars."
-msgstr "O HAI %star_numbers% STARZZZZ"
+
+msgid "Hello %count% stars."
+msgstr "O HAI %count% STARZZZZ"

Pretty self explanatory, but this should be the {{ count }} that's specified in the template.

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
@@ -10,19 +10,19 @@ Hello moon.
-{% set star_number = 1 %}
+{% set stars = 1 %}
 {% trans %}
 Hello star.
-{% plural star_number %}
+{% plural stars %}
 Hello {{ count }} stars.
 {% endtrans %}
 </div>
 <div>
-{% set star_numbers = 2 %}
+{% set stars = 2 %}
 {% trans %}
 Hello star.
-{% plural star_numbers %}
+{% plural stars %}

Renamed the variable name to something plural and shorter, to coincide with something that uses format_plural, like "comments".

markhalliwell’s picture

Status: Needs review » Needs work

I know this will fail. To follow up, anyone that wants to start debugging in the actual Twig token/replacements be my guest. The test should pass once that get's figured out. Might have something to do with the fact that it's using % at the beginning for the variable? Idk.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransFormatPluralTest.php
@@ -89,24 +70,22 @@ function testTransBlocks() {
     $this->assertText(
-      'O HAI <em class="placeholder">2</em> STARZZZZ',
+      'O HAI 2 STARZZZZ',
       '"Hello {{ count }} stars." translation block with variable was successfully translated.'
     );
     $this->assertText(
-      'O HAI THAR <em class="placeholder">Earth</em>',
+      'O HAI THAR Earth',
       '"Hello {{ name }}." translation block with variable was successfully translated.'
     );

assertText strips out HTML tags. This isn't why these are failing though. I still can't figure out why these two are failing, but I think it has something to do with the twig plural modifier in the trans block that interacts with format_plural().

star-szr’s picture

Status: Needs work » Needs review
FileSize
720 bytes
27.52 KB

Thanks Mark! Looking much better :)

As for the plural, what Gábor said in #21 seems to be true:

Additionally to this, it looks incorrect that no actual singular/plural pairs are present in the test .po file in the patch.

I took a look at \Drupal\locale\Tests\LocalePluralFormatTest for an example and updated the provided .po file accordingly. Looks green to me!

markhalliwell’s picture

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

Yay! I love that color. Now that the test is passing, I'll focus on getting the last few things done:

  • Add lolspeak translations for the format_plural twig block
  • Run through coder.
  • Provide a test only patch.
markhalliwell’s picture

Status: Needs work » Needs review
FileSize
27.64 KB
28.63 KB

Ok, summary of this patch (and why interdiff.txt is so large):

  • Cleaned up code with reviewer/sniffer.
  • Added lolspeak translations for the format_plural code block and properly (and successfully!) tested them.
   /**
-   * Test valid Twig "format_plural" blocks.
+   * Test invalid Twig "trans" blocks.
+   *
+   * @todo We need to figure out if this is testable, currently it just returns a 500 error on line 14 of twig_theme_test.trans_invalid.html.twig.
    */
-  function testTwigFormatPluralBlocks() {
+//  public function testTwigTransBlocksInvalid() {
+//    $this->drupalGet('twig-theme-test/trans-invalid', array('language' => language_load('xx')));
+//  }

Not sure if or why we should test for invalid twig syntax (seems like Twig handles that on our own for us, right?). If not, we take out the invalid tests. If so, we need to build/refactor these tests.

After that, we can upload a test only patch.

markhalliwell’s picture

Here's that error message. It returns a 500 page when trying to do invalid trans/format_plural blocks:

Twig_Error_Syntax: Unexpected token "end of statement block" of value "" in "core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans_invalid.html.twig" at line 14 in Twig_ExpressionParser->parsePrimaryExpression() (line 170 of ../core/vendor/twig/twig/lib/Twig/ExpressionParser.php).
The website has encountered an error. Please try again later. 
joelpittet’s picture

I'm on board with that, we don't really need to test syntax errors.

Also, OMG amazing work everybody!

star-szr’s picture

Now that we have things working :) going back to Gábor in #21:

- a new syntax for placeholder strings in text, eg. 'Hello %star_numbers% stars.'; how does this map to/relate to/support the existing three types of placeholders %item, @item and !item

Can we reconcile this difference? I don't think we should be introducing another placeholder format just for translatables in Twig templates.

markhalliwell’s picture

I was wondering about this problem myself.

+++ b/core/lib/Drupal/Core/Template/TwigNodeFormatPlural.php
@@ -0,0 +1,148 @@
+          $msg .= sprintf('%%%s%%', $argName);

This is where it coverts {{ name }} to %name%.

The only way I can see us interpreting this, would be to somehow detect the it from the template itself right? Doing something like {{ %name }}, {{ @name }} or {{ !name }}.... which (in theory) would convert it to something like %name%, @name@ or !name!. I really don't think we can prefix variable names with symbols though... any ideas?

Looks like Twig's docs are down too right now, ugh.

markhalliwell’s picture

Status: Needs review » Needs work
markhalliwell’s picture

In retrospect, I don't even think we would need the ending symbol once it gets converted.

joelpittet’s picture

I'd prefer not to add that syntax into twig if we can do that, the text variable filters can be done with twig already.

Default (currently unescaped/raw) and equivalent to !name :
Hello {{ name }}.

Same same:
Hello {{ name|raw }}.

Escaped and equivalent to @name:
http://twig.sensiolabs.org/doc/filters/escape.html
Hello {{ name|escape }}.
Hello {{ name|e }}.
Hello {{ name|e('js') }}.

Escaped and placeholder'd equivalent to %name:
http://twig.sensiolabs.org/doc/filters/escape.html
Hello <em class="placeholder">{{ name|e }}</em>.

joelpittet’s picture

If we really need placeholder (and I highly doubt it) we could make a twig filter calling String::placeholder...

|escape filter already does: ENT_QUOTES and is smarter about encoding than just 'UTF-8'

markhalliwell’s picture

markhalliwell’s picture

To summarize a chat on IRC for the issue re: #21, #59, #61, #62 and #63:

We should just pass the raw value of the variable in the translation, something like:

+ $msg .= sprintf('!%s', $argName);

Which would output: !name, which is consistent with how t() and format_plural() work.

The issue lies in the fact that %name% is interpreted by those functions as a "placeholder" because it starts with %.

Changing this would resolve this. If a value needs to be escaped, then do it before hand (ie: preprocess the variable or via set name = name|e. For placeholders, we're proposing creating the twig function: {{ placeholder(name) }}.

joelpittet’s picture

We were looking also into doing something like this:

{% trans %} Text {{ !foo }} {{ @bar }} {{ %baz }} {{ booyah }} {% endtrans %}?

But those variable names seem to fail when prefixed...

joelpittet’s picture

This may help... but must take a look later
{% trans(foo, bar, baz) %} Text !foo @bar %baz {{ booyah }} {% endtrans %}

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
6.23 KB
25.15 KB

Just some cleanup: removed rogue dpm() and the invalid tests per #57

markhalliwell’s picture

Status: Needs review » Needs work

Still need to figure out #67.

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -D8MI, -Twig

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

The last submitted patch, drupal-handle-trans-block-1927584-68.patch, failed testing.

markhalliwell’s picture

Ok, steveoliver and I had a pretty length discussion about this topic in irc today (http://pastebin.com/hHz5CbdY for reference).

We think the best course of action to actually get this to work with all parties involved (templates, .po files and Twig extensions). We're proposing that we should:

  1. Rename the trans tag to translate for better readability. We should also differentiate this from the t filter. It isn't necessarily just used for translation, but also translations between singular and plural iterations as well. Syntactically it also differs from both the t() and format_plural()functions entirely and used only in twig templates.
  2. Get rid of the format_plural tag entirely. It is completely redundant at this point.format_plural() runs through t().
  3. Move the arguments in with the tag. Make them associative, there's really no way getting around the !, @, % issue otherwise.
{% translate with {'@author':user.name, '@date':node.date} %}
  Submitted by @name on @date.
{% endtranslate %}
{% translate with { {'@name': user.name } } %}
  @name has only made one comment.
{% plural comments|length %}
  @name has made @count comments.
{% endtranslate %}
{% translate %}
  1 comment.
{% plural comments|length %}
  @count comments.
{% endtranslate %}

And of course:

{% translate %}
  Simple string.
{% endtranslate %}

But in all reality, that could probably be left to the t filter:
{{ "Simple string."|t }}

The translate tag should really just be used when needing placeholders and plural capabilities.

jenlampton’s picture

This is what I want:

{% trans with {'@author':user.name, '@date':node.created_date} %}
  Submitted by {{ author }} on {{ date }}.
{% endtrans %}
  • I'd like it to say trans instead of translate, because that will be familiar to people outside Drupal-land. (I may be persuaded to change my feelings on this, if you try hard enough)
  • I really, really don't want to loose the double braces around {{ author }} and {{ date }}. Twig's trans left these braces here for a reason, so I think we should respect that decision (even though translate is the equivalent of a print statement and so are the double braces, it does end up being a bit redundant). I still love it. It's easy to look at the translated sentence and see which things are the placeholders. Plus, anyone coming from Twig-land who has used trans will see that we are doing the same thing in Drupal.
  • I'd prefer not to have the array mapping in the 'with' but I understand that it is almost certainly necessary because of how t() works, so I am willing to accept it. Ugly or not, we need keyed arrays.
markhalliwell’s picture

Status: Needs work » Needs review
FileSize
17.63 KB

Lots of changes, summary in next comment

markhalliwell’s picture

FileSize
25.58 KB

interdiff for #74

markhalliwell’s picture

Example of @jenlampton's code (#73) with this new method:

{% set author = user.name %}
{% set date = node.created_date %}
{% trans %}
  Submitted by {{ author }} on {{ date }}.
{% endtrans %}

Or, in theory you could do this too. Just not sure how this would work with translation and .po files, they may have @name and @date in them... like above:

{% trans %}
  Submitted by {{ user.name }} on {{ node.created_date }}.
{% endtrans %}
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -48,7 +55,6 @@ public function getTokenParsers() {
-      new TwigFormatPluralTokenParser(),

Removed the format_plural tag... it's redundant since it can be handled with the trans tag.

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
@@ -10,26 +10,36 @@
+{% set string = '&"<>' %}
+<div>
+   {% trans %}
+    Escaped: {{ string }}
+  {% endtrans %}
+</div>
+<div>
+  {% trans %}
+    Pass-through: {{ string|passthrough }}
+  {% endtrans %}
+</div>
+<div>
+  {% trans %}
+    Placeholder: {{ string|placeholder }}
+  {% endtrans %}
+</div>

By default all tokens are prefixed with @ when sent to t(). If you want to "passthrough" the token (!) or use it as a placeholder (%), then use those filters on the token.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.php
@@ -0,0 +1,178 @@
+msgid "Escaped: @string"
+msgstr "ESCAPEE: @string"
+
+msgid "Pass-through: !string"
+msgstr "PAS-THRU: !string"
+
+msgid "Placeholder: %string"
+msgstr "PLAYSHOLDR: %string"

.po files work with any t() prefix now! w00t! Went ahead and created tests for these as well, they all passed on my local.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -32,6 +32,13 @@ public function getFunctions() {
+      /**
+       * The "raw" filter is not detectable with the trans tag. Create a faux
+       * filter here to assist so variables are passed-through un-escaped to
+       * the t() function.
+       */
+      'passthrough' => new \Twig_SimpleFilter('passthrough', 'twig_raw_filter'),
+      'placeholder' => new \Twig_SimpleFilter('placeholder', array('String', 'placeholder')),

As the comment suggests, there's really no way to detect the "raw" filter in the trans parser (not sure why, just doesn't show up). Created a "faux filter" which uses the raw filter function, but allows us to prepend the key sent to t() with ! so it doesn't get filtered. I would rather use "raw" (makes more sense), but can't figure out why it's not passed in the node construction (of the parser, see below). If anyone has better understanding of Twig and can make this work, please feel free. Otherwise, this may just have to be a minor compromise.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
@@ -127,13 +126,33 @@ protected function compileString(\Twig_NodeInterface $body)
+          $filter = '@';
+
+          if ($args instanceof \Twig_Node_Expression_Filter) {
+            switch ($args->getNode('filter')->getAttribute('value')) {
+              /**
+               * The "raw" filter is not detectable here. Use the duplicated
+               * passthrough filter here to assist so variables are
+               * passed-through un-escaped to the t() function.
+               */
+              case 'passthrough':
+                $filter = '!';
+                break;
+              case 'placeholder':
+                $filter = '%';
+                break;
+            }

This is where we detect how to construct the keys associated key names sent to t().

markhalliwell’s picture

PS. test is faster and overall patch is smaller now that we don't need a format_plural tag.

markhalliwell’s picture

Issue summary: View changes

blocking

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue tags: +Needs change record

.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

ZOMG. I <3 this :) Mark Carver++

I wouldn't worry to much about @name and @date, the .po files from 7 to 8 are probably going to need to change a lot, and if we change the names of our variables they will adjust. I know for testing purposes it's good to have things working, but there's absolutely no reason to add

{% set author = user.name %}
{% set date = node.created_date %}

into our code, we should instead change the names of the variables in the templates (and there are issues for that, see the tag for dreammarkup, and then update the po files to match.

markhalliwell’s picture

  • Added ability to use complex tokens (like: {{ author.name }}.
  • Added translation debug markup when twig_debug is enabled.
  • Added some more assertions to test.
  • Cleaned up some code and verbiage.
markhalliwell’s picture

An example of what the msgid should be when twig_debug is enabled.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work
Issue tags: -D8MI, -Needs change record, -Twig

The last submitted patch, drupal-handle-trans-block-1927584-81.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +D8MI, +Needs change record, +Twig

The last submitted patch, drupal-handle-trans-block-1927584-81.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
955 bytes
20.87 KB

Fixed 'twig_debug' not getting set in test properly.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

penyaskito’s picture

Issue tags: +sprint, +language-interface

Tagging for more exposure.

penyaskito’s picture

Status: Needs review » Needs work

I've never written a twig extension, so I cannot really discuss the approach used here.
Tests look really awesome, I think no edge case is missing there.

I'm about to RTBC it if tests are green and the following nitpicks are addressed, but would be great having another pair of eyes because of my lack of knowledge about the Twig system itself.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.phpundefined
@@ -32,6 +32,14 @@ public function getFunctions() {
+       *
+       * These filters are necessary to identify the type type of prefix to use

"type" is duplicated? I don't think we use empty strings in comments outside of docblocks.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,188 @@
+ * Definition of Drupal\Core\Template\TwigNodeTrans.

Should be "Contains \Drupal" (see http://drupal.org/node/1354#file)

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,188 @@
+ * A trans tag.

Change docblock to something like "The 'trans' tag allows the content string to be passed to the translation system for internationalization"

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,188 @@
+class TwigNodeTrans extends \Twig_Node {
+  public function __construct(\Twig_NodeInterface $body, \Twig_NodeInterface $plural = NULL, \Twig_Node_Expression $count = NULL, $lineno, $tag = NULL) {

Is the backslash really necessary here?
Maybe change lineno variable name to line_number for readability?

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,188 @@
+          /**
+           * Default prefix passed to t(), escapes printed token.
+           */

Should we use // comments instead?

+++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
@@ -0,0 +1,75 @@
+ * Definition of Drupal\Core\Template\TwigTransTokenParser.

"Contains \Drupal"

+++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
@@ -0,0 +1,75 @@
+class TwigTransTokenParser extends \Twig_TokenParser {

Is there a missing docblock?

penyaskito’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
20.06 KB
3.14 KB

Answering my own questions:

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,188 @@
+class TwigNodeTrans extends \Twig_Node {
+  public function __construct(\Twig_NodeInterface $body, \Twig_NodeInterface $plural = NULL, \Twig_Node_Expression $count = NULL, $lineno, $tag = NULL) {

Is the backslash really necessary here?
Maybe change lineno variable name to line_number for readability?

It is needed. Didn't change variable name because it is the same than the parent class (a Symfony class). Not sure if we should relax rules here for consistency.

Only comments changed in the patch.

joelpittet’s picture

Thank you @penyaskito! The comment cleanup is great:)

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Issue tags: +i18n

@Mark Carver I think this ready to go! Would like to get a once over from @Gábor Hojtsy or anybody on the i18n team. To make sure it meets their concerns to a point and it's something they can work with.

Since I created this issue, i'll just comment my:
RTBC +1

Gábor Hojtsy’s picture

I think its unfortunate that the twig template has

This {{ token.name }} has a length of: {{ count }}. It contains: {{ token.numbers|placeholder }} and {{ token.bad_text }}. Lets pass the bad text through: {{ token.bad_text|passthrough }}.

Which will end up in the translation system as

This @name has a length of: @count. It contains: %numbers and @bad_text. Lets pass the bad text through: !bad_text.

I see it is catering to use more standard twig approaches, it makes it harder to grep for certain strings, since they don't appear as-is in the template at all. Just wanted to note that this might make translator's life a bit harder. I see the proposal is that it makes templaters life so much easier (I guess).

Gábor Hojtsy’s picture

In other words, I don't have anything against this specifically. Drupal 8 already has like a dozen new APIs, so one more is really just a drop in the ocean... If people from the twig side agree this is the right thing, we can make parsing of this for pre-translation of this happen (for localize.drupal.org). I opened #2040089: Support for Drupal 8 twig %trans% template translatables for that.

jenlampton’s picture

@Gábor Hojtsy if you turn on twig_debug mode (by setting a variable in settings.php) then the string you want to see will appear in the source code of your Drupal site as a HTML comment. Something like:

<!-- TRANSLATION: "This @name has a length of: @count. It contains: %numbers and @bad_text. Lets pass the bad text through: !bad_text." -->

Does that help make the job easier for people working on translations?

Gábor Hojtsy’s picture

Probably yes.

star-szr’s picture

Status: Needs review » Needs work

First round of nitpicks as requested by @Mark Carver :)

And thank you very much @Gábor Hojtsy for taking a look.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.phpundefined
    @@ -32,6 +32,14 @@ public function getFunctions() {
       public function getFilters() {
         return array(
           't' => new \Twig_Filter_Function('t'),
    +      /**
    +       * Fake filters equivalent to "raw" and only used in the trans tag.
    +       *
    +       * These filters are necessary to identify the type of prefix to use
    +       * when passing tokens to t() from a trans tag.
    +       */
    +      'passthrough' => new \Twig_SimpleFilter('passthrough', 'twig_raw_filter'),
    +      'placeholder' => new \Twig_SimpleFilter('placeholder', 'twig_raw_filter'),
         );
       }
    

    The comment here should be an inline comment.

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
    @@ -0,0 +1,185 @@
    +class TwigNodeTrans extends \Twig_Node {
    +  public function __construct(\Twig_NodeInterface $body, \Twig_NodeInterface $plural = NULL, \Twig_Node_Expression $count = NULL, $lineno, $tag = NULL) {
    
    +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,84 @@
    +class TwigTransTokenParser extends \Twig_TokenParser {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function parse(\Twig_Token $token) {
    

    Need a blank line between the class definition and first method/property per https://drupal.org/node/608152. The constructor looks like it should probably have an @inheritdoc docblock as well.

  3. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
    @@ -0,0 +1,185 @@
    +  protected function compileString(\Twig_NodeInterface $body) {
    
    +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,84 @@
    +  public function decideForFork($token) {
    ...
    +  public function decideForEnd($token) {
    ...
    +  protected function checkTransString(\Twig_NodeInterface $body, $lineno) {
    

    These methods are missing docblocks. Some of them probably just need @inheritdoc.

  4. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
    @@ -0,0 +1,185 @@
    +  }
    +}
    
    +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,84 @@
    +  }
    +}
    

    Need a blank line between the last method and the end of the class, ref. https://drupal.org/node/608152

  5. +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,84 @@
    + * @see core\vendor\twig\twig\lib\Twig\TokenParser.php
    

    This seems a bit odd but I'm not sure how else to refer to it other than switching the backslashes to forward slashes. It's not namespaced code.

    https://drupal.org/node/1354#see

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,223 @@
    + * Definition of Drupal\system\Tests\Theme\TwigTransTest.
    

    Contains \Drupal\…

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,223 @@
    +  protected $admin_user;
    +  protected $langcode = 'xx';
    +  protected $name = 'Lolspeak';
    

    These should probably be documented. See https://drupal.org/node/1354#var.

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,223 @@
    +    $this->writeSettings($settings);
    +
    +    // Rebuild the service container and clear all caches.
    +    $this->rebuildContainer();
    +    $this->resetAll();
    

    I'm not sure if rebuilding the container and clearing caches is needed here, I would try just the writeSettings() and test locally.

  9. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,223 @@
    +    $this->assertRaw(
    +      '<!-- TRANSLATION: "This @name has a length of: @count. It contains: %numbers and @bad_text. Lets pass the bad text through: !bad_text." -->',
    +      'The "twig_debug" translation comment markup printed successfully for the above test.'
    +    );
    +
    +  }
    

    Extra blank line before end of method not needed.

  10. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,223 @@
    +  /**
    +   * Helper function: import a standalone .po file in a given language.
    +   * Borrowed from Drupal\locale\Tests\LocaleImportFunctionalTest.
    

    The second line should be its own paragraph (add a new line after the summary line) and the class should start with \Drupal\…

  11. +++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
    @@ -29,4 +29,14 @@ public function phpVariablesRender() {
    +  /**
    +   * Menu callback for testing translation blocks in a Twig template.
    +   */
    +  public function transBlockRender() {
    +    return array(
    +      '#theme' => 'twig_theme_test_trans',
    +    );
    +  }
    +
    +
     }
    

    Nit: extra blank line here.

drupalninja99’s picture

@Mark - at the sprint @Cottser thought I could take a pass at #97 code nitpicks since they are spelled out pretty clearly.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
20.18 KB

1. "The comment here should be an inline comment."
-done
2. "Need a blank line between the class definition and first method/property per https://drupal.org/node/608152. The constructor looks like it should probably have an @inheritdoc docblock as well."
-done and done
3. "These methods are missing docblocks. Some of them probably just need @inheritdoc."
-I added @inheritdoc to all of them (bc I'm lazy). Someone smarter can add more specific docs if they so choose
4. "Need a blank line between the last method and the end of the class…"
-done
5. "This seems a bit odd but I'm not sure how else to refer to it other than switching the backslashes to forward slashes. It's not namespaced code."
-I changed the slashes to forward slashes for now. That documentation page needs to be more clear how to handle files, I think it should include full paths
6. "Contains \Drupal\…"
-Fixed
7. "These should probably be documented. See https://drupal.org/node/1354#var."
-I looked at a bunch of scripts with $admin_user and none of them were documented, so I left that one
8. "I'm not sure if rebuilding the container and clearing caches is needed here, I would try just the writeSettings() and test locally."
-I removed the rebuild stuff and ran the tests locally. They all passed without it.
9. "Extra blank line before end of method not needed."
-Removed
10. "The second line should be its own paragraph (add a new line after the summary line) and the class should start with \Drupal\…"
-done
11. "Nit: extra blank line here."
-done

markhalliwell’s picture

Fixed some more doc issues, reworded some verbiage and changed "trans block" references to "trans tag", which is what they are.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Title: Handle trans block as Twig extension » Add support for the Twig {% trans %} tag extension

Renaming title to better reflect what we're doing.

Gábor Hojtsy’s picture

Fix D8MI subtag.

star-szr’s picture

Status: Needs review » Needs work

Nice @drupalninja99 and @Mark Carver :) Here's another pass:

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,251 @@
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    // Setup test_theme.
    +    theme_enable(array('test_theme'));
    +    \Drupal::config('system.theme')->set('default', 'test_theme')->save();
    +
    +    // Enable twig debug and write to the test settings.php file.
    +    $this->settingsSet('twig_debug', TRUE);
    +    $settings['settings']['twig_debug'] = (object) array(
    +      'value' => TRUE,
    +      'required' => TRUE,
    +    );
    +    $this->writeSettings($settings);
    

    I think we should definitely be testing both with and without twig_debug enabled! Maybe a helper function for all the non-twig_debug assertions.

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
    @@ -0,0 +1,206 @@
    +/**
    + * The 'trans' tag allows the content string to be passed to the translation
    + * system for internationalization.
    + */
    +class TwigNodeTrans extends \Twig_Node {
    

    This is too long for a summary line. If necessary split it up into a summary line and following paragraph. Ref. https://drupal.org/node/1354#drupal.

  3. +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,100 @@
    +class TwigTransTokenParser extends \Twig_TokenParser {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function parse(\Twig_Token $token) {
    

    Need a blank line between the class definition and first property/method.

  4. +++ b/core/lib/Drupal/Core/Template/TwigTransTokenParser.phpundefined
    @@ -0,0 +1,100 @@
    +  /**
    +   * Ensure that any nodes that are parsed are only of allowed types.
    +   *
    +   * @param \Twig_NodeInterface $body
    +   *   The expression to check.
    +   * @param integer $lineno
    +   *   The source line.
    +   */
    +  protected function checkTransString(\Twig_NodeInterface $body, $lineno) {
    ...
    +      throw new \Twig_Error_Syntax(sprintf('The text to be translated with "trans" can only contain references to simple variables'), $lineno);
    

    This should document that it @throws the Twig_Error_Syntax exception. See https://drupal.org/node/1354#throws.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,251 @@
    +    \Drupal::config('system.theme')->set('default', 'test_theme')->save();
    

    I think you can just use config() directly.

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,251 @@
    +    $this->drupalpost('admin/config/regional/settings', $edit, t('Save configuration'));
    

    $this->drupalPost()

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigTransTest.phpundefined
    @@ -0,0 +1,251 @@
    +  /**
    +   * Helper function: import a standalone .po file in a given language.
    +   *
    +   * Borrowed from \Drupal\locale\Tests\LocaleImportFunctionalTest.
    +   *
    +   * @param $contents
    +   *   Contents of the .po file to import.
    +   * @param $options
    +   *   Additional options to pass to the translation import form.
    +   */
    +  protected function importPoFile($contents, array $options = array()) {
    

    I realize this function is borrowed but let's add datatypes to the @params here.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
6.27 KB
23 KB

Fixed issues in #103.

markhalliwell’s picture

  1. The first test runs through the normal assertions and then check at the end that no debug markup is printed. I went ahead and added a second test that enables twig_debug and asserts that the markup is then printed.
  2. Fixed
  3. Fixed
  4. Fixed
  5. @see config() - That function is deprecated, the current implementation is correct.
  6. Fixed
  7. Fixed

Status: Needs review » Needs work

The last submitted patch, drupal-add-support-for-the-1927584-104.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
23.17 KB

Added comment block for new helper function.

Status: Needs review » Needs work

The last submitted patch, drupal-add-support-for-the-1927584-107.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
23.17 KB

Changed method name, put the "For" in the wrong place.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

FileSize
12.27 KB

Adding new test only patch, since the added test changed.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 1927584-tests.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review

Test only always fails. Complete patch in #109 passed.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.phpundefined
@@ -32,6 +32,13 @@ public function getFunctions() {
       't' => new \Twig_Filter_Function('t'),
+      // The "raw" filter is not detectable when parsing "trans" tags. To detect
+      // which prefix must be used for translation (@, !, %), we must clone the
+      // "raw" filter and give it identifiable names. These filters should only
+      // be used in "trans" tags.
+      // @see TwigNodeTrans::compileString()
+      'passthrough' => new \Twig_SimpleFilter('passthrough', 'twig_raw_filter'),
+      'placeholder' => new \Twig_SimpleFilter('placeholder', 'twig_raw_filter'),

This is a really nice idea!

Why Twig_SimpleFilter and not Twig_Filter_Function?

Also please add a 'trans' => new \Twig_Filter_Function('t')

here, so that it can be used both as block and simple string.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,205 @@
+   * {@inheritdoc}
+   */
+  public function compile(\Twig_Compiler $compiler) {
+    $compiler->addDebugInfo($this);

I think it should say here that this code is based heavily on i18n extension.

Besides the debug code and parameter handling it seems mostly identical.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,205 @@
+      if (settings()->get('twig_debug', FALSE)) {
+        $compiler->raw(" . '\n<!-- TRANSLATION: ");

I love this! Nice trick with appending the DEBUG code.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,205 @@
+      $compiler->write('echo ' . $function . '(');
+
+      // Move count in format_plural to first argument.
+      if (NULL !== $this->getNode('plural')) {
+        $compiler
+          ->raw('abs(')
+          ->subcompile($this->getNode('count'))
+          ->raw('),');
+      }
+
+      $compiler->subcompile($msg);
+
+      if (NULL !== $this->getNode('plural')) {
+        $compiler
+          ->raw(', ')
+          ->subcompile($msg1);
+      }

I am not a huge fan of code duplication.

Lets move this _identical_ chunk (in twig strtr is used) up before the if, k?

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,205 @@
+      foreach ($body as $node) {
+        if (get_class($node) === 'Twig_Node' && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
+          $node = $node->getNode(1);

This is from optimization.

I see what you are trying to do here, but this will look different with optimization off.

NVM: This is from twig upstream ... (I would not do it this way, but okay ...)

Have to ask @fabpot about it.

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.phpundefined
@@ -0,0 +1,205 @@
+          $argPrefix = '@';
+          while ($args instanceof \Twig_Node_Expression_Filter) {
+            switch ($args->getNode('filter')->getAttribute('value')) {
+              case 'passthrough':
+                $argPrefix = '!';
+                break;
+              case 'placeholder':

Again: This is _very_ very clever!

Props!

Besides what I found this is really almost RTBC! Great work!

Please keep it up!

jessebeach’s picture

I love the idea of providing a simpler syntax for the common use-case of printing a variable with a little string around it.

{% trans %}

I can't speak to the code without diving in, but the end-user approach is sound!

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
10.23 KB
23.08 KB

Refactored some code, added the 'trans' filter, updated some documentation.

Fabianx’s picture

Reviewed code, all feedback has been addressed, has extensive test coverage, is straightforward, based on Twig i18n extension and syntax compatible (another twig'ism supported by Drupal), makes syntax sooo much nicer for translated strings within Twig.

Has a very nice approach for Drupals different escape strategies on the tokens!

Lets get this in => This is RTBC!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #116

catch’s picture

Title: Add support for the Twig {% trans %} tag extension » Change notice: Add support for the Twig {% trans %} tag extension
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Patch looks great.

Committed/pushed to 8.x

Opened #2047095: Remove $submitted from node templates to actually apply this to the node template, there's probably others could be done as well.

fubhy’s picture

OH HAI TEH MUUN !!!!

markhalliwell’s picture

Title: Change notice: Add support for the Twig {% trans %} tag extension » Add support for the Twig {% trans %} tag extension
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record, -sprint

Added change notice: https://drupal.org/node/2047135

Xano’s picture

I like the feature, but not how themers should use it. What does trans mean? Translate? Transmission? Transatlantic flight? The word itself does not mean anything, and by looking at the code a layperson cannot figure out the meaning either. YesCT and I discussed config (short for configuration) last week. Trans is even less self-descriptive. For better DX and helping newcomers get going with this, can we please get a follow-up that will let us just write translation instead?

markhalliwell’s picture

@Xano: This has actually been debated in #72 and #73 here... but mostly [and heatedly] in IRC.

The general consensus on why we're sticking with {% trans %} is because of http://twig.sensiolabs.org/doc/extensions/i18n.html

It is an already established tag in the Twig community, we didn't create it.

markhalliwell’s picture

Short answer, no we're not going to create a follow-up issue to change it.

SeanKelly’s picture

Per issue #2047227: Update templates to leverage support for the Twig {% trans %} tag extension I've started updating templates to use this improved syntax.

Gábor Hojtsy’s picture

Unfortunately this did not include adding support for string contexts, so you cannot for example translate 'May' in the template as a long month name. It will always translate as a short month name. That is kind of a big deal. See #2049241: Add support for language options to the Twig {% trans %} tag extension.

giorgio79’s picture

How about letting contrib handle this like https://drupal.org/project/submitted_by

dlu’s picture

Component: base system » theme system

Moved to theme system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

Status: Fixed » Closed (fixed)

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

Jeff Burnz’s picture

Is this currently usable in Drupal, or are there outstanding issues that I can't find right now, when I do this:

    <p class="submitted">
      {% trans %}Submitted by {{ author }} on {{ created }}{% endtrans %}
    </p>

And RDF is on, you get this:

Submitted by <span rel="schema:author"><a href="/user/1" title="View user profile." class="username" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</a></span> on Wed, 09/11/2013 - 16:29

Gábor Hojtsy’s picture

@Jeff: if you mean how it expands to that, then that would not be up to translation, it is how the author gets rendered/themed.

Jeff Burnz’s picture

FileSize
237.92 KB

Ops, probably not clear at all, what I mean is that is how its printed to screen, thats not "code view", thats what I see in the front end, looking at the node or comment. Here, I post a screenshot to show what I mean.

The RDF was a red herring, it happens anyway, I was just doing different things with various tags inside trans and at one point found if I turned RDF off things worked ok, sorry I can't give more information, but the code I am using is as above - basically strait from the change notice notes on trans.

Gábor Hojtsy’s picture

Yeah, so the author is escaped because by default placeholder items are escaped. The following would avoid escaping and let it go through:

<p class="submitted">
      {% trans %}Submitted by {{ author|passthrough }} on {{ created }}{% endtrans %}
</p>

I think this is somewhat dangerous because if author does not have markup for some reason (eg. preprocessing made it not include links or other markup, then this may be an XSS vector. This is not evident or possible to trace back from the twig syntax. The processing logic for author itself should make sure if there is no markup then at least an escaping process is run on the name, so this does not become an XSS vector.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.