Patch comment: #19

Problem/Motivation

In #1927584: Add support for the Twig {% trans %} tag extension we added support for {% trans %} tags. However, this lacks support for passing $options to t() and format_plural() for things like "context" and "langcode":

{% trans %}
  January
{% endtrans %}

vs.

{{ "January"|t([],{'context': 'Long month name', 'langcode': 'fr'}) }}

Proposed resolution

Add an optional with {...} parameter to the {% trans %} tag that maps to the t() and format_plural() $options array:

{% trans with {'context': 'Long month name', 'langcode': 'fr'} %}
  January
{% endtrans %}

Example debug output:

<!-- TRANSLATION: "January", CONTEXT: "Long month name", LANGCODE: "fr" -->

Completed Tasks

  1. Discuss best approach (feedback from Twig team).
  2. Get a working patch, with new tests.

Remaining Tasks (@todo after commit)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

penyaskito’s picture

Symfony does not have support for contexts AFAIS, so they probably don't need support for this or it would be too much work.

penyaskito’s picture

Issue summary: View changes

tasks were not done.

penyaskito’s picture

Issue summary: View changes

symfony support not needed

Gábor Hojtsy’s picture

When I wrote http://hojtsy.hu/blog/2013-jul-24/drupal-8-multilingual-tidbits-10-conte... I did not think the missing context support may be that big of an issue in templates, but month names is a good example where you need to have it, so you can use the right translation for 'May'.

Also, templates usually use short strings, so support for context would be good generally.

markhalliwell’s picture

This doesn't make sense to me... what are y'all trying to do here? Why are you using the |t filter inside the {% trans %} tag... that's a little redundant:

{% trans %}
  {{ month|t('Long month name') }}
{% endtrans %}

Is recursively calling t(): t('@month', array('@month' => t('Long month name'))

{% trans %}
  Provided by {{'views'|t('module name') }}
{% endtrans %}

Is recursively calling t(): Provided by t('@views', array('@views' => t('module name'))

In reality, these Twig variables ({{ month }} and {{ module }}) should be provided via context in the preprocess theming layer. If they're not, you can manually {% set %} them before using {% trans %}.

From the change record:

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 %}
Gábor Hojtsy’s picture

@Mark: in short these will return different strings:

$short_month_may = t('May');
$long_month_may = t('May', array(), array('context' => 'Long month name');

You cannot reproduce both in twig, so you are forced to move those to preprocesses and in case your short string needs a context, you need to *not* include that in the template?

penyaskito’s picture

Status: Active » Needs review
FileSize
5.09 KB

Starting patch.

This allows to do:

{% trans context 'Long month name' %}
{{ month }}
{% endtrans %}

However, @markcarver suggested to use {% trans context='Long month name' %} and he is working on it.

Just upload my patch for the record, and maybe he can benefit from something of it.

penyaskito’s picture

Issue summary: View changes

del, not strike

markhalliwell’s picture

Ok, here is a little more work done on the TwigTransTokenParser so it handles the inline string argument properly

Status: Needs review » Needs work

The last submitted patch, drupal-context-support-for-2049241-6.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
8.26 KB

Removed dagnabbit tabs

markhalliwell’s picture

Component: comment.module » base system

Here's the interdiff for #5 -> #8

markhalliwell’s picture

FileSize
8.14 KB

........... fail...........might help if I actually upload it....

markhalliwell’s picture

Assigned: Unassigned » markhalliwell
Fabianx’s picture

Status: Needs review » Needs work

There is a handy function in TwigToken parser to get a list of attributes that can also be named (getAttributes - grep for it). I suggest switching to:

{% trans(context="context_123") %}

would be a good idea.

That would also allow further extensions in a clean way.

Otherwise: Good work already, nice code. Besides the syntax change this is almost RTBC. :-)

Fabianx’s picture

Mark Carver and me agreed in IRC on using the syntax of:

{% trans with options {context: "context_123"} %}

similar to how "include" works.

webchick’s picture

Dang, really? The syntax in #5 is sooo much nicer. :(

Is this for performance, or why would we do it as in #13?

markhalliwell’s picture

Its for keeping consistent syntax with other Twig tags: http://twig.sensiolabs.org/doc/tags/include.html

Although in retrospect we should probably drop the "options" and just do:

{% trans with {context: "context_123"} %}

markhalliwell’s picture

@penyaskito and I agreed (in IRC) that "with" kind of implies "options". Having "options" in the syntax feels a little too verbose. I'll get a revised patch uploaded later tonight to make the syntax work like:
{% trans with {'context': 'context_123', 'langcode': 'de'} %}

markhalliwell’s picture

Also to further clarify @webchick's concerns in #14: the "with {}" is basically a direct map to the t() and format_plural() $options parameter (which in theory allows for easier extension in the future if more "options" are added).

webchick’s picture

Ok, I guess that's fair enough. And yeah, removing "options" seems good to me if we can get away with it.

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
13.26 KB
14.63 KB
10.31 KB

Ok, here is a test only, complete and interdiff patches. All tests passed locally. This is a pretty big interdiff, but I realized I needed to refactor my original trans test so it would create "two" test languages. This is necessary so we can verify that using both {% trans with {'context': 'Lolspeak'} %} (the first test language) and {% trans with {'context': 'Lolspeak', 'langcode': 'zz'} %} (the second test language) actually work as expected.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary to make examples correct.

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.

Fabianx’s picture

On first glance looks really good!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed this again and this is ready IMO:

* Passes tests
* Had several rounds of review
* Has nice syntax
* And makes the i18n team happy

=> Full win!

penyaskito’s picture

+1 RTBC

Awesome! I would think about changing the issue subject to "Support same options than t() in the Twig {% trans %} tag extension", because there is also support (and test coverage) for target language explicitly.

markcarver++

markhalliwell’s picture

Title: Context support for the Twig {% trans %} tag extension » Add support for language options to the Twig {% trans %} tag extension
markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

I don't have a syntax preference either way, so long as people who will use it are fine with it and say it is as close to the spirit of twig as possible, I'm good :)

webchick’s picture

Title: Add support for language options to the Twig {% trans %} tag extension » Change notice: Add support for language options to the Twig {% trans %} tag extension
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Well, those are certainly some, er, colourful tests. :P

Looks like this has sign-off from Gábor and the Twig team, and the syntax falls in-line with what Twig developers would expect. I read the patch through and nothing stood out to me. Therefore...

Committed and pushed to 8.x. Thanks!

The change notice at https://drupal.org/node/2047135 will need an update, so bumping to active for that.

Gábor Hojtsy’s picture

@Mark Carver: can you help with updating the change notice? Thanks a lot!

markhalliwell’s picture

Title: Change notice: Add support for language options to the Twig {% trans %} tag extension » Add support for language options to the Twig {% trans %} tag extension
Priority: Critical » Normal
Status: Active » Fixed

Done :)

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Gábor Hojtsy’s picture

Sprint tag just does not want to go away :D

xjm’s picture

Priority: Normal » Major
Status: Closed (fixed) » Active

There's no "Change records for this issue" section in the summary. At the least this issue needs to be added in the issue list to whatever change notice is referenced in the comments above. Thanks!

Also remove the tag and restore status/priority once it's done.

Fabianx’s picture

Priority: Major » Normal
Status: Active » Closed (fixed)
Issue tags: -Needs change record

Added change record link and removed tag. Thanks!

Fabianx’s picture

Issue summary: View changes

Updated issue summary.