Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27367 closed Cleanup/optimization (fixed)

Document behaviour when several urls have the same name

Reported by: Christoph Schindler Owned by: Robert Roskam
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: reinout@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently -- as far as i can tell -- it is no problem to give the same name to different urls and then reverse them:

    url(r'^spam/$', 'myapp.views.index', name='myview'),
    url(r'^spam/(?P<ham>[\w-]+)/$', 'myapp.views.detail', name='myview'),
    url(r'^spam/(?P<ham>[\w-]+)/(?P<eggs>[\w-]+)/$', 'myapp.views.detail', name='myview'),
In [8]: reverse('myview', kwargs={'ham': 'foo', 'eggs': 'bar'})
Out[8]: u'/spam/foo/bar/'

In [9]: reverse('myview', kwargs={'ham': 'foo'})
Out[9]: u'/spam/foo/'

In [10]: reverse('myview')
Out[10]: u'/spam/'

But this is not documented and it is not clear, whether this is supported or unsupported behaviour.

It should either be stated clearly that url names should be unique /or/ that it is possible to "overload" url patterns by giving them the same name when they have a different number of arguments.

Change History (14)

comment:1 by Simon Charette, 8 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.10master

comment:2 by Robert Roskam, 8 years ago

Owner: changed from nobody to Robert Roskam
Status: newassigned

comment:3 by Marten Kenbeek, 8 years ago

You can use multiple urls with the same name. These urls will be tried in reverse order, so starting at the bottom of the list. All arguments passed to reverse() must be used by the pattern to be considered valid. Default arguments (passed as the kwargs parameter to url()) may be passed to reverse() as well, but if they are passed, the values must match the actual arguments to be considered a valid pattern. Consider these patterns:

    url(r'^spam/$', 'myapp.views.index', name='myview'),  # 1
    url(r'^spam/(?P<ham>[\w-]+)/$', 'myapp.views.detail', name='myview'),  # 2
    url(r'^spam/(?P<ham>[\w-]+)/(?P<eggs>[\w-]+)/$', 'myapp.views.detail', name='myview'),  # 3
    url(r'^spam/(?P<ham>[\w-]+)/$', 'myapp.views.detail', kwargs={'eggs': 'bar'}  name='myview'),  # 4

Both of these would successfully reverse pattern 4:

>>> reverse('myview', kwargs={'ham': 'foo'})
'/spam/foo/'
>>> reverse('myview', kwargs={'ham': 'foo', 'eggs': 'bar'})
'/spam/foo/'

However, this would not match pattern 4, as the value of eggs does not match the original value of eggs passed to url(). Instead, it will fall through and match pattern 3:

>>> reverse('myview', kwargs={'ham': 'foo', 'eggs': 'baz'})
'/spam/foo/baz/'

comment:4 by Robert Roskam, 8 years ago

Not sure if the behavior was intended or not based on available documentation. Rather than making a change, I've put tests around how it functions and updated documentation. You can see my changes here: https://github.com/django/django/pull/7417/files

comment:5 by Tim Graham, 8 years ago

Has patch: set

comment:6 by Reinout van Rees, 8 years ago

I've reviewed the pull request. The test looks handy.

I'm not sure about the text. The text might have to be moved somewhere else.

comment:7 by Reinout van Rees, 8 years ago

Oh, I retract my comment about it having to move somewhere else :-)

There is no further page where there's some urls/reverse() explanation: the https://docs.djangoproject.com/en/1.10/topics/http/urls/ page is huge. So the text belongs there. (It might be worthwhile to split up the huge page at another point in time, but that's outside of the scope of this ticket).

comment:8 by Reinout van Rees, 8 years ago

Patch needs improvement: set

I've added a pull request to Robert's branch with my suggested changes in there: https://github.com/raiderrobert/django/pull/1

(I've checked the "patch needs improvement" checkbox just for clarity, feel free to uncheck it if my changes aren't handy).

comment:9 by Reinout van Rees, 8 years ago

Cc: reinout@… added

comment:10 by Reinout van Rees, 8 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

I'd say it is ready for checkin now! Nice to have some documentation for this. I was surprised that it was under-documented :-) Thanks, Robert!

(My PR for the original PR has been merged).

comment:11 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

The content is looking good and I left some comments for further improvement.

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:13 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 98bcc5d8:

Fixed #27367 -- Doc'd and tested reversing of URLs with the same name.

Thanks Reinout van Rees for contributing to the patch.

comment:14 by Tim Graham <timograham@…>, 8 years ago

In c0916144:

[1.11.x] Fixed #27367 -- Doc'd and tested reversing of URLs with the same name.

Thanks Reinout van Rees for contributing to the patch.

Backport of 98bcc5d81bca578f3a5b4d47907ba4ac40446887 from master

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