Code

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#7163 closed (fixed)

Region locales' translation objects get overwritten

Reported by: oggy Owned by: mtredinnick
Component: Internationalization Version: master
Severity: Keywords: translation gettext region locale
Cc: ognjen.maric@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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 (3)

trans_real_v7513.diff (590 bytes) - added by oggy 6 years ago.
trans_real_v7513v2.diff (711 bytes) - added by oggy 6 years ago.
trans_real_v7513v3.diff (774 bytes) - added by oggy 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by oggy

comment:1 Changed 6 years ago by oggy

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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...

Changed 6 years ago by oggy

comment:2 Changed 6 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Ready for checkin

Changed 6 years ago by oggy

comment:3 Changed 6 years ago 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?

comment:4 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Patch needs improvement set
  • Triage 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.)

comment:5 Changed 6 years ago 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@…>"? Thanks

comment:6 Changed 6 years ago by mtredinnick

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

Fixed in [7857].

comment:8 Changed 5 years ago 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?

comment:9 Changed 5 years ago 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 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.