Twig templates incorrectly use % trans % with arbitrary filters

Adding a potentially related issue that may help.

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

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

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

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

There are only a few more of this in core, so we can fix all at once.

More 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 :)

+++ b/core/modules/image/templates/image-crop-summary.html.twig
@@ -21,12 +21,14 @@
+    {% set width = data.width|e %}
...
+    {% set height = data.height|e %}

+++ b/core/modules/image/templates/image-resize-summary.html.twig
@@ -19,12 +19,14 @@
+    {% set width = data.width|e %}
...
+    {% set height = data.height|e %}

+++ b/core/modules/image/templates/image-scale-summary.html.twig
@@ -20,12 +20,14 @@
+    {% set width = data.width|e %}
...
+    {% set height = data.height|e %}

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

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

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

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

Rolled 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 :)

Missed this, are we still dealing with this problem? Moving to -dev

Yes we need to. The thing is those filters don't to anything :)

This may be a duplicate? #2592207: {% trans %} breaks format_date twig filter

Also thanks for the update @Gábor Hojtsy, was just triaging issues on the wrong version/branch.

Closed #2592207: {% trans %} breaks format_date twig filter as duplicate.

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

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

Huh, 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?

I'm fine with removing the e and doing manual testing, committers may not be, but we can always try.

+++ b/core/modules/image/templates/image-crop-summary.html.twig
@@ -21,12 +21,14 @@
-      width {{ data.width|e }}
+      width {{ width }}

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

Yay, looks like a go-ahead then :)

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

@xjm: re documenting it, not sure where would that be possible. Where do we document {trans} in the first place?

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

And thanks, by the way :) review of the latest patch:

+++ b/core/modules/image/templates/image-crop-summary.html.twig
@@ -18,15 +18,17 @@
+    {% set width = data.width %}
...
-      width {{ data.width|e }}
+      width {{ width }}
...
+    {% set height = data.height %}
...
-      height {{ data.height|e }}
+      height {{ height }}

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:

• image-crop-summary.html.twig
• image-resize-summary.html.twig
• image-scale-summary.html.twig

+++ b/core/modules/image/templates/image-rotate-summary.html.twig
@@ -19,8 +19,9 @@
+  {% set degrees = data.degrees|abs %}

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.

Discussed this with @alexpott @xjm @catch @effulgentsia and we agreed that this is major, as long as:

1. The translatable strings are still being picked up without these changes
2. The redundant |e is not causing things to output as unescaped

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

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

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

@Gábor Hojtsy thanks! I would normally say yes we could try the {% set data.degrees … %} but I'm fairly sure you can't set 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.

Yay, cool, who is gonna RTBC this? :)

(As far as I know translation.module is no longer a thing, so trying a better fit given the fix. Carry on.)

This is correct, thanks Gábor Hojtsy.

Committed and pushed a0273f0 to 8.3.x and 4b7f7de to 8.2.x. Thanks!

Yay, thanks!