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
Discuss best approach (feedback from Twig team).Get a working patch, with new tests.
Remaining Tasks (@todo after commit)
- Update Twig coding standards documentation.
- Update change record: Added support for the Twig {% trans %} tag extension
Comment | File | Size | Author |
---|---|---|---|
#19 | 2049241-19-tests.patch | 10.31 KB | markhalliwell |
#19 | 2049241-19-complete.patch | 14.63 KB | markhalliwell |
#19 | interdiff.txt | 13.26 KB | markhalliwell |
#10 | interdiff.txt | 8.14 KB | markhalliwell |
#8 | drupal-context-support-for-2049241-8.patch | 8.26 KB | markhalliwell |
Comments
Comment #1
penyaskitoSymfony does not have support for contexts AFAIS, so they probably don't need support for this or it would be too much work.
Comment #1.0
penyaskitotasks were not done.
Comment #1.1
penyaskitosymfony support not needed
Comment #2
Gábor HojtsyWhen 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.
Comment #3
markhalliwellThis 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:Is recursively calling t():
t('@month', array('@month' => t('Long month name'))
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:
Comment #4
Gábor Hojtsy@Mark: in short these will return different strings:
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?
Comment #5
penyaskitoStarting patch.
This allows to do:
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.
Comment #5.0
penyaskitodel, not strike
Comment #6
markhalliwellOk, here is a little more work done on the TwigTransTokenParser so it handles the inline string argument properly
Comment #8
markhalliwellRemoved dagnabbit tabs
Comment #9
markhalliwellHere's the interdiff for #5 -> #8
Comment #10
markhalliwell........... fail...........might help if I actually upload it....
Comment #11
markhalliwellComment #12
Fabianx CreditAttribution: Fabianx commentedThere 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. :-)
Comment #13
Fabianx CreditAttribution: Fabianx commentedMark Carver and me agreed in IRC on using the syntax of:
{% trans with options {context: "context_123"} %}
similar to how "include" works.
Comment #14
webchickDang, really? The syntax in #5 is sooo much nicer. :(
Is this for performance, or why would we do it as in #13?
Comment #15
markhalliwellIts 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"} %}
Comment #16
markhalliwell@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'} %}
Comment #17
markhalliwellAlso 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).Comment #18
webchickOk, I guess that's fair enough. And yeah, removing "options" seems good to me if we can get away with it.
Comment #19
markhalliwellOk, 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.Comment #19.0
markhalliwellUpdated issue summary to make examples correct.
Comment #19.1
markhalliwellUpdated issue summary.
Comment #19.2
markhalliwellUpdated issue summary.
Comment #19.3
markhalliwellUpdated issue summary.
Comment #20
Fabianx CreditAttribution: Fabianx commentedOn first glance looks really good!
Comment #21
Fabianx CreditAttribution: Fabianx commentedI 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!
Comment #22
penyaskito+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++
Comment #23
markhalliwellComment #23.0
markhalliwellUpdated issue summary.
Comment #24
Gábor HojtsyI 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 :)
Comment #25
webchickWell, 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.
Comment #26
Gábor Hojtsy@Mark Carver: can you help with updating the change notice? Thanks a lot!
Comment #27
markhalliwellDone :)
Comment #29
Gábor HojtsyYay, thanks!
Comment #30
Gábor HojtsySprint tag just does not want to go away :D
Comment #31
xjmThere'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.
Comment #32
Fabianx CreditAttribution: Fabianx commentedAdded change record link and removed tag. Thanks!
Comment #32.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.