Opened 5 years ago

Closed 8 months ago

#12995 closed New feature (fixed)

"source" exception attribute no longer handled properly by debug exception handler

Reported by: Glenn Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The "source" attribute on exceptions handled by the debug view was very handy; I set it in my own custom templating system (which coexists with Django's) and it automatically showed up in errors generated by Django's, complete with line numbers, just like they do for when Django templates set that attribute. Duck typing made this work implicitly, which was extremely useful--that's how duck typing is supposed to work.

r12586 broke this, adding an isinstance check which makes it so only errors from Django's own templates can use this feature. This doesn't make sense; duck typing was the correct approach here.

Please revert r12586; that was an unnecessarily destructive change. If you're having namespace collisions, then use a less generic name ("source_with_lines" or something like that).

Change History (13)

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.1 to SVN

This change isn't going to be reverted wholesale. Also, how is changing to the attribute "source_with_lines" any less backwards incompatible for your usecase? I am accepting this ticket as custom templating systems should have a way of providing debug info.

comment:2 Changed 5 years ago by Glenn

I don't care about backwards-compatibility here; I care about being able to do this at all. I can change my source attribute to eg. "source_with_lines" without significant problems, but I can't make all of my exceptions derive from TemplateSyntaxError. (It applies the source attribute to the existing exception and re-throws it without changing the exception type, simply augmenting the exception in-place as it passes out of the template. IMO, this is what Django's templating should be doing, too, to avoid destroying the original exception.)

comment:3 Changed 5 years ago by russellm

If you read up on the history of the original bug, the change to avoid the source attribute was done specifically to support non-Django template languages (in particular Jinja2), and to avoid clashes with other libraries (like PyXML) that use a source attribute but have nothing to do with the templating system.

So - we're not going to simply revert here.

However, I agree that a simple type check isn't ideal -- [and I've http://groups.google.com/group/django-developers/browse_thread/thread/2a1901c961847e06 said as much on django-dev]. Patches implementing a more sophisticated approach to template exception handling are welcome.

comment:4 Changed 5 years ago by Glenn

That's exactly why I suggested renaming the attribute to a less generic name than "source". I don't see any problems with "source_with_lines" (other than sounding a little awkward). If you're still concerned about namespace collisions--it's inherent with duck typing--try "django_source".

comment:5 follow-up: Changed 5 years ago by ubernostrum

Is this a duplicate of #12992?

comment:6 in reply to: ↑ 5 Changed 5 years ago by kmtracey

Replying to ubernostrum:

Is this a duplicate of #12992?

No. That one's about the file name not being known with the new loaders (from r11862). This one is about the source lines only being shown if the exception raised is a Django TemplateSyntaxError (due to r12586), breaking debugging for other templating systems.

comment:7 Changed 5 years ago by mitsuhiko

I think the correct solution would be adding a hook into the DebugView to inject your own code there.

comment:8 Changed 5 years ago by Glenn

Storing source information in the exception is cleaner. It means that any templating engine supporting the interface can supply source context to any exception renderer supporting it, without the user code sitting between the two needing to manually ferry the information across with a hook. (Not to suggest that the interface should be made public as it is; it should also allow getting template source for each stack frame.)

Another reason (the original reason I noticed this) is discussed in ticket #11461. DebugNodeList shouldn't be destroying the original exception to attach information; it should simply be attaching the data to the existing exception. r12586 makes cleanly fixing that impossible.

comment:9 Changed 4 years ago by lukeplant

  • Type set to New feature

comment:10 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 8 months ago by aaugustin

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

This was fixed in [4397c587]. See also #16770.

django_template_source was chosen as a "less generic name".

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