Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Bilka2 opened this issue Mar 12, 2024 · 3 comments
Closed

Comments

@Bilka2
Copy link
Contributor

Bilka2 commented Mar 12, 2024

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

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 #688 (comment).

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

@Bilka2
Copy link
Contributor Author

Bilka2 commented Mar 28, 2024

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

  • A) Raise when translated entry contains interpolations for reserved keywords and no substitutions provided #678 is reverted to keep in line with the code comment and the behaviour of the MissingInterpolationArgument error.
  • B) The changes from #678 are adjusted to only potentially raise ReservedInterpolationKey when only reserved keywords are provided, so when options is a subset of RESERVED_KEYS.
    This allows t("bar") but raises in cases like t("bar", scope: "baz"). This still solves the original issue raised with Rails but allows the i18n-tasks usecase.
  • C) The error is kept as is, Fix that escaped interpolations with reserved keywords raised ReservedInterpolationKey #688 is merged, nothing else is changed.
    This allows to work around this issue by escaping the interpolation and adding a dummy substitution to the translation to force the escape character to be removed. Escaped interpolations will still not result in the desired string if no substitution is provided.
  • D) Strings with no substitutions are changed to: Escape interpolations and raise MissingInterpolationArgument.
    This allows to use escaped interpolations of reserved keywords without worrying about providing other substitutions. Raising more errors may break existing applications in the same way that the extra ReservedInterpolationKey broke i18n-tasks. This completely disagrees with the linked code comment.

@radar
Copy link
Collaborator

radar commented Mar 29, 2024

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

@radar
Copy link
Collaborator

radar commented May 6, 2024

Fixed by #688.

@radar radar closed this as completed May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants