Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#7206 closed (fixed)

django.core.urlresolvers.reverse doesn't work properly

Reported by: askfor@… Owned by: mtredinnick
Component: Core (Other) Version: master
Severity: Keywords: url reverse
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


here is the urlpatterns


then i test the reverse function as follows:

>>> from django.core.urlresolvers import reverse
>>> print reverse('test1')

the url i expect is '/test/1/' instead of '/test/1/?'
the '?' in the urlpatterns was add for matching both '/test/1/' and '/test/1

>>> print reverse('test2')
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "....django\django\core\", line 297, in
  File "....django\django\core\", line 283, in
  File "....django\django\core\", line 88, in
  File "C:\Python25\lib\", line 142, in sub
    return _compile(pattern, 0).sub(repl, string, count)
  File "....\django\django\core\", line 124, in
NoReverseMatch: Not enough positional arguments passed in

(?i) was added for matching '/test/2' or '/Test/2' and so forth

Attachments (2)

7206.patch (799 bytes) - added by tlpinney 8 years ago.
7206-2.patch (1.6 KB) - added by tlpinney 8 years ago.
Fixes problem with the test failing, adds a couple regression tests

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by ElliottM

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The CommonMiddleare automatically appends slashes to the end of urls if one isn't already there, so matching both isn't necessary.

That's not to say this isn't a bug, of course.

comment:2 Changed 8 years ago by askfor@…

  • Has patch set

got a error while attaching this patch file.

Index: django/core/


--- django/core/	(revision 7367)

+++ django/core/	(working copy)

@@ -85,8 +85,9 @@

     Raises NoReverseMatch if the args/kwargs aren't valid for the regex.
     # TODO: Handle nested parenthesis in the following regex.
-    result = re.sub(r'\(([^)]+)\)', MatchChecker(args, kwargs), regex.pattern)
-    return result.replace('^', '').replace('$', '')
+    pattern = re.sub(r'\(\?[iLmsux]+\)','',regex.pattern)
+    result = re.sub(r'\(([^)]+)\)', MatchChecker(args, kwargs), pattern)
+    return result.replace('^', '').replace('$', '').replace('?','')
 class MatchChecker(object):
     "Class used in reverse RegexURLPattern lookup."

comment:3 Changed 8 years ago by serialx

  • Component changed from Uncategorized to Core framework

comment:4 Changed 8 years ago by ericholscher

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

Changed 8 years ago by tlpinney

comment:5 Changed 8 years ago by tlpinney

This patch causes the following failure.

wireless-128-62-162-35:tests tlpinney$ ./ --settings=foobar.settings urlpatterns_reverse
FAIL: test_urlpattern_reverse (regressiontests.urlpatterns_reverse.tests.URLPatternReverse)
Traceback (most recent call last):
  File "/Users/tlpinney/hg/django_trunk/tests/regressiontests/urlpatterns_reverse/", line 37, in test_urlpattern_reverse
    self.assertEquals(got, expected)
AssertionError: 'hardcoded/doc\\.pdf' != 'hardcoded/doc.pdf'

Ran 1 test in 0.004s

FAILED (failures=1)

Changed 8 years ago by tlpinney

Fixes problem with the test failing, adds a couple regression tests

comment:6 Changed 8 years ago by tlpinney

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 8 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Triage Stage changed from Ready for checkin to Accepted

This is in the same boat as #4915, #6934, etc. It's unmaintainable to fix things piecemeal. I have a slightly larger rewrite that I'll finish up shortly. Leaving open so that I don't forget to consider this option (I'm not sure allowing ?i in URL reg-exp patterns designed for reversing is a good idea, but I'll think about it).

comment:8 Changed 8 years ago by mtredinnick

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

(In [8760]) A rewrite of the reverse URL parsing: the reverse() call and the "url" template tag.

This is fully backwards compatible, but it fixes a bunch of little bugs. Thanks
to SmileyChris and Ilya Semenov for some early patches in this area that were
incorporated into this change.

Fixed #2977, #4915, #6934, #7206.

comment:13 Changed 5 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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