Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7206 closed (fixed)

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

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

Description

here is the urlpatterns

  url(r'^test/1/?',views.test1,name='test1'),
  url(r'^(?i)test/2/?$',views.test2,name='test2'),

then i test the reverse function as follows:

>>> from django.core.urlresolvers import reverse
>>> print reverse('test1')
/test/1/?

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\urlresolvers.py", line 297, in
 reverse
  File "....django\django\core\urlresolvers.py", line 283, in
 reverse
  File "....django\django\core\urlresolvers.py", line 88, in
reverse_helper
  File "C:\Python25\lib\re.py", line 142, in sub
    return _compile(pattern, 0).sub(repl, string, count)
  File "....\django\django\core\urlresolvers.py", line 124, in
 __call__
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 16 years ago.
7206-2.patch (1.6 KB ) - added by tlpinney 16 years ago.
Fixes problem with the test failing, adds a couple regression tests

Download all attachments as: .zip

Change History (11)

comment:1 by ElliottM, 16 years ago

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 by askfor@…, 16 years ago

Has patch: set

got a error while attaching this patch file.

Index: django/core/urlresolvers.py

===================================================================

--- django/core/urlresolvers.py	(revision 7367)

+++ django/core/urlresolvers.py	(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 by Sung-jin Hong, 16 years ago

Component: UncategorizedCore framework

comment:4 by Eric Holscher, 16 years ago

milestone: 1.0
Triage Stage: UnreviewedAccepted

by tlpinney, 16 years ago

Attachment: 7206.patch added

comment:5 by tlpinney, 16 years ago

This patch causes the following failure.

wireless-128-62-162-35:tests tlpinney$ ./runtests.py --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/tests.py", 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)

by tlpinney, 16 years ago

Attachment: 7206-2.patch added

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

comment:6 by tlpinney, 16 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick
Triage Stage: Ready for checkinAccepted

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 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(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 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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