Opened 5 years ago

Closed 4 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 5 years ago.
18169.diff (989 bytes) - added by Szymon Teżewski 5 years ago.
18169.2.diff (980 bytes) - added by Szymon Teżewski 5 years ago.
181692.3.patch (3.1 KB) - added by Roman Gladkov 4 years ago.
#181692-NoReverseMatch-silenced.patch (3.1 KB) - added by regebro 4 years ago.
Just a very minor change

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by Szymon Teżewski

Attachment: project.tar.gz added

comment:1 Changed 5 years ago by Szymon Teżewski

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.

Changed 5 years ago by Szymon Teżewski

Attachment: 18169.diff added

Changed 5 years ago by Szymon Teżewski

Attachment: 18169.2.diff added

comment:2 Changed 5 years ago by anonymous

Summary: Strange exception handling in templatesNoReverseMatch silenced in template Variable

comment:3 Changed 4 years ago by anonymous

Triage Stage: UnreviewedDesign decision needed

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

comment:4 Changed 4 years ago by Aymeric Augustin

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

comment:5 Changed 4 years ago by Roman Gladkov

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.

Changed 4 years ago by Roman Gladkov

Attachment: 181692.3.patch added

comment:6 Changed 4 years ago by regebro

Owner: changed from nobody to regebro
Status: newassigned

Changed 4 years ago by regebro

Just a very minor change

comment:7 Changed 4 years ago by regebro

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 4 years ago by regebro (previous) (diff)

comment:8 Changed 4 years ago by regebro

Keywords: sprint2013 added

comment:9 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

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