Problem/Motivation
Several templates in Drupal core use filters within { % trans % }. However as per TwigNodeTrans::compileString(), there is no support whatsoever for arbitrary filters in twig %trans% blocks. There is only support for the |placeholder "filter". Other filters will just be ignored and not work. We also figured this out in #2547639: Rename ambiguous "Menu" toggle in Bartik for accessibility where we attempted to use the |lower filter in a %trans%.
Proposed resolution
Fix the core templates, so the filters the templates intended to be use will actually run. In turn change the % trans % usage to only use the resulting value and no inline filters.
Remaining tasks
Review. Commit.
User interface changes
None.
API changes
None. Some translatable strings will change, but the ones there would not have worked either.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff.txt | 4.2 KB | Gábor Hojtsy |
#33 | 2610436-33.patch | 5.56 KB | Gábor Hojtsy |
#27 | 2610436-27.patch | 5.92 KB | Gábor Hojtsy |
#27 | interdiff.txt | 5.28 KB | Gábor Hojtsy |
#15 | Translate Drupal core to Ukrainian | Translations 2015-11-09 20-52-29.png | 44.18 KB | Gábor Hojtsy |
Comments
Comment #2
pp CreditAttribution: pp commentedComment #3
star-szrAdding a potentially related issue that may help.
Comment #4
Gábor HojtsyThis also hits a string placeholder matching problem... The actual Drupal 8 string is https://localize.drupal.org/translate/languages/hu/translate?sid=2136773, not the one @pp linked. So localize.drupal.org extracts
"random between -° and °"
(ie. it redoes not recognize the placeholders at all). What does Drupal itself do with this string? Ie. when this template is used, what ends up in the interface translation module's UI?Comment #5
Gábor HojtsyOpened #2611584: Twig trans placeholders with extra filters result in broken strings for localize to figure out how should we proceed with the parsing there. I *think* Drupal itself would detect the above string as
random between -@data.degrees° and @data.degrees°
, but it would be good to verify.Comment #6
herom CreditAttribution: herom as a volunteer commentedEnabled the twig i18n debug output manually, and got this:
<!-- TRANSLATION: "random between -@data.degrees° and @data.degrees°" -->
It's the same as @Gabor said. The fix should come in #2611584: Twig trans placeholders with extra filters result in broken strings.
Comment #7
Gábor HojtsyResolved #2611584: Twig trans placeholders with extra filters result in broken strings. However, turns out its not the dots at all but the |abs filter. If you look at TwigNodeTrans::compileString() there is no support whatsoever for extra filters like |abs. It does not compile that over somehow to the array arguments, it just ignores them. So potx on the server side had broken code to extract this because this is not valid. Now made it more resilient so it will at least record the placeholder. However, the abs filter will be entirely ignored by core, so its pointless to try to use it here :)
This would result in the translatable string
random between -@degrees° and @degrees°
which is as intended.Comment #8
Gábor HojtsyThere are only a few more of this in core, so we can fix all at once.
Comment #9
Gábor HojtsyMore of those are found in image module. I did not find any more elsewhere. There is one in the patch in progress in #2547639: Rename ambiguous "Menu" toggle in Bartik for accessibility, but that is not yet in :)
Comment #10
Gábor HojtsyComment #11
star-szrFor these I wonder if we can remove the |e, these might have been from before we had autoescape?
Would be nice if trans blocks didn't have these limitations, I wonder if upstream it has the same limitations or not.
Comment #12
Gábor HojtsyIf someone is interested in futzing around to have some filter parsing in TwigNodeTrans::compileString() as well as fix potx's parsing then it would be possible to make that support available yes. :) I don't have that kind of experience unfortunately, neither the time before the release :/
Comment #13
star-szrI think fixing the translation bug is more important and adding the support for more complex things inside
trans
could probably happen in separate issue(s).Is there any way we can test this?
Comment #14
Gábor Hojtsy@Cottser: I think we can spot-test with some filters that they don't work in trans, but not sure that is what you are looking for? We can test that things like abs did not work before this patch and now they do. If you set a value where the value is different if the abs filter did not run. For the |e if there is already escaping, then a test should work either way. Not sure if |e would do double escaping since we already have autoescaping, but if it does, then we could test that too.
Comment #15
Gábor HojtsyRolled out #2611584: Twig trans placeholders with extra filters result in broken strings to l.d.o, so at least l.d.o parses the source strings more or less the same way as core. Looking at the Ukrainian team where they had 100% ready translations, these are the only "new" strings:
So only the templates we are dealing with here apparently :)
Comment #16
joelpittetMissed this, are we still dealing with this problem? Moving to -dev
Comment #17
Gábor HojtsyYes we need to. The thing is those filters don't to anything :)
Comment #18
joelpittetThis may be a duplicate? #2592207: {% trans %} breaks format_date twig filter
Comment #19
joelpittetAlso thanks for the update @Gábor Hojtsy, was just triaging issues on the wrong version/branch.
Comment #20
Gábor HojtsyClosed #2592207: {% trans %} breaks format_date twig filter as duplicate.
Comment #21
Gábor Hojtsy@joelpittet, @Cottser: what do you think needs to be done here? Also removing from D8MI sprint due to inactivity and adding security tag due the escape filters that don't run.
Comment #22
star-szr@Gábor Hojtsy thanks for the bump, I would still like to see if we can remove the |e filters from the patch otherwise this seems good to go. It seems like we might need manual testing to confirm, what do you think?
Comment #23
xjmHuh, interesting. I would never have guessed that this would break the translatabiity or the filter. I think we would at least want to add some documentation somewhere, and maybe add some validation that throws an exception or something when the filter is used in a template so that developers do not accidentally introduce this?
Comment #24
Gábor HojtsyI'm fine with removing the e and doing manual testing, committers may not be, but we can always try.
Comment #25
alexpottDo we even need this - I would hope that auto-escape would just do this for us? Also
|e
won't result in a double escape.Comment #26
Gábor HojtsyYay, looks like a go-ahead then :)
Comment #27
Gábor HojtsyUpdated patch with removing |e. The same problem applies to the stable theme's templates. Are we supposed to keep it "stable" to the extent of leaving bugs there or what is the policy for that @Cottser?
Also its not a security concern, so removing that tag.
Comment #28
Gábor Hojtsy@xjm: re documenting it, not sure where would that be possible. Where do we document {trans} in the first place?
Comment #29
star-szr@Gábor Hojtsy I believe this is completely safe to update in Stable because it doesn't change markup and also it's definitely a bug fix, not just changing things because we can. So +1 to updating the Stable templates here.
Comment #30
star-szrAnd thanks, by the way :) review of the latest patch:
Now that these don't have filters and we're just relying on autoescape, we can use data.width and data.height directly I think (no need for the set tags).
In these templates:
In other words I think the degrees variable in image-rotate-summary.html.twig (and the one in the same template in Stable) are the only ones that need a set tag.
Comment #31
star-szrDiscussed this with @alexpott @xjm @catch @effulgentsia and we agreed that this is major, as long as:
At least in theory the |e filters are just redundant, and removing those seems almost like cleanup. The |abs filter being used is not working so that's a bug when image-rotate-summary.html.twig is in use.
Also it seems like this won't be a translatable string change because the filters are stripped from these tokens by https://www.drupal.org/project/potx, @alexpott pointed out there is a test for extra_filter which uses |abs and the resulting token doesn't have the filter.
http://cgit.drupalcode.org/potx/tree/tests/potx_test_8.html.twig?id=fc68...
http://cgit.drupalcode.org/potx/tree/tests/potx.test?id=fc68c922e2b958f6...
Comment #32
Gábor HojtsyYeah so the variable renaming was there to make the strings nicer for translators. It is certainly a string change then though. Without that this could even get into 8.1 and no need to wait for 8.2.
Comment #33
Gábor HojtsyRemoved those sets. Should we
{% set data.degrees = data.degrees|abs %}
as well, so we reuse the same variable? That would truly not change any string then. Otherwise, we are changing that one string to fix a major bug (which I think is still allowed?).Comment #34
star-szr@Gábor Hojtsy thanks! I would normally say yes we could try the
{% set data.degrees … %}
but I'm fairly sure you can'tset
sub-items like that, I think it's a syntax error. I haven't dug into this deeply but it might be because when you drill down (in this case.degrees
) that could be calling a method or something else, in other words we don't know whether it's something that can be manipulated directly.So I think this is very likely RTBC now.
Comment #35
Gábor HojtsyYay, cool, who is gonna RTBC this? :)
Comment #36
xjm(As far as I know translation.module is no longer a thing, so trying a better fit given the fix. Carry on.)
Comment #38
joelpittetThis is correct, thanks Gábor Hojtsy.
Comment #39
alexpottCommitted and pushed a0273f0 to 8.3.x and 4b7f7de to 8.2.x. Thanks!
Comment #41
Gábor HojtsyYay, thanks!