Opened 6 years ago

Closed 3 years ago

#16147 closed Bug (fixed)

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

Reported by: Tai Lee Owned by: Aymeric Augustin
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 Aymeric Augustin 6 years ago.
16147.r17335.diff (10.4 KB) - added by Preston Timmons 5 years ago.
Updated patch @r17335

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by Aymeric Augustin

Has patch: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign 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 6 years ago by Aymeric Augustin

Attachment: 16147.patch added

comment:2 Changed 6 years ago by Aymeric Augustin

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 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

Changed 5 years ago by Preston Timmons

Attachment: 16147.r17335.diff added

Updated patch @r17335

comment:4 Changed 5 years ago by Preston Timmons

Needs tests: unset
Patch needs improvement: unset
UI/UX: unset
Version: 1.31.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 5 years ago by Preston Timmons

Cc: prestontimmons@… added

comment:6 Changed 5 years ago by Aymeric Augustin

Summary: {% include %} tag raises TemplateDoesNotExist at compile time of parent template if TEMPLATE_DEBUG is True{% include %} tag raises TemplateDoesNotExist at compile time if TEMPLATE_DEBUG is True

Shortening title because it degrades the layout of Trac :)

comment:7 Changed 5 years ago by Anton Strogonoff

Cc: anton@… added

comment:8 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:9 Changed 4 years ago by Preston Timmons

Updated the patch and added a pull request for review.

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

comment:10 Changed 4 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:11 Changed 3 years 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 3 years ago by loic84

Triage Stage: AcceptedReady for checkin
Version: 1.4-alpha-1master

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

Resolution: fixed
Status: newclosed

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

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