ruby-i18n / i18n

Internationalization (i18n) library for Ruby
MIT License
976 stars 408 forks source link

[BUG] Unexpected and inconsistent handling of escaped interpolation arguments #689

Closed Bilka2 closed 4 months ago

Bilka2 commented 5 months ago

What I tried to do

Escaped interpolation arguments in translations with %, e.g. This is %%{escaped} and not interpolated..

Also, tried to escape that escaping, e.g. This is %%%{not_escaped} and interpolated..

What I expected to happen

The first example This is %%{escaped} and not interpolated. should result in This is %{escaped} and not interpolated..

The second example This is %%%{not_escaped} and interpolated. given the substitution :not_escaped => 'foo' should result in This is %foo and interpolated.. If no substitution is given, it should result in a MissingInterpolationArgument error.

The above is based on the behaviour of sprintf:

irb(main):004> sprintf("%%{test}")
=> "%{test}"
irb(main):005> sprintf("%%%{test}")
(irb):5:in `sprintf': one hash required (ArgumentError)

sprintf("%%%{test}")
        ^^^^^^^^^^^
        from (irb):5:in `<main>'
        from <internal:kernel>:187:in `loop'
        from /usr/local/bundle/gems/irb-1.12.0/exe/irb:9:in `<top (required)>'
        from /usr/local/bundle/bin/irb:25:in `load'
        from /usr/local/bundle/bin/irb:25:in `<main>'
irb(main):006> sprintf("%%%{test}", test: "foo")
=> "%foo"

What actually happened

Various methods handle this differently, depending on whether any substitutions were provided and whether the InterpolationCompiler is used. Mostly the escape characters do not get stripped when no substitutions are provided. See tests linked below.

Versions of i18n, rails, and anything else you think is necessary

i18n 1.14.4, no Rails

Tests for the issue

https://github.com/ruby-i18n/i18n/compare/master...Bilka2:i18n:escaped-interpolations-test

I commented out the lines that were failing. As you can see, the actual I18n.interpolate implementation handles everything correctly, but the various paths that call it do not. The cause for this is simple: When no substitutions are provided the string is never interpolated. See https://github.com/ruby-i18n/i18n/pull/688#discussion_r1521561272.

The InterpolationCompiler is only incorrect for the case with %%%. Note that one of the previous asserts in the test was incorrect.

Bilka2 commented 5 months ago

I started looking into the MissingInterpolationArgument error and found https://github.com/ruby-i18n/i18n/blob/master/lib/i18n/tests/interpolation.rb#L6-L18, apologies for not seeing this sooner.

According to that code comment, i18n should not alter the string if no substitutions are provided, including not raising the MissingInterpolationArgument error. This invalidates all the test cases I linked above, with the exception of the incorrect handling of %%%{a} (with substitution provided) by the InterpolationCompiler backend. This means the behaviour I reported is in fact expected.

However, based on the code comment, the ReservedInterpolationKey error that led me to discover this issue should not be raised either - it was added specifically for the "no substitutions provided" case, which should not be raising interpolation errors.

I see a few ways forward that allow the usecase of i18n-tasks, which is to have a translation t("bar") that results in output such as Foo %{scope}:

radar commented 5 months ago

I think C is the ticket here. It won't break upstream Rails, and it will fix the underlying issue.

radar commented 4 months ago

Fixed by #688.