Opened 6 years ago

Closed 2 years ago

Last modified 2 years ago

#12064 closed New feature (fixed)

Impossible to conditionally include potentially non-existant templates

Reported by: mkruisselbrink Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords:
Cc: 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

It is not possible to conditionally include one template in another template, if the template to include might not exist. The include tag will raise an exception if the template does not exist (and is specified as a string literal, not a variable) even if the actual include tag will never be rendered because a surrounding if statement has a false condition. This can easily be fixed by moving the get_template call from ConstantIncludeNode.__init__ to it's render method.

Attachments (1)

includefix.diff (791 bytes) - added by mkruisselbrink 6 years ago.
patch that fixes this bug

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by mkruisselbrink

  • Component changed from Uncategorized to Template system
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 6 years ago by mkruisselbrink

patch that fixes this bug

comment:2 Changed 6 years ago by mkruisselbrink

  • Has patch set

comment:3 Changed 6 years ago by lukeplant

  • Needs tests set
  • Owner changed from nobody to lukeplant
  • Status changed from new to assigned

It should be quite easy to produce a test for this bug - no exception should be raised for a template like this:

   {% if some_var_that_equals_False %}
      {% include "some_template_that_does_not_exist.html" %}
   {% endif %}

That test is needed before the patch can go in.

comment:4 Changed 6 years ago by anonymous

see also: #7931

comment:5 Changed 6 years ago by lukeplant

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

comment:6 Changed 6 years ago by SmileyChris

The proper fix, assuming this is deemed a valid bug, would be to remove ConstantIncludeNode altogether, not make it work near identically to IncludeNode.

comment:7 Changed 6 years ago by russellm

  • Triage Stage changed from Unreviewed to Design decision needed

I think this is somewhat related to #12008 - it all hinges on the question of how exactly an {% include %} should be processed -- is it a parsing-level include, or is it a tag that is only evaluated if and when the IncludeNode is rendered?

comment:8 Changed 5 years ago by mk

Ticket #3544 is related too (Fix {% include %} to allow recursive includes) and could be solved at the same time.

comment:9 Changed 4 years ago by baumer1122

  • Severity set to Normal
  • Type set to New feature

comment:10 Changed 4 years ago by mrmachine

  • Easy pickings unset

See also #16147

I believe that the behaviour of {% include %} as it stands is both inconsistent between development (TEMPLATE_DEBUG = True) and production environments, but also within the tag itself depending on the type of argument passed to it. These inconsistencies have shown themselves in a few different buggy (or at least unexpected) ways that are not always easy to trace.

If including a non-existent template is worth raising TemplateDoesNotExist in a development environment, it should not fail silently in a production environment. The exception should also be raised there and redirected to admins via Django's logging.

If including a non-existent template should fail silently when the template does not exist, it should do so both for includes that have a literal string argument or a variable, and it should do so both in development and production environments.

I don't really have a firm opinion on whether or not including missing templates should always fail silently or loudly, although I believe the most common and expected behaviour currently is to fail silently so there may be backwards incompatible issues in changing that. But I do believe that the {% include %} tag should work it's magic at render time for both literal and variable arguments. I consider the currently (undocumented) difference in behaviour to be buggy or at the very least, surprising.

comment:11 Changed 4 years ago by akaihola

  • UI/UX unset

I'd very much like to see this applied and am willing to contribute to make it happen. We're relying on the silent failure in production, but it's a hassle to comment out the includes when developing in debug mode.

I see that the triage stage for both #12008 and #3544 is "Accepted". Would one or both of those fix this issue as well? #16147 is "Design decision needed". Should I just write tests for this ticket as lukeplant suggested, or should I direct my efforts at one of the related tickets?

Version 0, edited 4 years ago by akaihola (next)

comment:12 Changed 4 years ago by prestontimmons

I added a patch to #16147.

It also addresses the problem of this patch, and lets non-existent templates be conditionally included, even when TEMPLATE_DEBUG is true.

comment:13 Changed 2 years ago by prestontimmons

  • Needs tests unset

Updated the patch and added a pull request for review.

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

comment:14 Changed 2 years ago by jacob

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

This is basically a dup of #16147, especially now that there's a patch addressing 'em both at once.

comment:15 Changed 2 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:16 Changed 2 years ago by loic84

  • Triage Stage changed from Design decision needed to Ready for checkin
  • Version changed from 1.1 to master

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

  • Resolution changed from duplicate to fixed

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