Code

Opened 7 years ago

Closed 4 years ago

#3530 closed (fixed)

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

Reported by: Herbert Poul <herbert.poul@…> Owned by: neoj
Component: Core (Other) Version: master
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: UI/UX:

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@…> 7 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@…> 7 years ago.
discovered a small bug in the patch
urlresolvers_reverse_urlconf_func.diff (1.0 KB) - added by neoj 6 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 Changed 7 years ago by SmileyChris

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

Looks valid to me.

Changed 7 years ago by Herbert Poul <herbert.poul@…>

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

comment:2 Changed 7 years ago by 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

Changed 7 years ago by Herbert Poul <herbert.poul@…>

discovered a small bug in the patch

comment:3 Changed 7 years ago by Herbert Poul <herbert.poul@…>

  • 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 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

comment:5 Changed 7 years ago by anonymous

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

comment:6 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 7 years ago by ubernostrum

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 Changed 7 years ago by 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 ..

comment:9 Changed 7 years ago by mtredinnick

@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 Changed 7 years ago by ubernostrum

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

comment:11 Changed 7 years ago by gwilson

  • Needs tests set
  • Patch needs improvement set

Changed 6 years ago by neoj

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

comment:12 Changed 6 years ago by neoj

  • Keywords reverse added
  • Owner changed from nobody to neoj

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 Changed 6 years ago by SmileyChris

See also: #5034

comment:14 Changed 5 years ago by jos3ph

  • Cc joesaccount@… added

comment:15 Changed 4 years ago by SmileyChris

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

Fixed in [11740]

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.