[research] Bad localization strings shouldn't break the site.

RESOLVED WORKSFORME

Status

support.mozilla.org
Localization
P3
normal
RESOLVED WORKSFORME
6 years ago
5 years ago

People

(Reporter: mythmon, Unassigned)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: u=user c=general p=3 s=2013.backlog)

(Reporter)

Description

6 years ago
Right now, if a translator puts a string in Verbatim that messes with a format string, any page that uses that string raises an unhelpful 500. An example of a instance in which this has happened is bug 841392, in which a localizer submitted the translation "{0} antwoorden" for the string "{num} answers". This seems like an easy mistake to make, and we shouldn't punish localizers or users for this.

This might require patching Tower, or writing our own wrappers around gettext and company, or writing a custom string formatter. Since I think this generally happens *after* the call to gettext, we might just have to deal with it in string formatting instead of in the look up phase.

Options

* If a string format fails, just ignore that variable. For example, 'There are {num} {animals}.'.format(num=5) would return 'There are 5 {animals}' without raising an error (though we would probably log a warning or something.

* If a string format fails, go back and get the English version of the text. So in the Polish example in the first paragraph, would render as '11 replies' because '{0} antwoorden' did not properly format. Considering the order of calls to format and gettext, I'm not sure this is possible.

* Some third option?
Whatever solution we pick has to have the following properties:

1. the site must continue to be usable
2. we must be alerted as soon as possible so that we can fix the string

I think anything short of that just creates new problems.


When I fixed this in Miro, I did two things:

1. create gettext wrappers that, like the Python logging module functions, also took the values to be interpolated as arguments. The wrappers would take the msgid, convert it, and then interpolate the variables. If any of those pieces failed, it'd return the msgid interpolated.

https://github.com/pculture/miro/blob/master/tv/lib/gtcache.py#L180

2. change the entire codebase to use our gettext wrappers

It wasn't a trivial project. I had the advantage that Miro was using the Python gettext stuff directly and wasn't going through several layers of middlemen.


I think it's a lot more difficult here since we have a bunch of different ways that values get interpolated.

We could modify the f and fe filters in Jingo (that's where it was erroring out in the problem today) such that if they throw an exception, we just return the string *without* the bits interpolated.

Speaking to Mike's options, I can't think of how we could do the first one var-by-var--it'd have to be all or nothing. It's essentially what I'm suggesting by modifying the f and fe filters.

I have no idea how to do the second one without seriously modifying the plumbing and changing the entire site. That'd be a huge project. So huge that I don't think it'd be worth it.
(Reporter)

Comment 2

6 years ago
Doing formatting var-by-var: https://gist.github.com/mythmon/a24456ba39eed0fae26f.

Python uses string.Format by default, but we can make a custom subclass that defines whatever get_value() behavior we want, like not throwing errors. I did this in another project, but couldn't find it again, so I wrote up a quick sketch. If we are already modifying |f and |fe, this wouldn't be too hard to do. Maybe we could even monkeypatch... no, lets not do that.
Let's invest some time to figure out what would be needed to fix the root cause of the breakage. I'm assuming that this would also benefit other apps that use the .po files like MDN and AMO.
Summary: Bad localization strings shouldn't break the site. → [research] Bad localization strings shouldn't break the site.
Whiteboard: u=user c=general p= s=2013.5
Just to be clear, we can't fix the root cause since the root cause is bad l10n strings.

Further, strings get translated in the code through a series of different paths. The problem step is when the string is interpolated--not when it's translated. That's a step that happens in a bunch of different ways.

Like I said in comment #1, I don't think it's possible to alleviate it across the board without turning this into a *huge* project, but we can probably alleviate the problem in specific code paths.

This is going to need a few days of research. Research should answer the following questions:

1. What are the different codepath variations for a string to be translated and then interpolated?

2. Can we reduce the number of codepath variations by changing the code so that there are fewer variations?

3. At which points can we tweak something such that interpolation doesn't kick up Python string formatting errors?
(In reply to Will Kahn-Greene [:willkg] from comment #4)
> This is going to need a few days of research.
2pts or 3?

Let's do this because these issues are getting annoying.
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Whiteboard: u=user c=general p= s=2013.5 → u=user c=general p=2 s=2013.5
Target Milestone: --- → 2013Q1
I suspect it involves dives into tower, jinja, jingo, and the kitsune code to get a clear picture and answer the questions I asked above in comment #4.

My vote is to give it 3 points because that's far more likely enough time to wade through everything, get a clear view of the problem domain, and have a series of possible things we could implement.
3pts it is!! ^^
Whiteboard: u=user c=general p=2 s=2013.5 → u=user c=general p=3 s=2013.5
I guess this isn't really P1 since the site isn't really broken with it. It would just alleviate unexpected breakage at 9pm on a thursday night.
Priority: P1 → P3
Moving this to the backlog to ponder it more. There may be other solutions might adding the linter to our deploy process.
Whiteboard: u=user c=general p=3 s=2013.5 → u=user c=general p=3 s=2013.backlog
See bug 850215 for the strings breaking emails instead of the site
Q1 is in the past. Moving to the future.
Target Milestone: 2013Q1 → Future
Pretty sure Dennis took care of this. \o/ Thanks, Will!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.