Opened 7 years ago

Closed 7 years ago

Last modified 3 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:

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

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 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 7 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 7 years ago by serialx

  • Component changed from Uncategorized to Core framework

comment:4 Changed 7 years ago by ericholscher

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

Changed 7 years ago by tlpinney

comment:5 Changed 7 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 7 years ago by tlpinney

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

comment:6 Changed 7 years ago by tlpinney

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 7 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 7 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 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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