Opened 8 years ago

Closed 4 years ago

#7117 closed Bug (fixed)

Replace backslash in reverse URL matcher

Reported by: Bastian Kleineidam <calvin@…> Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The reverse URL pattern matcher should replace backslashes
stemming from regex escapes in the result URL.
Added also a test case.

Attachments (1)

commit-454b8e2 (1.4 KB) - added by Bastian Kleineidam <calvin@…> 8 years ago.

Download all attachments as: .zip

Change History (6)

Changed 8 years ago by Bastian Kleineidam <calvin@…>

comment:1 Changed 8 years ago by SmileyChris

  • Component changed from Uncategorized to Core framework
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

There are a lot more things to do to fix up the reverse URL matcher than this, but it is a simple change. I'll push to a design decision.

Should probably have a test case.

comment:2 Changed 8 years ago by Bastian Kleineidam <calvin@…>

  • Needs tests unset

There is a test case in the patch, it's just very small :)

What is it that needs fixing up in the reverse matcher?
Perhaps it is the fact that the reverse matcher cannot be a (perfect) function, since there might be two or more URLs that resolve to the same view. For example this pattern:

('docs/index[12].doc', 'docs.display')

resolves two different URLs to the exact same view. Same with multiple patterns pointing to the same view:

('docs/index1.doc', 'docs.display')
('docs/index2.doc', 'docs.display')

A reverse function for docs.display does not exist then. Perhaps {{reverse()}}} should throw ValueError in these cases to make the user aware of this issue, but that is a more invasive change.

comment:3 Changed 8 years ago by SmileyChris

Sorry Bastian, i 10 second scanned the patch and overlooked the test :)

The reverse matcher can be much more sane, even if there are multiple cases. My patch the ancient (still open) #2977 ticket covers most cases. But, like I said - yours is a simple fix and a reasonably common case.

comment:4 Changed 5 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by SmileyChris

  • Easy pickings unset
  • Resolution set to fixed
  • Status changed from new to closed
  • UI/UX unset

Fixed (3 years ago in r8760)

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