Opened 7 years ago

Closed 19 months ago

#8116 closed Bug (invalid)

django.template.loader.select_template should not silently skip a template which includes another template that does not exist

Reported by: Michael P. Jung Owned by: unaizalakain
Component: Template system Version: master
Severity: Normal Keywords: sprint2013
Cc: mpjung@…, anton@…, FunkyBob Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using django.template.loader.select_template for a template that does exist, but contains an {% include ... %} tag which throws a TemplateDoesNotExist exception, the template is silently skipped instead of raising an error. That makes it extremely hard to track down an include error within a template that is loaded using select_template.

I haven't dug very deep into this issue, but as far as I got it seams that the most simple solution to this problem would be a different exception to be thrown by the include tag.

Attachments (3)

8116-r11637-IncludedTemplateDoesNotExist.diff (2.0 KB) - added by akaihola 6 years ago.
patch: included templates throw a different exception
8116.diff (2.1 KB) - added by robhudson 4 years ago.
Updated to work with 1.3alpha1
8116.2.diff (4.7 KB) - added by robhudson 4 years ago.
Updated to work with 1.3alpha1. Now with passing tests.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 7 years ago by mattmcc

  • Component changed from Uncategorized to Template system
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 7 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0

I'm punting this to post-1.0; it's a minor issue, an instance of a larger "problem" that's not specific to Django, and there are much more important things to do for the 1.0 release.

comment:3 Changed 7 years ago by Mike Amy

Ran into this one...

Problem is bad exception handling... TemplateDoesNotExist exceptions arising from the missing child templates are ignored in select_template, in effect causing it to ignore the parent template. A fix is to check the message in the exception (in django/template/loader.py ):

def select_template(template_name_list):
    "Given a list of template names, returns the first that can be loaded."
    for template_name in template_name_list:
        try:
            return get_template(template_name)
        except TemplateDoesNotExist, e:
            if e.message is template_name: # <<< Check the template is not a child template
                continue
            raise
    # If we get here, none of the templates could be loaded
    raise TemplateDoesNotExist, template_name_list

However, that is making assumptions about the exception, there ought to be a better way, like a template_name attribute in the exception

comment:4 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

Changed 6 years ago by akaihola

patch: included templates throw a different exception

comment:5 Changed 6 years ago by akaihola

The idea of the patch above is that all TemplateDoesNotExist exceptions encountered while compiling a template get changed to IncludedTemplateDoesNotExist exceptions, which are then checked for in select_template().

This is kind of what Michael P. Jung suggested, except it's handled at the loader level instead of in the {% include %} tag.
Therefore, I think this will catch sub-templates loaded with whatever mechanism, not only with {% include %}.

Also, in case of a missing included template, the exception displayed is:

IncludedTemplateDoesNotExist: template_path/template_name.html

Does this make sense, or would Make Amy's patch or his suggestion for a template_name exception attribute be a better approach? Some kind of a fix would be nice since the current behavior can be really confusing.

Changed 4 years ago by robhudson

Updated to work with 1.3alpha1

Changed 4 years ago by robhudson

Updated to work with 1.3alpha1. Now with passing tests.

comment:6 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 3 years ago by aaugustin

  • Easy pickings unset
  • Needs documentation set
  • Triage Stage changed from Design decision needed to Accepted
  • UI/UX unset

comment:8 Changed 3 years ago by zuko

  • Cc anton@… added

comment:9 Changed 2 years ago by shanx

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

comment:10 Changed 2 years ago by shanx

  • Keywords sprint2013 added

comment:11 Changed 2 years ago by shanx

  • Resolution set to invalid
  • Status changed from assigned to closed

Closing this as invalid, the current behavior is well tested and the expected behavior. If you want template tags to raise errors then one should set TEMPLATE_DEBUG to True (even though DEBUG is False) this will correctly lead to a 500 error. Otherwise template errors fail silently. See table below for what happens in each situation.

comment:12 Changed 2 years ago by joeri

DEBUG TEMPLATE_DEBUG Comments
True True Django raises a TemplateDoesNotExist in the debug screen.
True False Django silently ignores the missing template.
False True Django raises a TemplateDoesNotExist resulting in a simple HTTP 500 error.
False False Django silently ignores the missing template.

I concur with shanx that this is the expected behaviour.

comment:13 Changed 2 years ago by Michael P. Jung <michael.jung@…>

  • Resolution invalid deleted
  • Status changed from closed to new

I have to disagree. This behavior is neither expected nor well documented.

It is about an existing template that includes another template which does not exist.

If the template does exist but raises a TemplateDoesNotExist due to {% include "does_not_exist.html" %} the error should be raised and not silently ignored. A template which includes a missing template is definitely an error in the code and nothing I would call "expected behavior".

Even if it was well documented it would be highly counter intuitive. I doubt that anybody uses broken templates on purpose and expects them to be silently skipped by select_template.

