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.

Comments

pp created an issue. See original summary.

pp’s picture

Component: transliteration system » translation.module
Cottser’s picture

Adding a potentially related issue that may help.

Gábor Hojtsy’s picture

Status: Active » Postponed (maintainer needs more info)

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?

Gábor Hojtsy’s picture

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.

herom’s picture

Status: Postponed (maintainer needs more info) » Closed (works as designed)

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.

Gábor Hojtsy’s picture

Title: image-rotate-summary.html.twig untranslatable » image-rotate-summary.html.twig uses an extra filter in trans
Status: Closed (works as designed) » Needs review
Issue tags: +D8MI, +language-ui, +sprint
FileSize
587 bytes

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.

Gábor Hojtsy’s picture

Title: image-rotate-summary.html.twig uses an extra filter in trans » Twig templates incorrectly use % trans % with arbitrary filters
Priority: Normal » Major

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue summary: View changes
Cottser’s picture

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

Gábor Hojtsy’s picture

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

Cottser’s picture

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?

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

joelpittet’s picture

Version: 8.0.0-rc3 » 8.0.x-dev

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

Gábor Hojtsy’s picture

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

joelpittet’s picture

joelpittet’s picture

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

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint +security

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

Cottser’s picture

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

xjm’s picture

Status: Needs review » Needs work

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?

Gábor Hojtsy’s picture

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

alexpott’s picture

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

Gábor Hojtsy’s picture

Yay, looks like a go-ahead then :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -security +sprint
FileSize
5.28 KB
5.92 KB

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.

Gábor Hojtsy’s picture

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

Cottser’s picture

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

Cottser’s picture

Status: Needs review » Needs work

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.

Cottser’s picture

Issue tags: +Triaged D8 major

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

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.1.x-dev

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
4.2 KB

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

Cottser’s picture

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

Gábor Hojtsy’s picture

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

xjm’s picture

Component: translation.module » theme system

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This is correct, thanks Gábor Hojtsy.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed a0273f0 on 8.3.x
    Issue #2610436 by Gábor Hojtsy, Cottser, joelpittet, pp, xjm, alexpott:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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