Opened 16 years ago
Closed 11 years ago
#7571 closed Bug (fixed)
named groups in the regexes of include() urls break reverse()
Reported by: | Trevor Caira | Owned by: | afuna |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | flavio.curella@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
# In project urls.py: from django.conf.urls.defaults import * urlpatterns = patterns('', (r'^(?P<foo>\w+)/(?P<bar>\w+)/', include('asdf.urls')), ) # In asdf/urls.py: from django.conf.urls.defaults import * urlpatterns = patterns('', url(r'^(?P<bat>\w+)/$', lambda: None, name='test'), ) # Running this in ./manage.py shell... reverse('test', args=['foo', 'bar', 'bat']) # returns: '/foo/bar/foo/' # instead of: '/foo/bar/bat/'
Change History (14)
comment:1 by , 16 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
comment:2 by , 16 years ago
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
If args
is this incorrect way to use reverse with include()
'd URLs, it should raise an exception, not give surprising results, and this should be documented. There is no mention of this behavior in [1]. Also, there is no way to use kwargs
with the {% url %}
template tag.
[1] http://www.djangoproject.com/documentation/url_dispatch/#including-other-urlconfs
comment:3 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
Keyword arguments with the url template tag are documented. http://www.djangoproject.com/documentation/templates/#url
Leaving open so someone else can address if an error should be raised here.
comment:4 by , 16 years ago
Resolution: | → worksforme |
---|---|
Status: | reopened → closed |
The URL dispatch documentation explains quite clearly the difference between named and non-named groups, and the fact that they result in keyword and positional arguments, respectively. Meanwhile, the documentation for the 'url' tag quite clearly shows how to use keyword arguments.
Please do not reopen this ticket unless you can demonstrate a genuine bug.
comment:5 by , 16 years ago
Then why does reverse()
work at all with args
if only named groups have been given in the URL regex? In fact, they work perfectly well until someone tries to use include()
in this way.
I suspect many people might be relying on this undocumented feature, and may find it surprising when they try to use include()
like this. Perhaps the best solution would be to fully support matching named groups with positional parameters (as partial support already exists), and provide documentation of it.
Otherwise a developer might be lured into the trap of depending on this feature and be very surprised to learn that in order to factor out the matching of parts of the url above the application, they have to fundamentally alter the way those URLs are resolved.
comment:6 by , 16 years ago
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
Upon further testing, this issue doesn't seem to be related to named URL groups in particular. Witness:
# In project urls.py: from django.conf.urls.defaults import * urlpatterns = patterns('', (r'^(\w+)/', include('asdf.urls')), ) # In asdf/urls.py: from django.conf.urls.defaults import * urlpatterns = patterns('', url(r'^(\w+)/$', lambda: 0, name='asdf'), ) # ./manage.py shell: from django.core.urlresolvers import reverse reverse('asdf', args=['foo', 'bar']) # gives: '/foo/foo/' # and from django.core.urlresolvers import RegexURLResolver from django.conf import settings r = RegexURLResolver('^/', settings.ROOT_URLCONF) r.resolve('/foo/bar/') (<function <lambda> at 0x87a317c>, ('bar',), {})
comment:7 by , 16 years ago
Cc: | added |
---|
comment:9 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:10 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Accepted |
UI/UX: | unset |
trevor's comment demonstrates a genuine bug.
comment:11 by , 12 years ago
it seems okay now:
reverse('test', args=['foo', 'bar', 'bat']) # gives: '/foo/bar/bat/'
comment:12 by , 12 years ago
Status: | reopened → new |
---|
comment:13 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
So, it looks like this is actually two things.
The original issue as described has been fixed. This is a case of outer regex + included regex, both using named groups. There's a test case in urlpatterns_reverse/tests.py that matches and confirms:
('inner-extra', '/outer/42/extra/inner/', ['42', 'inner'], {}),
in test_data
where the relevant url patterns are: url(r'^outer/(?P<outer>\d+)/', include('urlpatterns_reverse.included_urls')),
and url(r'^extra/(?P<extra>\w+)/$', empty_view, name="inner-extra"),
comment:6 is a different thing and that one is still broken. That is a case of both the outer and inner regex using positional parameters, and the positions overlapping when the inner regex is appended to the outer regex. So for example:
outer regex: outer/(\d+)/
inner regx: inner/(\d+)/
arguments: 123, 456
resolves to /outer/123/inner/123/
comment:14 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Named groups are populated with keyword arguments, not positional arguments.