comment:14 Changed 2 years ago by apollo13

I agree with Michael here, if select_template finds a template it has to use it, if it can't use it due to errors it should not be allowed to continue silently!

comment:15 Changed 22 months ago by FunkyBob

  • Cc FunkyBob added

It looks like this could be related to #3544.

Oddly enough, the fix refers to [optionally] allowing {% include %} to "quietly" fail if the template doesn't exist.

However, as it no longer tries to load the included template at parse time, it won't raise an error then.

comment:16 Changed 20 months ago by unaizalakain

I would rather add a template_name attribute to the TemplateDoesNotExist class. This attribute could be used not only in this case but also in normal TemplateDoesNotExist exception.

comment:17 Changed 20 months ago by unaizalakain

  • Owner changed from shanx to unaizalakain
  • Status changed from new to assigned

comment:18 Changed 20 months ago by unaizalakain

  • Has patch set
  • Needs documentation unset

PR with the IncludedTemplateDoesNotExist exception, passing tests and documentation: https://github.com/django/django/pull/1795

comment:19 Changed 19 months ago by jonathanslenders

Can't we inherit IncludedTemplateDoesNotExist from TemplateDoesNotExist? That way, we don't have to change existing unit tests.

comment:20 Changed 19 months ago by unaizalakain

Good point, I would inherit from TemplateDoesNotExist but I still would check for IncludedTemplateDoesNotExist in tests, checking for specific exception is better than checking for a broader one.

comment:21 Changed 19 months ago by jonathanslenders

Sure.

comment:22 Changed 19 months ago by unaizalakain

PR updated!

comment:23 Changed 19 months ago by jonathanslenders

Sorry, one more remark. You should actually do the same in ExtendsNode.render. There we have exactly the same issue. (There are also tests for "extends nonexistent" which needs to be updated.)

Maybe you should rename the exception to something like ReferencedTemplateDoesNotExist.

comment:24 Changed 19 months ago by unaizalakain

I'm not sure about what to do. The problem that this ticket was addressing is that no exception is raised when a included template does not exist. In this case, the solution has been to create a new exception so that this one isn't captured.

The way things are now, a TemplateDoesNotExist exception is raised if an extended template doesn't exist. Though it could make sense to be more explicit about this and raise a ReferencedTemplateDoesNotExist, if we use that instead of the IncludedTemplateDoesNotExist for included templates too, we aren't being less explicit.

A solution could be to create two new exceptions: ExtendedTemplateDoesNotExist and IncludedTemplateDoesNotExist. Maybe it's a bit of overkill, I don't know.

comment:25 Changed 19 months ago by jonathanslenders

Maybe I'm wrong, but I don't think it's really overkill in this case. Maybe even all of them:

class ReferencedTemplateDoesNotExist(TemplateDoesNotExist):
    pass

class ExtendedTemplateDoesNotExist(ReferencedTemplateDoesNotExist):
    pass

class IncludedTemplateDoesNotExist(ReferencedTemplateDoesNotExist):
    pass

comment:26 Changed 19 months ago by unaizalakain

It could be, but I think the best thing to do is to open an other ticket about being more explicit with the exception raised if an extended template doesn't exist and leave these ticket for the include tag thing. Because, after all, they're two different discussions. Any thoughts? Will you open the ticket or do I?

comment:27 Changed 19 months ago by jonathanslenders

Okay, can you open a new ticket?
I'll put this in ready for checkin.

comment:28 Changed 19 months ago by jonathanslenders

  • Triage Stage changed from Accepted to Ready for checkin

comment:29 Changed 19 months ago by unaizalakain

Done! #21393

comment:30 Changed 19 months ago by akaariai

  • Triage Stage changed from Ready for checkin to Accepted

How is the select_template() issue resolved by the patch? The select_template() method still catches TemplateDoesNotExist, and thus IncludedTemplateDoesNotExist. Am I missing something?

I don't think the patch actually solves this ticket's issue. It does look good to me otherwise.

comment:31 Changed 19 months ago by unaizalakain

Uh, you are quite right, I don't know how I worked on it so blindly!!

The thing is that select_template doesn't even render the templates so there isn't any chance of handling the inexistence of included templates. The included template is rendered when the master template is rendered and there, there isn't any issue with TemplateDoesNotExist being ignored. I think this issue could be from a time when instantiating a Template would automatically render it (dunno).

I would like somebody else to confirm it but I would close this issue as invalid.

Thanks for making sense of all this and stopping it before it was merged!

comment:32 Changed 19 months ago by chrismedrela

  • Resolution set to invalid
  • Status changed from assigned to closed

I confirm -- IncludeNode doesn't raise TemplateDoesNotExist in its __init__ since https://github.com/django/django/commit/e2f06226ea4a38377cdb69f2f5768e4e00c2d88e.

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