reverse_for_language does not work when called with different languages

Question

reverse_for_language does not work when called with different languages

Closed this issue 14 years ago · 20 comments

Here's my ipython test output that explains the issue:

In [5]: reverse_for_language('pl-gallery', 'et', kwargs=dict(slug='foo'))
Out[5]: '/et/galerii/foo/'

In [6]: reverse_for_language('pl-gallery', 'en', kwargs=dict(slug='foo'))
Out[6]: '/en/galerii/foo/'

The output for those languages should be different, but it's always whatever the function returned for the first call.

Answer 1 · 2011-03-11T17:23:17.000Z

This looks like the issue that's caused by Django's URL caching. Try calling django.core.urlresolvers.clear_url_caches() between calls to reverse_for_language.

transurlvania comes with a middleware that does this for you called URLCacheResetMiddleware which you should be using.

It's not ideal, but it's the best I think we can do without changing Django.

Answer 2 · 2011-03-11T17:46:32.000Z

In [11]: reverse_for_language('pl-gallery', 'et', kwargs=dict(slug='foo'))
Out[11]: '/et/galerii/foo/'

In [12]: urlresolvers.clear_url_caches()

In [13]: reverse_for_language('pl-gallery', 'en', kwargs=dict(slug='foo'))
Out[13]: '/en/galerii/foo/'

Answer 3 · 2011-03-11T17:49:59.000Z

Django 1.3 RC1

Answer 4 · 2011-03-11T20:18:08.000Z

Crap. Thanks for logging the issue.

Could you do a couple things for me to help me diagnose this?

First, run the tests (./tests/run_tests.py) with Django 1.3RC1, then run them with Django 1.2.5. Let me know which tests fail in each case.

Answer 5 · 2011-03-12T10:21:29.000Z

OK, I'm not sure if I did it right, but here's what I got. Inside an activated virtualenv with Django 1.3 RC1 installed, I ran:

$ pip install -e git+https://github.com/trapeze/transurlvania.git#egg=transurlvania

$ ~/.virtualenvs/myvenv/src/transurlvania/tests/run_tests.py 
/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/db/__init__.py:19: DeprecationWarning: settings.DATABASE_* is deprecated; use settings.DATABASES instead.
  DeprecationWarning
/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/db/__init__.py:60: DeprecationWarning: Short names for ENGINE in database configurations are deprecated. Prepend default.ENGINE with 'django.db.backends.'
  DeprecationWarning
Creating test database for alias 'default'...
./Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/core/context_processors.py:27: DeprecationWarning: The context processor at `django.core.context_processors.auth` is deprecated; use the path `django.contrib.auth.context_processors.auth` instead.
  DeprecationWarning
