Code

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4292 closed (fixed)

[unicode] django.utils.functional.lazy can't decorate functions with unicode results

Reported by: Ivan Sagalaev <Maniac@…> Owned by: mtredinnick
Component: Translations Version: other branch
Severity: Keywords: unicode
Cc: Maniac@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently the admin page showing, say, a user object is broken for Russian translation. I've traced the reason -- a leaky abstraction of a lazy proxy object.

The proxy object created by 'lazy' should pretend to be an object of type returned by a decorated function. And this should work:

ugettext = lazy(ugettext, unicode)
s = ugettext(...) # a proxy object pretending to be a unicode instance
s = unicode(s)

But it doesn't work because unicode(s) checks:

  1. if s is an instance of unicode (it isn't)
  2. if s has __unicode__ (it doesn't)
  3. if s has __str___, which it does (! unicode object itself has it)

Then it calls __str__ on unicode object and it breaks for non-ascii characters.

I've created a fix but it seems a bit fragile because it creates a special case for unicode objects in lazy proxies.

P.S. The patch also contains two str -> smart_unicode conversions that don't strictly belong here but they are required for the testcase to work.

Attachments (1)

4292.diff (2.5 KB) - added by Ivan Sagalaev <Maniac@…> 7 years ago.
Patch

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by Ivan Sagalaev <Maniac@…>

Patch

comment:1 Changed 7 years ago by mtredinnick

  • Cc mtredinnick removed
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

On one hand, I don't really like that change because it's special-casing one methods out of the infinity of possibilities lazy() pretends to handle. On the other hand, I don't really like the way lazy() works at the moment, so this simple fix is redeemed because it fixes a problem in a reliable fashion.

I was thinking of writing a proper lazy()-like function specifically for things that returned bytestrings and unicodes (i.e. the i18n functions) and was understandable, but this fix will work for now (and maybe forever).

Thanks, Ivan.

comment:2 Changed 7 years ago by mtredinnick

Looking at this change a bit closer, particularly the part in contrib.contenttypes, I'm wondering if that isn't actually a bug: if opts.verbose_name is a ugettext_lazy() result, then shouldn't we be explicitly de-activating any thread-local translation effects so that we always use the same (untranslated) name. Otherwise, there are going to be different results depending on your current locale setting, right?

More importantly, contrib.contenttypes.models needs changing to do the same thing on retrieval (which is when varying locales could really be in effect).

Am I missing anything? Fixing it isn't hard, but I want to check I'm not being stupid.

I only stumbled on this because your patch means we can try to fix smart_unicode() to do the right thing (be lazy!) and it was causing bugs in the Content Type stuff.

comment:3 Changed 7 years ago by Ivan Sagalaev <Maniac@…>

As I understand verbose_name should be locale dependent since it's primarily used as human-readable title in admin. In that line it is used as a default parameter for get_or_create and hence doesn't take part in querying that goes strictly by app_label and model.

comment:4 Changed 7 years ago by mtredinnick

I've no problem with verbose name being a lazy translation. That's correct.

The problem is that if the get_or_create() method has to create the object (and this happens), the value of the name inserted into the database will depend upon the current locale, which means it's almost randomly chosen from amongst all the possible translations of that name. It should always give the same result, ideally.

comment:5 Changed 7 years ago by mtredinnick

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

(In [5239]) unicode: Fixed #4292 -- Added support for unicode to lazy() objects.
Thanks, Ivan Sagalaev.

comment:6 Changed 7 years ago by mtredinnick

[5240] also contains some of the changes from this patch. I wanted to do all the contentype changes in one hit.

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.