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

Files: 
CommentFileSizeAuthor
#131 trans-output.png237.92 KBJeff Burnz
#115 drupal-add-support-for-the-1927584-115.patch23.08 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,290 pass(es).
[ View ]
#115 interdiff.txt10.23 KBMark Carver
#110 1927584-tests.patch12.27 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] 57,301 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#109 drupal-add-support-for-the-1927584-107.patch23.17 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,436 pass(es).
[ View ]
#109 interdiff.txt1.25 KBMark Carver
#107 drupal-add-support-for-the-1927584-107.patch23.17 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#107 interdiff.txt1.25 KBMark Carver
#104 drupal-add-support-for-the-1927584-104.patch23 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#104 interdiff.txt6.27 KBMark Carver
#100 drupal-handle-trans-block-1927584-100.patch22.7 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,147 pass(es).
[ View ]
#100 interdiff.txt6.96 KBMark Carver
#99 1927584-complete-99.patch20.18 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 57,042 pass(es).
[ View ]
#99 interdiff.txt5.99 KBdrupalninja99
#91 1927584-tests.patch11.43 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] 56,621 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#91 1927584-complete.patch21.28 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 56,991 pass(es).
[ View ]
#89 drupal-handle-trans-block-1927584.86-89.txt3.14 KBpenyaskito
#89 drupal-handle-trans-block-1927584-89.patch20.06 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#86 drupal-handle-trans-block-1927584-86.patch20.87 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es).
[ View ]
#86 interdiff.txt955 bytesMark Carver
#82 Screen Shot 2013-07-10 at 12.08.09 AM.png31.93 KBMark Carver
#81 drupal-handle-trans-block-1927584-81.patch20.67 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] 57,003 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#81 interdiff.txt13.97 KBMark Carver
#75 interdiff.txt25.58 KBMark Carver
#74 drupal-handle-trans-block-1927584-74.patch17.63 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]
#68 drupal-handle-trans-block-1927584-68.patch25.15 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] 56,851 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#68 interdiff.txt6.23 KBMark Carver
#55 drupal-handle-trans-block-1927584-55.patch28.63 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 56,900 pass(es).
[ View ]
#55 interdiff.txt27.64 KBMark Carver
#53 1927584-53.patch27.52 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 56,938 pass(es).
[ View ]
#53 interdiff.txt720 bytesCottser
#50 drupal-handle-trans-block-1927584-50.patch29.72 KBMark Carver
FAILED: [[SimpleTest]]: [MySQL] 56,886 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#50 interdiff.txt7.93 KBMark Carver
#45 1927584-45.patch27.28 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] 56,823 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#42 1927584-42.patch962.07 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1927584-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#42 interdiff.txt1.92 KBdrupalninja99
#37 1927584-37.patch27.36 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 56,545 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#37 interdiff.txt2.48 KBCottser
#34 drupal-twigtranstest-1927584-34.patch27.42 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 56,462 pass(es), 5 fail(s), and 222 exception(s).
[ View ]
#34 interdiff.txt1.21 KBjenlampton
#24 drupal-twigtranstest-1927584-24.patch27.43 KBJohn Bickar
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#12 1927584.patch27.43 KBgeoffreyr
FAILED: [[SimpleTest]]: [MySQL] 55,822 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#12 1927584-interdiff.txt23.2 KBgeoffreyr
#8 drupal-1927584.patch7.5 KBezeedub
PASSED: [[SimpleTest]]: [MySQL] 54,497 pass(es).
[ View ]
#8 interdiff.txt458 bytesezeedub
#7 drupal-1927584.patch815 bytesezeedub
FAILED: [[SimpleTest]]: [MySQL] 51,769 pass(es), 1,012 fail(s), and 105 exception(s).
[ View ]
#4 twig_trans_block-1927584-4.patch6.58 KBezeedub
PASSED: [[SimpleTest]]: [MySQL] 54,471 pass(es).
[ View ]

Comments

Issue tags:+Twig

Adding tag

Category:feature» task

Assigned:joelpittet» ezeedub

StatusFileSize
new6.58 KB
PASSED: [[SimpleTest]]: [MySQL] 54,471 pass(es).
[ View ]

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!

Title:Add format_plural to twig filtersHandle trans block as Twig extension

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

Status:Active» Needs review
StatusFileSize
new815 bytes
FAILED: [[SimpleTest]]: [MySQL] 51,769 pass(es), 1,012 fail(s), and 105 exception(s).
[ View ]

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

