Opened 8 years ago

Closed 4 years ago

#5831 closed Bug (fixed)

Template Debug highlights wrong {% for %} tag

Reported by: Charmless <jim-django@…> Owned by: vladmos
Component: Template system Version: master
Severity: Normal Keywords: debug for toplevel nesting
Cc: m@… 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

When an exception happens within a for tag's head, the error source is assigned always assigned to the top-level for loop, even if the erroneous for tag is nested.

Example:

{% for a in as %}
  {% for b in a.bs %}
    {% for c in b.relatedobject_set %}
       {{ c.foo }}
    {% endfor %}
  {% endfor %}
{% endfor %}

The error here is at "b.relatedobject_set", which should be "b.relatedobject_set.all" (assuming b is a model related to a model releatedobject).

As is, the template debug system marks the top level for tag, "{% for a in as %}", as being the point of error. The correct place to mark is the for tag where the error happened.

Attached is a patch that fixes this. I'm not familiar with Django's tag tests, so I can't include any, but would be happy to learn and supply some if it is agreed that this is a correct fix.

Attachments (2)

forloop.nodelist.patch (1.6 KB) - added by Charmless <jim-django@…> 8 years ago.
patch against SVN revision 6626
forloop.nodelist.2.patch (5.2 KB) - added by vladmos 4 years ago.
Fixed typo in a comment

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by Charmless <jim-django@…>

patch against SVN revision 6626

comment:1 Changed 8 years ago by Charmless <jim-django@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Template Debug only highlights wrong {% for %} tag to Template Debug highlights wrong {% for %} tag

comment:2 Changed 7 years ago by gwilson

#6468 marked as a dup. It also has a template example that shows the bug.

comment:3 Changed 7 years ago by gwilson

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 7 years ago by charmless

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

comment:6 Changed 7 years ago by kmtracey

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:7 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 4 years ago by julien

  • Easy pickings unset
  • Needs tests set
  • Patch needs improvement set

Confirming that this issue is still occurring. Needs tests to take this further. The patch also needs improvement as the ForNode's code has changed since 4 years ago.

comment:9 Changed 4 years ago by vladmos

  • Owner changed from charmless to vladmos
  • Status changed from reopened to new
  • UI/UX unset

comment:10 Changed 4 years ago by vladmos

  • Needs tests unset
  • Patch needs improvement unset

django.template.defaulttags.ForNode.render method calls .render method for subnodes and there loses the information which exactly node caused the error, that's why DebugNodeList always marked the first forloop of nested as which caused the error.

I've made a patch which adds source of the the node that actually caused the error as the exception's attribute, which is later intercepted by DebugNodeList.render method.

Tests are included.

comment:11 Changed 4 years ago by vladmos

Chanded order if and try..except statements. Now try..except is performed only when settings.TEMPLATE_DEBUG is true, so it shouldn't slower rendering with debugger disabled.

comment:12 Changed 4 years ago by vladmos

Chanded order if and try..except statements. Now try..except is performed only when settings.TEMPLATE_DEBUG is true, so it shouldn't slower rendering with debugger disabled.

Changed 4 years ago by vladmos

Fixed typo in a comment

comment:13 Changed 4 years ago by vladmos

  • Cc m@… added

comment:14 Changed 4 years ago by lrekucki

  • Triage Stage changed from Accepted to Ready for checkin

comment:15 Changed 4 years ago by jezdez

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

In [16605]:

Fixed #5831 -- Made sure the ForNode reports the correct source of an exception happening in one of the loops. Thanks, Charmless and vladmos.

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