django.db.models.permalink should use request.urlconf if defined

[Herbert Poul <herbert.poul@…>]

#3187 made it possible to use a middleware to overwrite settings.ROOT_URLCONF by setting request.urlconf

permalink currently only uses settings.ROOT_URLCONF which obviously leads to problems.. with e.g. syndication-feed framework or sitemaps
so imho permalink should also use request.urlconf if defined.

[Chris Beaven]

Looks valid to me.

[Herbert Poul <herbert.poul@…>]

simple patch to allow passing an additional function arg which returns the request object, or directly an urlconf list

[Herbert Poul <herbert.poul@…>]

i've attached a simple patch which can receive an additional function parameter which is ment to be used with a thread local middleware like: ​http://code.djangoproject.com/wiki/CookBookThreadlocalsAndUser - all that is needed is a function that returns the current request object..
seems to work fine for me

[Herbert Poul <herbert.poul@…>]

discovered a small bug in the patch

[Herbert Poul <herbert.poul@…>]

i found a small problem in my old patch - it is fixed in permalink_with_additional_function_arg_fixed.diff

anyway . .is there any chance this patch gets into django one day ? .. if not i would simply include it in my own codebase instead of patching django..
would be nice to know your opinion ..

[anonymous]

Is there a reason this is still waiting to be checked in?

[Malcolm Tredinnick]

There's a bit more needed before this is ready to go in.

Firstly, it would no longer work as a decorator after this change. It looks like the new signature for permalink will have to be

def permalink(func=None, get_urlconf_func=None):

so that you can call it as

@permalink(get_urlconf_func = blah)
def get_absolute_url(self):
   # ...

In other words, both these forms have to work

get_absolute_url = permalink(get_absolute_url, blah)
get_absolute_url = permalink(get_urlconf_func = blah)(get_absolute_url)

We'll have to use a named parameter in the second case (and the decorator-with-argument case) because we can't type check the args to see which form is being used: both arguments are callables.

Secondly, let's just have get_urlconf_func return the urlconf directly. The user's function can worry about whether that means returning request.urlconf or something else (I'm trying to avoid tying it tightly to any assumptions about usage).

No need to type-check the result (by the way, using isinstance() is preferred to type() checks), since anything that acts appropriately is fine and we shouldn't restrict that unnecessarily.

So, the inner function now looks like this:

   def inner(*args, **kwargs):
      if get_urlconf_func:
           urlconf = get_urlconf_func()
      else:
           urlconf = None
      bits = func(*args, **kwargs)
      return reverse(bits[0], urlconf, *bits[1:3])

Half the length and with less assumptions. That still needs to be wrapped in the necessary code to make it work as a decorator, though.

Finally, some documentation needs to be added in the appropriate place in model_api.txt.

[James Bennett]

FWIW I'd be -1 on adding a new required argument to permalink. That's a decorator that needs to stay as simple as possible for the common case. (adding the get_urlconf_func bit as an optional argument for advanced use would be OK, though).

[Herbert Poul <herbert.poul@…>]

the get_urlconf_func argument was meant to be optional anyway .. although i haven't tested it with the "@decorator" syntax - that's probably what isn't working.. (sorry, i don't have much experience with decorators yet.. or python in general actually)

but i think the code part

urlconf = get_urlconf_func() 
if type(urlconf) != list: 
    # If type is no list, we assume it is a request object and 
    # look for a 'urlconf' attribute 
    urlconf = getattr(urlconf, 'urlconf', None) 

isn't that bad imho (except.. the usage of type(..)) - because it allows to use the request object instead of directly the urlconf variable .. and the logic that there can be a 'urlconf' attribute in the request object is already within django (BaseHandler) .. so it makes no additional assumptions

but anyway .. i have no problem if it directly requires the urlconf dictionary ..

[Malcolm Tredinnick]

@ubernostrum: the extra function arg is optional in both Herbet's and my version. It's fully backwards compatible.

@Herbet: I'd leave it out. It an extra assupmtion in the permalink() function we have to remember and maintain and it's not needed (i.e. it doesn't really make any external code significantly more complex if we leave it out). The reduced version says 'give me what I need', your original interface was 'give me what I need or something that is kind of like what I need and I'll rummage around inside and pull out what I need'. We don't need the second option.

It's the difference between get_urlconf_function being

lambda : get_current_user()   # with your API

versus

lambda : get_current_user().urlconf   # with my API
# or lambda : getattr(get_current_urse(), 'urlconf', None) to be really robust

and it saves at least two lines of code in permalink().

My preference is to keep the core code as simple as possible and if we're adding a new interface, as here, add the smallest interface necessary. I realise neither way is a disaster, but neatness counts.

[James Bennett]

#5034 was duped for this; both patches should roll together into a single solution which fixes reverse somehow.

[John Engdal]

allows the user to specify a function in settings.py that returns the urlconf to be used in reverse.

[John Engdal]

My patch allows the programmer to specify a function in settings.py that will return the urlconf to be used by reverse.

Example:
A middleware would save request.urlconf in thread locals. A function would return that value from thread locals. That function would be specified in settings.py like this:

DYNAMIC_URLCONF_FUNCTION = 'djangosite.middleware.url_locale.dynamic_urlconf'

[Chris Beaven]

See also: #5034

[Chris Beaven]

Fixed in [11740]