Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#14910 closed (fixed)

I18N locale per app resolving - documentation incorrect and bug

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

Description

Hi,
According to
http://docs.djangoproject.com/en/1.2/howto/i18n/#using-internationalization-in-your-own-projects
and
http://docs.djangoproject.com/en/1.2/topics/i18n/deployment/#how-django-discovers-translations

At runtime, Django looks for translations by following this algorithm:

  • First, it looks for a locale directory in the application directory of the view that’s being called. If it finds a translation for the selected language, the translation will be installed.
  • Next, it looks for a locale directory in the project directory. If it finds a translation, the translation will be installed.
  • Finally, it checks the Django-provided base translation in django/conf/locale.

However, this has not been my experience. I also do not see anything
in the django source code or tests that responds to the above.
In django/utils/translation/trans_real.py I see the following:

def translation(language): 
     [......] 
        for appname in settings.INSTALLED_APPS: 
            apppath = os.path.join(get_apppath(appname), 'locale') 
            if os.path.isdir(apppath): 
                res = _merge(apppath)

_merge (and deeper down "merge") is defined as a dictionary update in
the same file.

I see two problems here:

  1. the documented behavior is not implemented
  2. the implemented behavior has the opposite order-of-preference for apps as the rest of Django. (Normally, the higher up in your INSTALLED_APPS you put an app, the more likely it's templates, etc. will be called. Here we're using a dict update, so the lowest app has preference)

Also: I think the documented behavior is not desirable. Changing
translations per-view makes for hard to debug once-in-a-while bugs.
This means the bug '2' is still a problem. I suggest either reversing
the loop

158         for appname in settings.INSTALLED_APPS: 

or making the update of a 'merge' non-destructive.

Reported before on Django-Developers, but I got no response:
http://groups.google.com/group/django-developers/browse_thread/thread/6c098e16da4c49ae

Attachments (4)

trans_real.patch (2.8 KB) - added by vanschelven 4 years ago.
docs.patch (2.0 KB) - added by vanschelven 4 years ago.
New docs (half the problem was incorrect docs)
with_test.patch (5.2 KB) - added by vanschelven 4 years ago.
Patch with test
docs.current_situation.patch (2.1 KB) - added by vanschelven 4 years ago.
Fixed docs for the current situation. Point 1. of the original posting.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by vanschelven

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

Django's default template loader is documented here:

http://docs.djangoproject.com/en/dev/ref/settings/?from=olddocs#template-loaders

('django.template.loaders.filesystem.Loader',
 'django.template.loaders.app_directories.Loader')

Keeping with that order I propose the following changes:

  • First look at explicit locale paths, then the project path, then the app paths.
  • Make updates of the locale dict non-destructive

I'll add a patch once my tests are done running.

Changed 4 years ago by vanschelven

comment:2 Changed 4 years ago by vanschelven

  • Has patch set

Having looked at the original tests the most desirable order appears to be:

  1. Project path locale
  2. Apps (in order)
  3. Locale-path
  4. Django's own translations

The attached patch is a reflection of

  1. The order mentioned above
  2. Non-destructive dictionary updates
  3. I had to move the monkey-patching code into the _merge function, because the first actually found locale is no longer known in advance.

Changed 4 years ago by vanschelven

New docs (half the problem was incorrect docs)

comment:3 Changed 4 years ago by jezdez

  • Needs tests set

Hm, I wonder if this is related to r12447.

There are definitely tests that make sure this works, so I'd appreciate it if you provide testcases for this issue.

comment:4 follow-up: Changed 4 years ago by jezdez

  • milestone set to 1.3

Changed 4 years ago by vanschelven

Patch with test

comment:5 in reply to: ↑ 4 Changed 4 years ago by vanschelven

Replying to jezdez:

It's related to r12447 in a minor way: that revision provides a nice testbed, though if you read the tests you'll see that they don't match with the documentation.

I've also added a test in the latest patch. (I'm not sure why the quickview shows 2 files as /dev/null, but I'm sure you'll figure it out).

comment:6 in reply to: ↑ description ; follow-up: Changed 4 years ago by ramiro

  • Component changed from Uncategorized to Internationalization
  • Triage Stage changed from Unreviewed to Accepted

Replying to vanschelven:

Hi,
According to
http://docs.djangoproject.com/en/1.2/howto/i18n/#using-internationalization-in-your-own-projects
and
http://docs.djangoproject.com/en/1.2/topics/i18n/deployment/#how-django-discovers-translations

At runtime, Django looks for translations by following this algorithm:

  • First, it looks for a locale directory in the application directory of the view that’s being called. If it finds a translation for the selected language, the translation will be installed.
  • Next, it looks for a locale directory in the project directory. If it finds a translation, the translation will be installed.
  • Finally, it checks the Django-provided base translation in django/conf/locale.

However, this has not been my experience. I also do not see anything
in the django source code or tests that responds to the above.

From what I've found doing some code archeology, this was the original behavior, note the First, it looks for a locale directory in the application directory of the view that’s being called. part. Note there is no mention of iterating over apps that appear in INSTALLED_APPS.

A key part of the scheme (IMHO missed by e.g. #11384) was this paragraph located a few paragraphs below: Application message files are a bit complicated to discover – they need the LocaleMiddleware. If you don’t use the middleware, only the Django message files and project message files will be installed and available at runtime.

It was LocaleMiddleware's task to activate translations from the app containing the current view: http://code.djangoproject.com/browser/django/branches/i18n/django/middleware/locale.py?rev=963#L9

But this was changed in r964, documentation wasn't updated accordingly and have remained untouched and unchallenged for five years. So much e.g. for my i18n docs refactoring :(

I haven thought about the OP proposals yet, just wanted to get these notes written down. IMHO this ticket should deal with what OP call item 1. Item 2 should be reported and dealt in another ticket, one issue per ticket/commit.

comment:7 in reply to: ↑ 6 Changed 4 years ago by vanschelven

Replying to ramiro:

I haven thought about the OP proposals yet, just wanted to get these notes written down. IMHO this ticket should deal with what OP call item 1. Item 2 should be reported and dealt in another ticket, one issue per ticket/commit.

In that case I would suggest to adapt the documentation to reflect the reality of the code:

  1. This won't break any existing code
  2. It makes more sense to have static translations than dynamic ones. Behavior will be more reliable, debugging and testing will be easier.

Changed 4 years ago by vanschelven

Fixed docs for the current situation. Point 1. of the original posting.

comment:8 Changed 4 years ago by vanschelven

This ticket is about "point 1" above from now on.

Point 2 is moved out here: #14924

comment:9 Changed 4 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 4 years ago by jezdez

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

(In [14998]) Fixed #14910 -- Clarified the order of precedence of loading translation catalogues. Thanks, vanschelven.

comment:11 Changed 4 years ago by jezdez

(In [15002]) [1.2.X] Fixed #14910 -- Clarified the order of precedence of loading translation catalogues. Thanks, vanschelven.

Backport from trunk (r14998).

comment:12 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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