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
- Add the Twig i18n extension
- Rewrite the i18n extension and override it in a way that it maps to our functions.
- Change the usage of gettext() to t() and format_plural().
- Create tests for the
{% trans %}
tag that maps to t() and format_plural() from templates. - Ensure the
{% trans %}
tag works with String::format() (used by t() and format_plural()) - Ensure strings are translatable using .po files.
- Wish: Provide debug markup for which translation should be used.
Remaining Tasks (@todo after commit)
- Update documentation.
- Create change notification.
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
Comment | File | Size | Author |
---|---|---|---|
#131 | trans-output.png | 237.92 KB | Jeff Burnz |
#115 | drupal-add-support-for-the-1927584-115.patch | 23.08 KB | markhalliwell |
#115 | interdiff.txt | 10.23 KB | markhalliwell |
#110 | 1927584-tests.patch | 12.27 KB | markhalliwell |
#109 | drupal-add-support-for-the-1927584-107.patch | 23.17 KB | markhalliwell |
Comments
Comment #1
joelpittetAdding tag
Comment #2
joelpittetComment #3
ezeedub CreditAttribution: ezeedub commentedComment #4
ezeedub CreditAttribution: ezeedub commentedHere'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.
This works.
This doesn't work yet.
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!
Comment #5
ezeedub CreditAttribution: ezeedub commentedComment #6
disasm CreditAttribution: disasm commentedNormally, 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.
Comment #7
ezeedub CreditAttribution: ezeedub commentedAh thx Sam, was updating eclipse when I did this.
Comment #8
ezeedub CreditAttribution: ezeedub commentedLet's try that again including new files (trying out drush iq-submit).
Comment #9
geoffreyr CreditAttribution: geoffreyr commentedComment #10
geoffreyr CreditAttribution: geoffreyr commentedAm 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.
This also works.
This is fine.
This still doesn't work, it prints out an empty space where "stars" should go.
Comment #11
geoffreyr CreditAttribution: geoffreyr commentedCurrent status: got trans block working with variable substitution and plural tag. format_plural still in progress.
Comment #12
geoffreyr CreditAttribution: geoffreyr commentedOK, 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.
Comment #13
joelpittetTest bot activate! Thank you @geoffreyr you were awesome at the sprints and great to meet you!
Comment #15
joelpittet#12: 1927584.patch queued for re-testing.
Comment #17
geoffreyr CreditAttribution: geoffreyr commented@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.
Comment #18
jenlamptonBumping this up to major since some of our other template conversions will be held up by trans :)
Comment #19
andypostthis could lead us to
Language::FRONTEND
which could be useful for theme layer as custom translationsComment #20
andypostFiled #2015575: Introduce Language::FRONTEND and start separate format_plural from theme and form to allow language able to detect a twig/template-render
Comment #21
Gábor HojtsyIt 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.
Comment #22
drupalninja99 CreditAttribution: drupalninja99 commentedThis seems liek a bug "$this->assertText($out, $out);" line 80 in TwigTransFormatPluralTest.php.
Comment #23
jenlamptonUnassigning in hopes of getting someone more movement on this one.
Comment #23.0
jenlamptontask not feature
Comment #23.1
jenlamptonbetter why
Comment #23.2
jenlamptonthis
Comment #24
John Bickar CreditAttribution: John Bickar commentedNot setting to needs review yet b/c I want to test with simplytest.me first.
Comment #25
John Bickar CreditAttribution: John Bickar commentedLooks 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.)
Comment #26
John Bickar CreditAttribution: John Bickar commentedDid not mean to set to needs review.
Comment #27
drupalninja99 CreditAttribution: drupalninja99 commented@John, you are correct, I get the same thing.
Comment #28
drupalninja99 CreditAttribution: drupalninja99 commentedI keep getting whitespace errors from https://drupal.org/files/drupal-twigtranstest-1927584-24.patch
Comment #29
John Bickar CreditAttribution: John Bickar commentedThe 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
Comment #30
drupalninja99 CreditAttribution: drupalninja99 commentedI 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.
Comment #31
star-szrtwig_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 ;)
Comment #32
drupalninja99 CreditAttribution: drupalninja99 commentedK 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.
Comment #33
drupalninja99 CreditAttribution: drupalninja99 commentedOK 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'?
Comment #34
jenlampton@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.
Comment #34.0
jenlamptonsymbols
Comment #35
jenlamptonAnyone know how to test this? Can you add a note to the issue summary for our reviewers? :)
Comment #35.0
jenlamptonadded another gist example :)
Comment #36.0
(not verified) CreditAttribution: commenteddream markup usage
Comment #37
star-szrJust a patch to move things along slightly, @jenlampton and I were both looking at this last night.
Comment #39
drupalninja99 CreditAttribution: drupalninja99 commented@Cottser, this looks like it's on the right track. Has anyone been able to actually manually get the test translation page to work?
Comment #40
drupalninja99 CreditAttribution: drupalninja99 commentedThis line "lolspeak_langcode = 'xx-lolspeak';" is not being used, can it be removed from the test?
Comment #41
star-szr@drupalninja99 - yes, I don't see why not.
I missed a tab in this file :/
Comment #42
drupalninja99 CreditAttribution: drupalninja99 commentedI 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:
So confusingly it only is partially working. I don't know why Hello star didn't translate.
Comment #44
joelpittet@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.
Comment #45
drupalninja99 CreditAttribution: drupalninja99 commentedOkay sorry, here let's try that again
Comment #47
jenlamptonThis 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?
Comment #48
drupalninja99 CreditAttribution: drupalninja99 commentedSeems like all of the tests are failing to me:
Comment #49
markhalliwellYeah, I've been looking at this too. Jump on irc?
Comment #50
markhalliwellOk. 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.
Comment #51
markhalliwellI 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.
Removed unused classes.
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.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?
This was probably the main culprit. The page wasn't redirecting to the correct language so the translated assertions would fail.
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 twigplural
modifier in thetrans
block that interacts with format_plural().Moved this into the
setUp()
method. Renamed it so it better reflects the test name.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.
The new lines needed to be double escaped.
Pretty self explanatory, but this should be the {{ count }} that's specified in the template.
Renamed the variable name to something plural and shorter, to coincide with something that uses format_plural, like "comments".
Comment #52
markhalliwellI 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.Comment #53
star-szrThanks Mark! Looking much better :)
As for the plural, what Gábor said in #21 seems to be true:
I took a look at \Drupal\locale\Tests\LocalePluralFormatTest for an example and updated the provided .po file accordingly. Looks green to me!
Comment #54
markhalliwellYay! I love that color. Now that the test is passing, I'll focus on getting the last few things done:
format_plural
twig blockComment #55
markhalliwellOk, summary of this patch (and why interdiff.txt is so large):
format_plural
code block and properly (and successfully!) tested them.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.
Comment #56
markhalliwellHere's that error message. It returns a 500 page when trying to do invalid trans/format_plural blocks:
Comment #57
joelpittetI'm on board with that, we don't really need to test syntax errors.
Also, OMG amazing work everybody!
Comment #58
star-szrNow that we have things working :) going back to Gábor in #21:
Can we reconcile this difference? I don't think we should be introducing another placeholder format just for translatables in Twig templates.
Comment #59
markhalliwellI was wondering about this problem myself.
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.
Comment #60
markhalliwellComment #61
markhalliwellIn retrospect, I don't even think we would need the ending symbol once it gets converted.
Comment #62
joelpittetI'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>.
Comment #63
joelpittetIf 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'Comment #64
markhalliwellRef: #2011442: Support for Drupal 8 twig t filter translatables
Comment #65
markhalliwellTo 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 howt()
andformat_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) }}
.Comment #66
joelpittetWe were looking also into doing something like this:
{% trans %} Text {{ !foo }} {{ @bar }} {{ %baz }} {{ booyah }} {% endtrans %}?
But those variable names seem to fail when prefixed...
Comment #67
joelpittetThis may help... but must take a look later
{% trans(foo, bar, baz) %} Text !foo @bar %baz {{ booyah }} {% endtrans %}
Comment #68
markhalliwellJust some cleanup: removed rogue dpm() and the invalid tests per #57
Comment #69
markhalliwellStill need to figure out #67.
Comment #70
markhalliwell#68: drupal-handle-trans-block-1927584-68.patch queued for re-testing.
Comment #72
markhalliwellOk, 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:
trans
tag totranslate
for better readability. We should also differentiate this from thet
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 thet()
andformat_plural()
functions entirely and used only in twig templates.format_plural
tag entirely. It is completely redundant at this point.format_plural()
runs throught()
.!, @, %
issue otherwise.And of course:
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.Comment #73
jenlamptonThis is what I want:
{{ 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 usedtrans
will see that we are doing the same thing in Drupal.'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.Comment #74
markhalliwellLots of changes, summary in next comment
Comment #75
markhalliwellinterdiff for #74
Comment #76
markhalliwellExample of @jenlampton's code (#73) with this new method:
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:
Removed the format_plural tag... it's redundant since it can be handled with the trans tag.
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.
.po files work with any t() prefix now! w00t! Went ahead and created tests for these as well, they all passed on my local.
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.
This is where we detect how to construct the keys associated key names sent to t().
Comment #77
markhalliwellPS. test is faster and overall patch is smaller now that we don't need a
format_plural
tag.Comment #77.0
markhalliwellblocking
Comment #77.1
markhalliwellUpdated issue summary.
Comment #77.2
markhalliwellUpdated issue summary.
Comment #77.3
markhalliwellUpdated issue summary.
Comment #77.4
markhalliwellUpdated issue summary.
Comment #78
markhalliwellUpdated issue summary.
Comment #78.0
markhalliwellUpdated issue summary.
Comment #78.1
markhalliwellUpdated issue summary.
Comment #78.2
markhalliwellUpdated issue summary.
Comment #78.3
markhalliwellUpdated issue summary.
Comment #78.4
markhalliwellUpdated issue summary.
Comment #78.5
markhalliwellUpdated issue summary.
Comment #78.6
markhalliwellUpdated issue summary.
Comment #79
markhalliwell.
Comment #79.0
markhalliwellUpdated issue summary.
Comment #80
jenlamptonZOMG. 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
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.
Comment #81
markhalliwell{{ author.name }}
.Comment #82
markhalliwellAn example of what the msgid should be when
twig_debug
is enabled.Comment #82.0
markhalliwellUpdated issue summary.
Comment #82.1
markhalliwellUpdated issue summary.
Comment #82.2
markhalliwellUpdated issue summary.
Comment #82.3
markhalliwellUpdated issue summary.
Comment #82.4
markhalliwellUpdated issue summary.
Comment #82.5
markhalliwellUpdated issue summary.
Comment #84
markhalliwell#81: drupal-handle-trans-block-1927584-81.patch queued for re-testing.
Comment #86
markhalliwellFixed 'twig_debug' not getting set in test properly.
Comment #86.0
markhalliwellUpdated issue summary.
Comment #87
penyaskitoTagging for more exposure.
Comment #88
penyaskitoI'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.
"type" is duplicated? I don't think we use empty strings in comments outside of docblocks.
Should be "Contains \Drupal" (see http://drupal.org/node/1354#file)
Change docblock to something like "The 'trans' tag allows the content string to be passed to the translation system for internationalization"
Is the backslash really necessary here?
Maybe change lineno variable name to line_number for readability?
Should we use // comments instead?
"Contains \Drupal"
Is there a missing docblock?
Comment #88.0
penyaskitoUpdated issue summary.
Comment #88.1
markhalliwellUpdated issue summary.
Comment #88.2
markhalliwellUpdated issue summary.
Comment #89
penyaskitoAnswering my own questions:
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.
Comment #90
joelpittetThank you @penyaskito! The comment cleanup is great:)
Comment #90.0
joelpittetUpdated issue summary.
Comment #90.1
markhalliwellUpdated issue summary.
Comment #91
markhalliwellUploading test only patch per https://drupal.org/contributor-tasks/write-tests
Comment #91.0
markhalliwellUpdated issue summary.
Comment #91.1
markhalliwellUpdated issue summary.
Comment #92
joelpittet@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
Comment #93
Gábor HojtsyI think its unfortunate that the twig template has
Which will end up in the translation system as
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).
Comment #94
Gábor HojtsyIn 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.
Comment #95
jenlampton@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:
Does that help make the job easier for people working on translations?
Comment #96
Gábor HojtsyProbably yes.
Comment #97
star-szrFirst round of nitpicks as requested by @Mark Carver :)
And thank you very much @Gábor Hojtsy for taking a look.
The comment here should be an inline comment.
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.
These methods are missing docblocks. Some of them probably just need @inheritdoc.
Need a blank line between the last method and the end of the class, ref. https://drupal.org/node/608152
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
Contains \Drupal\…
These should probably be documented. See https://drupal.org/node/1354#var.
I'm not sure if rebuilding the container and clearing caches is needed here, I would try just the writeSettings() and test locally.
Extra blank line before end of method not needed.
The second line should be its own paragraph (add a new line after the summary line) and the class should start with \Drupal\…
Nit: extra blank line here.
Comment #98
drupalninja99 CreditAttribution: drupalninja99 commented@Mark - at the sprint @Cottser thought I could take a pass at #97 code nitpicks since they are spelled out pretty clearly.
Comment #99
drupalninja99 CreditAttribution: drupalninja99 commented1. "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
Comment #100
markhalliwellFixed some more doc issues, reworded some verbiage and changed "trans block" references to "trans tag", which is what they are.
Comment #100.0
markhalliwellUpdated issue summary.
Comment #100.1
markhalliwellUpdated issue summary.
Comment #100.2
markhalliwellUpdated issue summary.
Comment #100.3
markhalliwellUpdated issue summary.
Comment #101
markhalliwellRenaming title to better reflect what we're doing.
Comment #102
Gábor HojtsyFix D8MI subtag.
Comment #103
star-szrNice @drupalninja99 and @Mark Carver :) Here's another pass:
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.
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.
Need a blank line between the class definition and first property/method.
This should document that it @throws the Twig_Error_Syntax exception. See https://drupal.org/node/1354#throws.
I think you can just use config() directly.
$this->drupalPost()
I realize this function is borrowed but let's add datatypes to the @params here.
Comment #104
markhalliwellFixed issues in #103.
Comment #105
markhalliwelltwig_debug
and asserts that the markup is then printed.Comment #107
markhalliwellAdded comment block for new helper function.
Comment #109
markhalliwellChanged method name, put the "For" in the wrong place.
Comment #109.0
markhalliwellUpdated issue summary.
Comment #110
markhalliwellAdding new test only patch, since the added test changed.
Comment #110.0
markhalliwellUpdated issue summary.
Comment #112
markhalliwellTest only always fails. Complete patch in #109 passed.
Comment #113
Fabianx CreditAttribution: Fabianx commentedThis 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.
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.
I love this! Nice trick with appending the DEBUG code.
I am not a huge fan of code duplication.
Lets move this _identical_ chunk (in twig strtr is used) up before the if, k?
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.
Again: This is _very_ very clever!
Props!
Besides what I found this is really almost RTBC! Great work!
Please keep it up!
Comment #114
jessebeach CreditAttribution: jessebeach commentedI love the idea of providing a simpler syntax for the common use-case of printing a variable with a little string around it.
I can't speak to the code without diving in, but the end-user approach is sound!
Comment #115
markhalliwellRefactored some code, added the 'trans' filter, updated some documentation.
Comment #116
Fabianx CreditAttribution: Fabianx commentedReviewed 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!
Comment #117
Fabianx CreditAttribution: Fabianx commentedRTBC as per #116
Comment #118
catchPatch 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.
Comment #119
fubhy CreditAttribution: fubhy commentedOH HAI TEH MUUN !!!!
Comment #120
markhalliwellAdded change notice: https://drupal.org/node/2047135
Comment #121
XanoI 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?
Comment #122
markhalliwell@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.htmlIt is an already established tag in the Twig community, we didn't create it.
Comment #123
markhalliwellShort answer, no we're not going to create a follow-up issue to change it.
Comment #124
SeanKelly CreditAttribution: SeanKelly commentedPer issue #2047227: Update templates to leverage support for the Twig {% trans %} tag extension I've started updating templates to use this improved syntax.
Comment #125
Gábor HojtsyUnfortunately 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.
Comment #126
giorgio79 CreditAttribution: giorgio79 commentedHow about letting contrib handle this like https://drupal.org/project/submitted_by
Comment #127
dlu CreditAttribution: dlu commentedMoved 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...).
Comment #129
Jeff Burnz CreditAttribution: Jeff Burnz commentedIs this currently usable in Drupal, or are there outstanding issues that I can't find right now, when I do this:
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
Comment #130
Gábor Hojtsy@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.
Comment #131
Jeff Burnz CreditAttribution: Jeff Burnz commentedOps, 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.
Comment #132
Gábor HojtsyYeah, so the author is escaped because by default placeholder items are escaped. The following would avoid escaping and let it go through:
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.
Comment #132.0
Gábor HojtsyUpdated issue summary.