Code

Opened 3 years ago

Closed 8 months ago

#16147 closed Bug (fixed)

{% include %} tag raises TemplateDoesNotExist at compile time if TEMPLATE_DEBUG is True

Reported by: mrmachine Owned by: aaugustin
Component: Template system Version: master
Severity: Normal Keywords: include template TemplateDoesNotExist TEMPLATE_DEBUG
Cc: prestontimmons@…, anton@…, FunkyBob 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

I think this is a bug. Or at the very least, it can make for unexpected behaviour that is difficult to trace. When developing locally, DEBUG and TEMPLATE_DEBUG are often True. When TEMPLATE_DEBUG is True, compiling a template that contains an {% include %} to a template that doesn't exist will raise TemplateDoesNotExist at compile time of the parent template.

>>> from django.template import Template
>>> t = Template('{% include "missing.html" %}')
Traceback (most recent call last):
...
TemplateDoesNotExist: missing.html

The problem is that when you try to conditionally load a template, and perform a different action if it does not exist. For example, you might have a view that uses select_template to load one of several templates (if one exists) and send an email. If no email was sent (because no template exists), render a different template/context to the response or take some other action. If the email template contains a typo in the name of an included template, the email template will not be loaded and no email will be sent because TemplateDoesNotExist was raised.

In the local dev environment, this will not display a pretty debug page showing the error inside the email template, it will simply behave unexpectedly (by not sending an email when the email template DOES exist). On the production server (or when TEMPLATE_DEBUG is False), it will behave as expected. The email template will be found and it will be rendered.

This problem doesn't exist when using a variable as the template name.

>>> t = Template('{% include somevar %}')
>>> 

Why does Django try to execute the {% include %} when it contains a hard coded string at compile time at all? I think this is a case of premature optimisation. If it execution of the {% include %} was delayed until render time, select_template would correctly return the email template when TEMPLATE_DEBUG is True, and the proper error would be displayed in the rendered email template (making it an easy fix to correct the typo).

Attachments (2)

16147.patch (1.1 KB) - added by aaugustin 3 years ago.
16147.r17335.diff (10.4 KB) - added by prestontimmons 2 years ago.
Updated patch @r17335

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by aaugustin

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

Replying to:

Why does Django try to execute the {% include %} when it contains a hard coded string at compile time at all?


This feature (or bug) has "always" existed. I couldn't find an explanation in the code or the commit messages.

  • It was added at r1349, which introduced the {% include %} tag.
  • It was moved from django/core/template/loader.py to django/template/loader_tags.py at r1443, and not changed since then.

Both changes were committed by Adrian in Nov '05.

Hopefully the discussion on the mailing list will help sort it out: https://groups.google.com/group/django-developers/browse_thread/thread/f1c2e3664c8aace2


Technically, the problem is the implementation of django.template.loader_tags.ConstantIncludeNode: the template resolution with get_template should be moved from __init__ to render.
I'm attaching a patch that fixes it with the minimum amount of changes, just to show the idea. However:

  • after the patch, ConstantIncludeNode and IncludeNode are very similar; some refactoring would be useful,
  • it breaks the test suite.

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

The original reporter found the explanation for this optimization and posted it on IRC:
https://code.djangoproject.com/ticket/598#comment:2

comment:3 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

Changed 2 years ago by prestontimmons

Updated patch @r17335

comment:4 Changed 2 years ago by prestontimmons

  • Needs tests unset
  • Patch needs improvement unset
  • UI/UX unset
  • Version changed from 1.3 to 1.4-alpha-1

I added an updated patch with tests.

It's a comprehensive patch that replaces BaseIncludeNode, ConstantIncludeNode, and IncludeNode with a single implementation.

I also rewrote the tests in a form I find readable, but kept the existing tests in place so a reviewer can verify backwards compatibility. Besides that, the old tests are redundant.

In addition to this ticket, this patch addresses #12064 and #3544.

comment:5 Changed 2 years ago by prestontimmons

  • Cc prestontimmons@… added

comment:6 Changed 2 years ago by aaugustin

  • Summary changed from {% include %} tag raises TemplateDoesNotExist at compile time of parent template if TEMPLATE_DEBUG is True to {% include %} tag raises TemplateDoesNotExist at compile time if TEMPLATE_DEBUG is True

Shortening title because it degrades the layout of Trac :)

comment:7 Changed 2 years ago by zuko

  • Cc anton@… added

comment:8 Changed 18 months ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:9 Changed 13 months ago by prestontimmons

Updated the patch and added a pull request for review.

https://github.com/django/django/pull/920

comment:10 Changed 13 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:11 Changed 8 months ago by FunkyBob

  • Cc FunkyBob added

I've made a PR for a simpler patch for this:

https://github.com/django/django/pull/1528

comment:12 Changed 8 months ago by loic84

  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.4-alpha-1 to master

comment:13 Changed 8 months ago by Anssi Kääriäinen <akaariai@…>

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

In e2f06226ea4a38377cdb69f2f5768e4e00c2d88e:

Improved {% include %} implementation

Merged BaseIncludeNode, ConstantIncludeNode and Include node.

This avoids raising TemplateDoesNotExist at parsing time, allows recursion
when passing a literal template name, and should make TEMPLATE_DEBUG behavior
consistant.

Thanks loic84 for help with the tests.

Fixed #3544, fixed #12064, fixed #16147

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.