Opened 5 years ago

Closed 4 months ago

#15180 closed New feature (duplicate)

reverse doesn't check default_args

Reported by: olaf Owned by: nobody
Component: Core (URLs) Version: 1.3-beta
Severity: Normal Keywords: resolve default_args
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Lets say we have the following urls:

    (r'^photos/?$', 'views.photo_index', {'lang':'en'}, "photo_index"),
    (r'^fotos/?$', 'views.photo_index', {'lang':'de'}, "photo_index"),
    (r'^photo/(?P<photo_id>[\d]+)$', 'views.photo', {"lang":"en"} ,"photo"),
    (r'^foto/(?P<photo_id>[\d]+)$', 'views.photo', {"lang":"de"} ,"photo"),

When I want to get the specific URL for a German visitor I wan't to just to get the reverse for photo an add a keyword argument {'lang':'de'}. It is very much writing to write for each language a different wrapper function. You also would need to write a bunch of if-else statements to select the correct one.

I saw that also some else expected this and asked a question at stackoverflow about this, but he got only workaround as answers. I added a few lines to correct behavior, and I hope this is useful for someone. I am not sure, if I broke something so it would be nice someone else could test this code also.

Attachments (3)

urlresolvers.diff (1.9 KB) - added by olaf 5 years ago.
urlresolvers-v2.diff (3.4 KB) - added by olaf 4 years ago.
urlresolvers-v2.2.diff (3.4 KB) - added by olaf 4 years ago.

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by olaf

comment:1 Changed 5 years ago by russellm

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

comment:2 Changed 5 years ago by russellm

I have a background suspicion that this will cause all sorts of havoc with existing URL lookups -- for example, existing views with default arguments that *aren't* using the default arguments to differentiate between views.

However, since the patch doesn't have any tests, I can't verify this quickly.

Marking this as a qualified accepted, the qualification being that the implementation *must* be backwards compatible with existing uses, and be tested as such. If that can't be achieved, then we'll need to close wontfix.

comment:3 Changed 4 years ago by olaf

Ok, I changed my patch a little bit. I added a new optional parameter to the reverse function. If check_default_args is not set(=None), reverse tries to find a match the standard way, if it will not succeed it tries one time again including the default_args. If it is set to False, it behaves just like the current reverse function. If it is set to true, it will check them from the beginning.

I would like to write those test cases, but I don't know how to use different url configs. Could you point me to an Howto or give me a small example, and I will write them.

Thanks

Changed 4 years ago by olaf

Changed 4 years ago by olaf

comment:4 Changed 4 years ago by julien

  • milestone 1.3 deleted

This can't make it into 1.3 unless comprehensive regression tests are provided. Olaf, if you're interested in writing some tests, take a look at the guide available at http://docs.djangoproject.com/en/dev/topics/testing/
Don't hesitate to ask questions on the django-users list too if need be.

comment:5 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:6 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:8 Changed 2 years ago by aaugustin

  • Component changed from Core (Other) to Core (URLs)

comment:9 Changed 4 months ago by bpeschier

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

This was fixed in #13154

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