Opened 8 years ago

Closed 3 months ago

Last modified 3 months ago

#5904 closed Cleanup/optimization (wontfix)

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

Reported by: SmileyChris 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 SmileyChris 8 years ago.

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by SmileyChris

comment:1 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

I'll be brash and promote straight to checkin

comment:2 Changed 8 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

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 7 years ago by Conrad Irwin

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 7 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 4 years ago by julien

  • Needs tests set
  • Severity set to Normal
  • Type set to 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 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:8 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:9 Changed 3 months ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed

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

comment:10 Changed 3 months ago by timgraham

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