Django

Code

Ticket #7163 (closed: fixed)

Opened 7 months ago

Last modified 2 weeks ago

Region locales' translation objects get overwritten

Reported by: oggy Assigned to: mtredinnick
Milestone: Component: Internationalization
Version: SVN Keywords: translation gettext region locale
Cc: ognjen.maric@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description

Internal caching performed by Python's gettext module causes translation objects for different region locales of the same language (e.g. en-us and en-gb) to get overwritten.

Consider the scenario of having en-us and en-gb localized versions of a project or an app. When the translation for one of these locales is first loaded, Django loads the translation objects by processing .mo files in this order:

1. From root django conf/locale dir
2. From project locale dir
3. From settings.LOCALE_PATHS
4. From apps' locale dirs

Due to the way that Python's ugettext module works, since no en_US or en_GB subdirs are found in step 1, the base language 'en' translation in conf/locale/en will be used. Hence, both of these translations will refer to the same .mo file. However, this will cause ugettext to cache, returning two translation objects which share the same _catalog dictionary.

In [15]: t1 = gettext.translation('django', '/usr/lib/python2.5/site-packages/django/conf/locale/', ['en_US'])

In [16]: t2 = gettext.translation('django', '/usr/lib/python2.5/site-packages/django/conf/locale/', ['en_GB'])

In [17]: id(t1._catalog)
Out[17]: 3079088844L

In [18]: id(t2._catalog)
Out[18]: 3079088844L

Therefore, any changes to one translation at stages 2-4 will effectively change the other one as well, rendering regional translations useless at the project/LOCALE_DIRS/app level. To prevent this, it's necessary to perform a deep copy of the translation object at stage 1. Since Django implements its own caching of translation objects, this should not affect the performance too much.

Attachments

trans_real_v7513.diff (0.6 kB) - added by oggy on 05/06/08 08:16:31.
trans_real_v7513v2.diff (0.7 kB) - added by oggy on 05/14/08 10:08:33.
trans_real_v7513v3.diff (0.8 kB) - added by oggy on 06/15/08 18:22:41.

Change History

05/06/08 08:16:31 changed by oggy

  • attachment trans_real_v7513.diff added.

05/14/08 10:07:57 changed by oggy

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

Oops, didn't realize that the "partition" string method was only introduced in Python 2.5. This one's not quite as elegant, but it works for elderly snakes as well...

05/14/08 10:08:33 changed by oggy

  • attachment trans_real_v7513v2.diff added.

06/14/08 07:04:59 changed by Simon Greenhill

  • stage changed from Unreviewed to Ready for checkin.

06/15/08 18:22:41 changed by oggy

  • attachment trans_real_v7513v3.diff added.

06/15/08 18:25:11 changed by oggy

Argh. My patch broke Django when run under 2.4, due to the differences in deepcopy between 2.4 and 2.5. This one has been tested to work under 2.4, but I don't have any installs of 2.3 around, could somebody please verify before commiting the patch?

07/06/08 00:17:26 changed by mtredinnick

  • owner changed from nobody to mtredinnick.
  • needs_better_patch set to 1.
  • stage changed from Ready for checkin to Accepted.

This is some very nice debugging and analysis. However, I think you might be going too far in the patch. It looks like only a copy() is needed, not a deepcopy(). The reason being that both _catalog and _info are dictionaries where keys are updated or accessed, but we (Django or Python) are never doing in-place updates of the values in the dictionaries -- only atomic replacements.

When I replace your deepcopy() calls with plain dictionary copies, it seems to work for me (I saw the problem without any patches and it went away with patches). You clearly have a failing case as well. Can you make sure it looks fixed for you, too. Just replace your deepcopy() calls with

res._catalog = res._catalog.copy()
res._info = res._info.copy()

I'm pretty sure that's a fix, but I'd like verification. Also, how do you want to be credited in the AUTHORS file?

(Don't worry about submitting a new patch or anything; I'll see when you reply and can modify things in my local branch appropriately.)

07/06/08 17:03:26 changed by oggy

You're right of course, deepcopy() doesn't make sense since those dicts contain only unicode (i.e. immutable) values. It was a leftover from my previous attempt - figured it'd be nicer to deep copy the entire object instead of messing with other people's _-ed variables, but sometimes a man's gotta do what a man's gotta do ;)

Anyway I tested your patch and it works fine. As for the AUTHORS, could you make it to "oggy <ognjen.maric@gmail.com>"? Thanks

07/06/08 21:10:55 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

Fixed in [7857].

11/06/08 06:56:17 changed by sunrise2 <likenikeshoes01@gmail.com>

I'm closing it according to the last comment. jordan nike shoes

11/14/08 08:50:58 changed by spoof

what if

 res = _translation(globalpath) 

returns None? The following code:

base_lang = lambda x: x.split('-', 1)[0] 
if base_lang(lang) in [base_lang(trans) for trans in _translations]: 
  res._info = res._info.copy() 
  res._catalog = res._catalog.copy() 

will fail with error:

AttributeError: 'NoneType' object has no attribute '_info'

is it right?

11/19/08 04:49:16 changed by anonymous

Yes it's right but it should not return None unless there's no translation of the Django core for that particular language.


Add/Change #7163 (Region locales' translation objects get overwritten)




Change Properties
Action