Opened 6 years ago

Closed 5 years ago

#16516 closed Cleanup/optimization (fixed)

Blocktrans should tolerate bad locale data

Reported by: Simon Litchfield Owned by: nobody
Component: Internationalization Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Currently, blocktrans crashes entire pages when it tries to merge strings that aren't properly formatted in the locale data files.

For example, if %(person)s is the original, the spanish translated version might erroneously include %(persona)s. If that's being blocktrans'd in your base template, it'll crash your whole spanish site.

Translators work separately to template writers, and are generally less technical. Mistakes happen. They are minor and shouldn't bring sites down. Especially with tools like django-rosetta in their hands.

IMHO, the default behaviour here should be to ignore (or perhaps throw a warning) on improperly formatted translation strings.

Attachments (2)

16516.diff (3.8 KB) - added by Claude Paroz 6 years ago.
Fix KeyError, with tests
16516-2.diff (5.4 KB) - added by Claude Paroz 5 years ago.
Same patch without hardcoding 'en' fallback

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by Aymeric Augustin

Easy pickings: unset
Triage Stage: UnreviewedAccepted

Django goes to great lengths to ensure template authors can't break a page, because they don't have the same skill set as developers.

This argument applies even more to translators: HTML authors know what code is, translators using POEdit don't.

For this reason, I support the suggestion.

I'm not sure it's an easy picking — please write a patch and prove me wrong :)

Changed 6 years ago by Claude Paroz

Attachment: 16516.diff added

Fix KeyError, with tests

comment:2 Changed 6 years ago by Claude Paroz

Has patch: set

comment:3 Changed 5 years ago by Aymeric Augustin

I think the proper solution is to output settings.TEMPLATE_STRING_IF_INVALID when the translation string is invalid.

I'm against hardcoding a fallback to 'en':

  • if someone builds a website in Greek and the English translation is wrong, this doesn't fix the error;
  • if someone builds a website in Turkish and the German translation is wrong, I don't know what happens, but if it works it's a hack :)

Currently, the 'en' locale is special-cased in only one place: django.views.i18n.javascript_catalog. This has bugged me for a long time, the implementation is incredibly convoluted, and I still can't figure out why it's necessary. Apparently, you were involved in #14924, so you probably know this piece of code better than I do :)

comment:4 Changed 5 years ago by Claude Paroz

Patch needs improvement: set

I understand your concern about using the 'en' translation. But I'm opposed to use the TEMPLATE_STRING_IF_INVALID setting. If a translation has an error, the logical fallback is to use the original untranslated string, not the empty string (which is default for TEMPLATE_STRING_IF_INVALID).

So I will try to improve the patch by replacing the 'en' hardcoded value by a NullTranslations instance (which corresponds to the original string, whatever the original language is).

Changed 5 years ago by Claude Paroz

Attachment: 16516-2.diff added

Same patch without hardcoding 'en' fallback

comment:5 Changed 5 years ago by Claude Paroz

Patch needs improvement: unset

comment:6 Changed 5 years ago by Aymeric Augustin

To apply the patch, I first removed the chunk about the binary .mo file, and then I ran: msgfmt -o tests/regressiontests/i18n/other/locale/fr/LC_MESSAGES/ tests/regressiontests/i18n/other/locale/fr/LC_MESSAGES/django.po

The fix and the tests for the original issue are OK. You're right, TEMPLATE_STRING_IF_INVALID isn't a good idea here.

Regarding the changes to translation.override, I don't understand why you added the deactivate; it isn't used anywhere by this patch; is it a fix for a separate issue?

comment:7 Changed 5 years ago by Claude Paroz

I added the deactivate_all call to allow for translation.override to set no translation at all. While I was completing the docs, I also realized that the *existing* deactivate parameter was undocumented. This last bit could have been part of a separate issue.

comment:8 Changed 5 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

Oh right, you modified the signature in the docs, not in the code. How did I manage to mis-read that? :/

comment:9 Changed 5 years ago by Jannis Leidel

Looks good.

comment:10 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16723]:

Fixed #16516 -- Relaxed the blocktrans rendering a little by falling back to the default language if resolving one of the arguments fails, raising a KeyError. Thanks, Claude Paroz and Aymeric Augustin.

Note: See TracTickets for help on using tickets.
Back to Top