Opened 17 years ago

Closed 14 years ago

#3530 closed (fixed)

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

Reported by: Herbert Poul <herbert.poul@…> Owned by: John Engdal
Component: Core (Other) Version: dev
Severity: Keywords: urlconf permalink ROOT_URLCONF reverse
Cc: herbert.poul@…, joesaccount@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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

Attachments (3)

permalink_with_additional_function_arg.diff (1.3 KB ) - added by Herbert Poul <herbert.poul@…> 17 years ago.
simple patch to allow passing an additional function arg which returns the request object, or directly an urlconf list
permalink_with_additional_function_arg_fixed.diff (1.3 KB ) - added by Herbert Poul <herbert.poul@…> 17 years ago.
discovered a small bug in the patch
urlresolvers_reverse_urlconf_func.diff (1.0 KB ) - added by John Engdal 16 years ago.
allows the user to specify a function in settings.py that returns the urlconf to be used in reverse.

Download all attachments as: .zip

Change History (18)

comment:1 by Chris Beaven, 17 years ago

Triage Stage: UnreviewedAccepted

Looks valid to me.

by Herbert Poul <herbert.poul@…>, 17 years ago

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

comment:2 by Herbert Poul <herbert.poul@…>, 17 years ago

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

by Herbert Poul <herbert.poul@…>, 17 years ago

discovered a small bug in the patch

comment:3 by Herbert Poul <herbert.poul@…>, 17 years ago

Has patch: set

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

comment:4 by Simon G. <dev@…>, 17 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by anonymous, 17 years ago

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

comment:6 by Malcolm Tredinnick, 17 years ago

Triage Stage: Ready for checkinAccepted

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.

comment:7 by James Bennett, 17 years ago

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

comment:8 by Herbert Poul <herbert.poul@…>, 17 years ago

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

comment:9 by Malcolm Tredinnick, 17 years ago

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

comment:10 by James Bennett, 17 years ago

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

comment:11 by Gary Wilson, 16 years ago

Needs tests: set
Patch needs improvement: set

by John Engdal, 16 years ago

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

comment:12 by John Engdal, 16 years ago

Keywords: reverse added
Owner: changed from nobody to 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'

comment:13 by Chris Beaven, 16 years ago

See also: #5034

comment:14 by jos3ph, 15 years ago

Cc: joesaccount@… added

comment:15 by Chris Beaven, 14 years ago

Resolution: fixed
Status: newclosed

Fixed in [11740]

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