Opened 12 years ago

Closed 11 years ago

#18169 closed Bug (fixed)

NoReverseMatch silenced in template Variable

Reported by: Szymon Teżewski Owned by: regebro
Component: Template system Version: 1.4
Severity: Normal Keywords: template exception sprint2013
Cc: d1fffuz0r@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Let's see some - maybe strange, but possible and working - template schema.

base.html:

{% block content %}
	{% block error_here %}{% endblock %}
{% endblock %}

and some template extending it:

{% extends "base.html" %}

{% block content %}
	content
	{{ block.super }}
{% endblock %}

{% block error_here %}
	error here
	{% url non_existing_url %}
{% endblock %}

I was really surprised that in this situation exception from url tag - NoReverseMatch - fails silently, and whole 'error_here' block just don't render.

But when the same faulty tag is directly in 'content' block - we see error page. We also see error page when we replace url tag f.e. with some invalid tag - error will be normally returned.

All this cases are shown in simple project (fresh django 1.4) which I attach.

Attachments (5)

project.tar.gz (3.5 KB ) - added by Szymon Teżewski 12 years ago.
18169.diff (989 bytes ) - added by Szymon Teżewski 12 years ago.
18169.2.diff (980 bytes ) - added by Szymon Teżewski 12 years ago.
181692.3.patch (3.1 KB ) - added by Roman Gladkov 11 years ago.
#181692-NoReverseMatch-silenced.patch (3.1 KB ) - added by regebro 11 years ago.
Just a very minor change

Download all attachments as: .zip

Change History (15)

by Szymon Teżewski, 12 years ago

Attachment: project.tar.gz added

comment:1 by Szymon Teżewski, 12 years ago

I found reason why it is acting like this. It is because NoReverseMatch exception still have silent_variable_failure = True and it's just not silenced because url tag is re-raising exception.

But in this case exception is passing also {{ block.super }} Variable, which silences exceptions with silent_variable_failure = True. Don't know how to better do it (or if it should be done at all), but I'm attaching small diff that re-raises NoReverseMatch exception while resolving Variable.

by Szymon Teżewski, 12 years ago

Attachment: 18169.diff added

by Szymon Teżewski, 12 years ago

Attachment: 18169.2.diff added

comment:2 by anonymous, 12 years ago

Summary: Strange exception handling in templatesNoReverseMatch silenced in template Variable

comment:3 by anonymous, 12 years ago

Triage Stage: UnreviewedDesign decision needed

I tried the attached code and I got the reported exception.

comment:4 by Aymeric Augustin, 12 years ago

Has patch: set
Needs tests: set
Triage Stage: Design decision neededAccepted

comment:5 by Roman Gladkov, 11 years ago

Cc: d1fffuz0r@… added
Needs tests: unset

Patch can't applied

(django)django|master⚡ ⇒ git apply 18169.2.patch                                    
error: patch failed: django/template/base.py:774                                  
error: django/template/base.py: patch does not apply 

I made some improvements and tests for this patch in attached file.

by Roman Gladkov, 11 years ago

Attachment: 181692.3.patch added

comment:6 by regebro, 11 years ago

Owner: changed from nobody to regebro
Status: newassigned

by regebro, 11 years ago

Just a very minor change

comment:7 by regebro, 11 years ago

Triage Stage: AcceptedReady for checkin

I tested the patch on current trunk and it worked.

When just reraising an exception you can do so implicitly, so I made that very minor change.

Last edited 11 years ago by regebro (previous) (diff)

comment:8 by regebro, 11 years ago

Keywords: sprint2013 added

comment:9 by Anssi Kääriäinen, 11 years ago

I don't think the patch is corrrect. It is special-casing NoReverseMatch in a place where such special casing isn't appropriate.

If I understand correctly the reason why NoReverseMatch doesn't cause errors when {% url %} fails is that the error isn't raised from variable lookup, it is raised from node rendering. {{block.super}} goes through variable lookup, and the variable lookup then silences the error.

The whole silent_variable_failure attribute in NoReverseMatch seems pointless. I don't know why we would want to silence NoReverseMatch error if it is risen in variable resolution context, but not if it is risen from {% url %}. Removing that attribute fixes this issue. All of the tests pass.

There might be some cases where the removal will cause backwards compatibility problems. For example if you have a method view_link() for some object, and that raises NoReverseMatch -> before you would get silent failure if doing {{ someobj.view_link }}, after NoReverseMatch. I am ready to call that a bugfix...

There is another issue, that is {{ block.super }} should not silence *any* errors, even if they happen to have silent_variable_failure. The reason is that if a silent_variable_failure exception gets to block.super, it wasn't from variable lookup to begin with (that is, block.super has already dealt with all exceptions in the proper way, there is no need to do that processing a second time.

See branch https://github.com/akaariai/django/tree/ticket_18169 for fixes. The first commit removes the silent_variable_failure from NoReverseMatch, the second one removes the silent_variable_failure from exceptions in {{block.super}}.

I think the first patch is ready for commit, the second one might need a separate ticket.

comment:10 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 369b6fab25b55ceb363ba2a8cb7e0f1a88ef8f8d:

Fixed #18169 -- NoReverseMatch not silenced if from block.super

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