Opened 3 years ago

Closed 2 years ago

#18169 closed Bug (fixed)

NoReverseMatch silenced in template Variable

Reported by: jasisz 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 jasisz 3 years ago.
18169.diff (989 bytes) - added by jasisz 3 years ago.
18169.2.diff (980 bytes) - added by jasisz 3 years ago.
181692.3.patch (3.1 KB) - added by d1ffuz0r 3 years ago.
#181692-NoReverseMatch-silenced.patch (3.1 KB) - added by regebro 3 years ago.
Just a very minor change

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by jasisz

comment:1 Changed 3 years ago by jasisz

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 3 years ago by jasisz

Changed 3 years ago by jasisz

comment:2 Changed 3 years ago by anonymous

  • Summary changed from Strange exception handling in templates to NoReverseMatch silenced in template Variable

comment:3 Changed 3 years ago by anonymous

  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:4 Changed 3 years ago by aaugustin

  • Has patch set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted

comment:5 Changed 3 years ago by d1ffuz0r

  • 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 3 years ago by d1ffuz0r

comment:6 Changed 3 years ago by regebro

  • Owner changed from nobody to regebro
  • Status changed from new to assigned

Changed 3 years ago by regebro

Just a very minor change

comment:7 Changed 3 years ago by regebro

  • Triage Stage changed from Accepted to Ready 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 3 years ago by regebro (previous) (diff)

comment:8 Changed 3 years ago by regebro

  • Keywords sprint2013 added

comment:9 Changed 2 years ago by akaariai

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In 369b6fab25b55ceb363ba2a8cb7e0f1a88ef8f8d:

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

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