Opened 8 years ago

Closed 3 years 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: Unai Zalakain
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 Antti Kaihola 7 years ago.
patch: included templates throw a different exception
8116.diff (2.1 KB) - added by Rob Hudson 6 years ago.
Updated to work with 1.3alpha1
8116.2.diff (4.7 KB) - added by Rob Hudson 6 years ago.
Updated to work with 1.3alpha1. Now with passing tests.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 8 years ago by Matt McClanahan

Component: UncategorizedTemplate system
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 8 years ago by James Bennett

milestone: 1.0post-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 8 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 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

Changed 7 years ago by Antti Kaihola

patch: included templates throw a different exception

comment:5 Changed 7 years ago by Antti Kaihola

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 6 years ago by Rob Hudson

Attachment: 8116.diff added

Updated to work with 1.3alpha1

Changed 6 years ago by Rob Hudson

Attachment: 8116.2.diff added

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

comment:6 Changed 6 years ago by Luke Plant

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset
Needs documentation: set
Triage Stage: Design decision neededAccepted
UI/UX: unset

comment:8 Changed 5 years ago by Anton Strogonoff

Cc: anton@… added

comment:9 Changed 4 years ago by Remco Wendt

Owner: changed from nobody to Remco Wendt
Status: newassigned

comment:10 Changed 4 years ago by Remco Wendt

Keywords: sprint2013 added

comment:11 Changed 4 years ago by Remco Wendt

Resolution: invalid
Status: assignedclosed

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 4 years ago by Joeri Bekker

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 4 years ago by Michael P. Jung <michael.jung@…>

Resolution: invalid
Status: closednew

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 4 years ago by Florian Apolloner

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 3 years 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 3 years ago by Unai Zalakain

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 3 years ago by Unai Zalakain

Owner: changed from Remco Wendt to Unai Zalakain
Status: newassigned

comment:18 Changed 3 years ago by Unai Zalakain

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 3 years ago by jonathanslenders

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

comment:20 Changed 3 years ago by Unai Zalakain

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 3 years ago by jonathanslenders

Sure.

comment:22 Changed 3 years ago by Unai Zalakain

PR updated!

comment:23 Changed 3 years 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 3 years ago by Unai Zalakain

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 3 years 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 3 years ago by Unai Zalakain

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 3 years ago by jonathanslenders

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

comment:28 Changed 3 years ago by jonathanslenders

Triage Stage: AcceptedReady for checkin

comment:29 Changed 3 years ago by Unai Zalakain

Done! #21393

comment:30 Changed 3 years ago by Anssi Kääriäinen

Triage Stage: Ready for checkinAccepted

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 3 years ago by Unai Zalakain

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 3 years ago by chrismedrela

Resolution: invalid
Status: assignedclosed

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