StatusFileSize
new458 bytes
new7.5 KB
PASSED: [[SimpleTest]]: [MySQL] 54,497 pass(es).
[ View ]

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

Assigned:ezeedub» geoffreyr

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.

Status:Needs review» Needs work

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

StatusFileSize
new23.2 KB
new27.43 KB
FAILED: [[SimpleTest]]: [MySQL] 55,822 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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.

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.

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

Priority:Normal» Major

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

Issue tags:+D8MI

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

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

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.

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

Assigned:geoffreyr» Unassigned

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

Issue summary:View changes

task not feature

Issue summary:View changes

better why

Issue summary:View changes

this

StatusFileSize
new27.43 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

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

Status:Needs review» Needs work

Did not mean to set to needs review.

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

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.

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

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

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

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.

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,
    ));

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new27.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,462 pass(es), 5 fail(s), and 222 exception(s).
[ View ]

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

Issue summary:View changes

symbols

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

Issue summary:View changes

added another gist example :)

Status:Needs review» Needs work

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

Issue summary:View changes

dream markup usage

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
new27.36 KB
FAILED: [[SimpleTest]]: [MySQL] 56,545 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

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

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

@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 :/

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new962.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1927584-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new27.28 KB
FAILED: [[SimpleTest]]: [MySQL] 56,823 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Okay sorry, here let's try that again

Status:Needs review» Needs work

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

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?

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

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

Status:Needs work» Needs review
StatusFileSize
new7.93 KB
new29.72 KB
FAILED: [[SimpleTest]]: [MySQL] 56,886 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new720 bytes
new27.52 KB
PASSED: [[SimpleTest]]: [MySQL] 56,938 pass(es).
[ View ]

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!

Assigned:Unassigned» Mark Carver
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.

Status:Needs work» Needs review
StatusFileSize
new27.64 KB
new28.63 KB
PASSED: [[SimpleTest]]: [MySQL] 56,900 pass(es).
[ View ]

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.

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.

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

Also, OMG amazing work everybody!

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.

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.

Status:Needs review» Needs work

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

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

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'

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

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

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

Status:Needs work» Needs review
StatusFileSize
new6.23 KB
new25.15 KB
FAILED: [[SimpleTest]]: [MySQL] 56,851 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

Still need to figure out #67.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new17.63 KB
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]

Lots of changes, summary in next comment

StatusFileSize
new25.58 KB

interdiff for #74

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

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

Issue summary:View changes

blocking

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue tags:+Needs change record

.

Issue summary:View changes

Updated issue summary.

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.

StatusFileSize
new13.97 KB
new20.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,003 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
  • 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.

StatusFileSize
new31.93 KB

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new955 bytes
new20.87 KB
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es).
[ View ]

Fixed 'twig_debug' not getting set in test properly.

Issue summary:View changes

Updated issue summary.

Issue tags:+sprint, +language-interface

Tagging for more exposure.

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?

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new20.06 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.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.

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new21.28 KB
PASSED: [[SimpleTest]]: [MySQL] 56,991 pass(es).
[ View ]
new11.43 KB
FAILED: [[SimpleTest]]: [MySQL] 56,621 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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

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

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.

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

Probably yes.

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.

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

Status:Needs work» Needs review
StatusFileSize
new5.99 KB
new20.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,042 pass(es).
[ View ]

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

StatusFileSize
new6.96 KB
new22.7 KB
PASSED: [[SimpleTest]]: [MySQL] 57,147 pass(es).
[ View ]

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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

Renaming title to better reflect what we're doing.

Fix D8MI subtag.

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.

Status:Needs work» Needs review
StatusFileSize
new6.27 KB
new23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixed issues in #103.

  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.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new23.17 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new23.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,436 pass(es).
[ View ]

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

Issue summary:View changes

Updated issue summary.

StatusFileSize
new12.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,301 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

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

Issue summary:View changes

Updated issue summary.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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!

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!

Status:Needs work» Needs review
StatusFileSize
new10.23 KB
new23.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,290 pass(es).
[ View ]

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

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!

Status:Needs review» Reviewed & tested by the community

RTBC as per #116

Title:Add support for the Twig {% trans %} tag extensionChange 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.

OH HAI TEH MUUN !!!!

Title:Change notice: Add support for the Twig {% trans %} tag extensionAdd 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

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?

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

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

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

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.

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

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.

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

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

StatusFileSize
new237.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.

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.

Issue summary:View changes

Updated issue summary.