.......E..................
======================================================================
ERROR: testTranslatedURL (garfield.tests.LangInPathTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/erik/.virtualenvs/myvenv/src/transurlvania/tests/garfield/tests.py", line 116, in testTranslatedURL
    response = self.client.get('/en/garfield/the-cat/')
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/test/client.py", line 445, in get
    response = super(Client, self).get(path, data=data, **extra)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/test/client.py", line 229, in get
    return self.request(**r)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/core/handlers/base.py", line 111, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/Users/erik/.virtualenvs/myvenv/src/transurlvania/tests/garfield/views.py", line 27, in comic_strip_list
    template_object_name='comic_strip',
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/views/generic/list_detail.py", line 108, in object_list
    return HttpResponse(t.render(c), mimetype=mimetype)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 123, in render
    return self._render(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/test/utils.py", line 57, in instrumented_test_render
    return self.nodelist.render(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 744, in render
    bits.append(self.render_node(node, context))
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 757, in render_node
    return node.render(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/loader_tags.py", line 55, in render
    result = self.nodelist.render(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 744, in render
    bits.append(self.render_node(node, context))
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 757, in render_node
    return node.render(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 792, in render
    output = self.filter_expression.resolve(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 510, in resolve
    obj = self.var.resolve(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 653, in resolve
    value = self._resolve_lookup(context)
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/base.py", line 698, in _resolve_lookup
    current = current()
  File "/Users/erik/.virtualenvs/myvenv/lib/python2.6/site-packages/django/template/loader_tags.py", line 71, in super
    render_context = self.context.render_context
AttributeError: 'BlockNode' object has no attribute 'context'

----------------------------------------------------------------------
Ran 27 tests in 0.299s

FAILED (errors=1)
Destroying test database for alias 'default'...

Answer 6 · 2011-03-12T10:29:17.000Z

WIth Django 1.3 RC1 uninstalled and 1.2.5 installed in the same virtualenv:

$ ~/.virtualenvs/myvenv/src/transurlvania/tests/run_tests.py 
Creating test database 'default'...
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_user_permissions
Creating table auth_user_groups
Creating table auth_user
Creating table auth_message
Creating table django_admin_log
Creating table django_content_type
Creating table django_session
Creating table garfield_comicstrip
Installing index for auth.Permission model
Installing index for auth.Group_permissions model
Installing index for auth.User_user_permissions model
Installing index for auth.User_groups model
Installing index for auth.Message model
Installing index for admin.LogEntry model
No fixtures found.
...........................
----------------------------------------------------------------------
Ran 27 tests in 0.244s

OK
Destroying test database 'default'...

Answer 7 · 2011-03-12T20:55:45.000Z

Okay, new information.
I have an app with translated URLs. I'm including that app's URLs into the main URLconf, but I don't want the app to have a URL prefix, so this is what I did:

urlpatterns += lang_prefixed_patterns('',
    (r'', include('my_app.urls'),
)

Turns out the reason why it didn't work even after clear_url_caches was that I hadn't used the correct version:

from transurlvania.defaults import url
urlpatterns += lang_prefixed_patterns('',
    url(r'', include('my_app.urls'),
)

But this doesn't work either, beucause transurlvania.defaults.url fails with an empty regex argument and this is what the URLconf will looks like:

Using the URLconf defined in myproject.urls, Django tried these URL patterns, in this order:
^admin/
^$ [name='home']
^et/ Project-Id-Version: PACKAGE VERSION Report-Msgid-Bugs-To: POT-Creation-Date: 2011-03-10 02:55+0200 PO-Revision-Date: 2011-03-10 02:56+0200 Last-Translator: FULL NAME <EMAIL@ADDRESS> Language-Team: LANGUAGE <LL@li.org> Language: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plural-Forms: nplurals=2; plural=(n != 1)
^static\/(?P<path>.*)$
^media/(?P<path>.*)$

So with an empty match pattern, what gets included is some crap.
I did manage to solve this by using a workaround instead:

urlpatterns += lang_prefixed_patterns('',
    url(r'^.{0}', include('my_app.urls')),
)

Since I don't know the internals of transurlvania (yet?), I don't know why exactly it works with a non-empty pattern and fails with an empty one.

Also, a side note:

In [2]: from transurlvania.urlresolvers import reverse_for_language

In [3]: reverse_for_language('gallery', 'et', kwargs=dict(slug='foo'))
Out[3]: '/et/galerii/foo/'

In [4]: reverse_for_language('gallery', 'en', kwargs=dict(slug='foo'))
Out[4]: '/en/gallery/foo/'

As you can see, clear_url_caches is not needed. I think clear_url_caches is only needed for URL resolving, not reversing, but I don't know why.

My proposition is to do one of the following (or both):

At add an actual usage example to the README/docs and draw attention to how and where exactly one has to use the custom URLconf functions provided by transurlvania: where is url needed and where should lang_prefixed_patterns be put etc, especially in the context of non-trivial/nested URLconfs. Also point out the fact that transurlvania.defaults.url fails to work with an empty pattern.
Fix the empty pattern issue, if not in an elegant way, then perhaps by adding a check to transurlvania.defaults.url:

if regex == '':
    regex = r'^.{0}'

And also, thank you for the cool piece of code you've written — I wish the Django URLconf mechanism would be less stubborn/rigid/flat so as to enable one to actually achieve all this with less fuss (imagine repoze.bfg/Pyramid or Grok doing the same thing).

Answer 8 · 2011-03-12T23:46:12.000Z

Hey Erik,

Thanks so much for the feedback. I've tagged a new version that fixes the empty regex issue and adds support for the terser url pattern syntax.

Please give it a try and let me know if it fixes the issues you had.

Answer 9 · 2011-03-13T15:58:42.000Z

Hmm, I just realised that I can use Django's own url function for the inclusion, and just use the custom url function for patterns that are actually translated.

Answer 10 · 2011-03-13T17:42:20.000Z

Ok, I tested the new version. Here's what I got.

transurlvania.defaults.url now works with r''
inside a lang_prefixed_patterns you can skip the url(...) call altogether and use the more basic (r'...', ...) form
but you cannot use Django's own url function inside a lang_prefixed_pattern form — the original issue will pop up again; this is kind of confusing now because you can use the (r'...', ...) form but not the built-in url function. I'd suggest either making both work, or neither. Or maybe rename transurlvania.defaults.url to translated_url to show that it is explicitly meant for translated URL patterns and is not needed for inclusions. A matter of taste, I guess. And probably a non-issue anyway for non-perfectionists.

Answer 11 · 2011-03-13T18:57:02.000Z

I designed transurlvania.defaults to be used as a replacement for django.conf.urls.defaults. What's the use case for mixing functions from both modules?

If you wanted to rename one of the functions I think it would make more sense to rename "patterns" to "trans_patterns" or something like that.

What do you think?

Answer 12 · 2011-03-13T19:10:48.000Z

I did realise that the original intent of the transurlvania.defaults module was to provide a drop in replacement for django.conf.urls.defaults, but it was a bit confusing because it wasn't obvious which function provides the actual functionality, which maybe in turn caused confusion about which to use when. I guess it also has to do with making stuff self-documenting.

So I'm personally only importing at most transurlvania.defaults.lang_prefixed_patterns and transurlvania.defaults.url, and will explicitly stick with importing include and patterns from django.conf.urls.defaults, and also url if I don't need a translated URL pattern (or need to mix both translated and untranslated and want to signify which is which). So I thought it would make a nice symmetry to have lang_prefixed_patterns and translated_url, and have the user always explicitly import everything else from django.conf.urls.defaults.

But again, it's a matter of taste (I personally prefer explicit, obvious and self-documenting over implicit and thus less obvious). And I've used this (very useful) piece of software for much less than you have.

Answer 13 · 2011-03-13T23:43:47.000Z

I can see your point of view in terms of distinguishing translatable URLs from non-translatable URLs, would it help if I renamed the url func to "trans_url" and kept an "url" func that's an alias to it? That way it could operate as a drop-in replacement but it would also communicate which specific function to import if you wanted to mix translatable and non-translatable URLs.

I also just want to point out that there's no reason you can't use the translatable defaults for all the URLs in your site. The URLs work the same as django's normal defaults until you start providing translations.

Answer 14 · 2011-03-14T00:50:27.000Z

Hmm, yeah, you're right, there's not really too much need to distinguish the two. And you're the final judge anyway :) But the documentation could still be a little bit more detailed and with an actual example or two. I could actually help you with that.

Answer 15 · 2011-03-14T01:12:39.000Z

I would really appreciate your help with the documentation. You'll have a clearer perspective on what users need to know.

Answer 16 · 2011-03-14T10:52:04.000Z

Hey, please check out the two commits that I've made at https://github.com/eallik/transurlvania before I send a pull request.

The first one is only additional details in the documentation.

The second one actually adds a few (documented) util classes that I use. If you think it's too much to add them or there's a problem with them, just ignore this commit.

Answer 17 · 2011-03-16T15:17:20.000Z

The doc additions look great. I'm not sure I understand the value of the mix-in. If you're using it with models that don't have any multi-lang functionality, I don't see what it gets you that the view-based URL generation does. If it's designed to be used with models that do have some multi-lang functionality, I can't see how the mix-in can integrate with that to get the slug field in the appropriate language or whatever.

this_page_in_lang calls get_translation on model objects because it was originally developed along side a multi-lingual ORM solution. We didn't open source that because we weren't as happy with it, but the get_translation pattern seemed decent and I vaguely remember reading that django-multilingual supported it.

Answer 18 · 2011-03-16T15:25:26.000Z

this_page_in_lang will return the URL using the translated URL patterns, but using the slug of the current language. So you get stuff like /fr/galerie/slug-in-english/ when you call {% this_page_in_lang 'fr' %} when English is active. If there's another approach to solving this without implementing the proxy-returning get_language, I'd be glad to implement that instead :) To be honest, though, I like the idea of having a get_absolute_url(lang=None) approach.

I probably didn't look enough into the URL scheme lookup customization stuff and the translators module because there's no documentation/examples on those, but I'll just read the source code then.

Answer 19 · 2011-03-16T15:50:40.000Z

I understand the issue of having slugs of the wrong language in the URL. Another reason you'd want this would be if you had separate, related objects that represented the same data, but in different languages. It's for this use case that we used get_translation instead of get_absolute_url(self, language), but I can see your point. Using get_absolute_url is more flexible to different solutions.

How about this: add support for calling obj.get_absolute_url(language) in the translators code, falling back to the existing get_translation if that call fails. That way we don't break compatibility with existing code and use cases like yours are met without having to add a mix-in and a proxy class.

Answer 20 · 2011-03-16T16:04:52.000Z

That's actually exactly the idea I just had in my head.