Opened 9 years ago

Closed 18 months ago

Last modified 18 months ago

#5904 closed Cleanup/optimization (wontfix)

reverse should be more lenient on non-existent views in the urlconf

Reported by: Chris Beaven Owned by: nobody
Component: Core (URLs) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If you try to use the reverse (even for an existing view) while you have a non-existent view referenced in your urlconf, a ViewDoesNotExist error will be raised.

This happens because a reverse_dict cache is built for the RegexURLResolver containing a list to all pattern callbacks and names.

Rather than the whole thing falling over, all that should really happen in this case is that the callback is left out of reverse_dict.

Attachments (1)

reverse_bug.diff (901 bytes) - added by Chris Beaven 9 years ago.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by Chris Beaven

Attachment: reverse_bug.diff added

comment:1 Changed 9 years ago by Chris Beaven

Triage Stage: UnreviewedReady for checkin

I'll be brash and promote straight to checkin

comment:2 Changed 9 years ago by Malcolm Tredinnick

Resolution: wontfix
Status: newclosed

Firstly, not ready for checkin, since it doesn't include a test (which would have been one line long).

More importantly, this is covering up bugs in peoples' code. If you are referencing a view that doesn't exist, it's equivalent to having invalid syntax in your Python source. We shouldn't be continuing on in that case, regardless of whether the bug is nearby or just in a referenced file. Raising an exception is the right thing to do.

If somebody wants to make improvements in this area, come up with a way to report such errors back to the developer when the {% url %} tag is used (not via the template). Don't pretend the bug in their code doesn't exist.

comment:3 Changed 8 years ago by Conrad Irwin

Resolution: wontfix
Status: closedreopened

In the {% url %} tag, which also raises this error it is very annoying. The default for non existant variables in template syntax is to silently ignore the problem, this should also occur for views that don't exist yet (whether relevant or irrelevant).

Secondly it is bad that an error in a different application can cause an error in the current application, as they are supposed to be independent. There is no reason to raise this error ever, and particularly not for parts of the code that don't affect the result of the call.

comment:4 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

It's reasonable that the url tag should catch the error and do nothing -- that's normal template behaviour and not doing so is a bug. A patch to fix that would be good.

However, the behaviour of reverse() with respect to the error reporting it does now should be unchanged. It's an error to refer to invalid view files. Equivalent to having syntax errors in your code or randomly deleting files. There's simply no way to argue otherwise. You might hope for less draconian error handling, but that would be disguising symptoms. The correct fix is to fix the bugs in the code that causes the problem (it takes two seconds to comment out a line that isn't implemented yet, for example). So no change to reverse() behaviour is required.

comment:5 Changed 6 years ago by Julien Phalip

Needs tests: set
Severity: Normal
Type: Cleanup/optimization

#6379 was closed as dupe and has a patch. While I'm here: nothing is actually broken so I'll call this an optimization. Things have changed in the last 3 years though, so I'm not sure if the issue still exist.

comment:6 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:9 Changed 18 months ago by Tim Graham

Resolution: wontfix
Status: newclosed

By deprecating string views in urlpatterns, I don't think we'll have the problem of referencing non-existent views.

comment:10 Changed 18 months ago by Tim Graham

Component: Core (Other)Core (URLs)
Note: See TracTickets for help on using tickets.
Back to Top