Opened 16 years ago

Closed 15 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 15 years ago.
7206-2.patch (1.6 KB) - added by tlpinney 15 years ago.
Fixes problem with the test failing, adds a couple regression tests

Download all attachments as: .zip

Change History (11)

comment:1 Changed 16 years ago by ElliottM

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

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 Changed 15 years ago by Sung-jin Hong

Component: UncategorizedCore framework

comment:4 Changed 15 years ago by Eric Holscher

milestone: 1.0
Triage Stage: UnreviewedAccepted

Changed 15 years ago by tlpinney

Attachment: 7206.patch added

comment:5 Changed 15 years ago by tlpinney

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)

Changed 15 years ago by tlpinney

Attachment: 7206-2.patch added

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

comment:6 Changed 15 years ago by tlpinney

Triage Stage: AcceptedReady for checkin

comment:7 Changed 15 years ago by Malcolm Tredinnick

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 Changed 15 years ago by Malcolm Tredinnick

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

milestone: 1.0

Milestone 1.0 deleted

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