Code

Opened 3 years ago

Closed 3 years ago

#16516 closed Cleanup/optimization (fixed)

Blocktrans should tolerate bad locale data

Reported by: simon29 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

Description

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 claudep 3 years ago.
Fix KeyError, with tests
16516-2.diff (5.4 KB) - added by claudep 3 years ago.
Same patch without hardcoding 'en' fallback

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by aaugustin

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by claudep

Fix KeyError, with tests

comment:2 Changed 3 years ago by claudep

  • Has patch set

comment:3 Changed 3 years ago by aaugustin

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 3 years ago by claudep

  • 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 3 years ago by claudep

Same patch without hardcoding 'en' fallback

comment:5 Changed 3 years ago by claudep

  • Patch needs improvement unset

comment:6 Changed 3 years ago by aaugustin

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/django.mo 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 3 years ago by claudep

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 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready 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 3 years ago by jezdez

Looks good.

comment:10 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.