Fix {% include %} to allow recursive includes

[David Danier <goliath.mailinglist@…>]

If you try to do recursive includes (for example to display hierarchical data) the include-tag will fail, because it trys to parse the template when loading (at least ConstantIncludeNode does). This can be fixed by saving the loaded templates and avoid multiple loading. Besides this should improve performance, if templates get included multiple times inside a request. Perhaps it would be best to include some form of caching in django.template.loader.get_template?

I have created a simple RecursiveIncludeNode, which caches the loaded templates. The drawback is, that I think the cache stays in RAM over multiple requests (should be no real problem, only if very much templates are used). Also this combines ConstantIncludeNode and IncludeNode (constant templates get cached when parsing, variable templates get cached inside render()).

But I think it would be best to put caching inside the get_template()-function of django (perhaps as a special loader?), as this could improve performance (and is no real drawback).

The Code:

class RecursiveIncludeNode(Node):
	cache = {}
	@staticmethod
	def _get_template(template_name):
		if not RecursiveIncludeNode._has_template(template_name):
			RecursiveIncludeNode._load_template(template_name)
		return RecursiveIncludeNode.cache[template_name]
	@staticmethod
	def _has_template(template_name):
		return RecursiveIncludeNode.cache.has_key(template_name)
	@staticmethod
	def _load_template(template_name):
		RecursiveIncludeNode.cache[template_name] = None
		try:
			RecursiveIncludeNode.cache[template_name] = get_template(template_name)
		except:
			del RecursiveIncludeNode.cache[template_name]
			raise
	def __init__(self, template_name):
		self.template_name = template_name
	def render(self, context):
		try:
			template_name = resolve_variable(self.template_name, context)
			t = RecursiveIncludeNode._get_template(template_name)
			return t.render(context)
		except TemplateSyntaxError, e:
			if settings.TEMPLATE_DEBUG:
				raise
			return ''
		except:
			return '' # Fail silently for invalid included templates.

def rec_include(parser, token):
	bits = token.contents.split()
	if len(bits) != 2:
		raise TemplateSyntaxError, "%r tag takes one argument: the name of the template to be included" % bits[0]
	path = bits[1]
	from django.template.loader_tags import IncludeNode
	if path[0] in ('"', "'") and path[-1] == path[0]:
		template_name = path[1:-1]
		if not RecursiveIncludeNode._has_template(template_name):
			RecursiveIncludeNode._load_template(template_name)
	return RecursiveIncludeNode(bits[1])

register.tag('rec_include', rec_include)

[Michael Radziej <mir@…>]

I checked 'patch needs improvement' because we prefer a patch created by diff as attachment (see contribution docs).

[David Danier <goliath.mailinglist@…>]

Thats appropriate. The submitted code is not really intended to be a patch, I would call it "solution code example", but this cannot be selected when submitting tickets. ;-)
(As a side-effect it shows how such a cache could be used to merge the two Include-classes)

Besides the example has one drawback I didn't think of when writing it: The cache will survive multiple requests. If you change the template it will not get loaded again. "Fixed" this by clearing the cache on the "request_finished" and "got_request_exception"-signal.
If someone wants to use it, even though it's a hack (perhaps as a workaround, like I do):

from django.core import signals
from django.dispatch import dispatcher

def clear_template_include_cache():
	RecursiveIncludeNode.cache = {}

dispatcher.connect(clear_template_include_cache, signal=signals.request_finished)
dispatcher.connect(clear_template_include_cache, signal=signals.got_request_exception)

As I'm not into the template-code I'm sure there is some better way to implement this.

[Flavio Curella <flavio.curella@…>]

attached snippets from Daniel Davier (including cleaning cache by signals) as .diff

[Flavio Curella <flavio.curella@…>]

patch updated with new variable resolution

[Jacob]

Can you explain why you want/need recursive includes? A use case would go a long way towards helping me figure out how to handle this ticket.

[David Danier <goliath.mailinglist@…>]

I use this for a tree-view of comments, as used here:
​http://www.webmasterpro.de/coding/article/mehrere-checkboxen-mit-einer-de-bzw-aktivieren.html#comments

Each template includes {% include "display-comments.html" %}, which is the file itself.

Current code runs into endless loop.

[Johannes Dollinger]

3544.contexttemplatecache.diff does the following:

• Removes ConstantIncludeNode. {% include %} is always lazy.
• Adds Template caching to Context (templates will not be cached between requests)

Cons: makes {% include %} lazy (?)

Pros: makes {% include %} lazy and therefore allows recursive includes. The following will not load debug.html if DEBUG is False:
{% if DEBUG %}{% include "debug.html" %}{% endif %}

Todo: cleanup, docs+tests, inclusion_tag should use this.

[Johannes Dollinger]

test for 3544.contexttemplatecache.diff

[Johannes Dollinger]

3544.contexttemplatecache.2.diff:

• Adds a template_cache to Context, accessible via get_template(name) and select_template(name_list).
• Removes ConstantIncludeNode, {% include %} always uses Context.get_template(). This fixes #3544.
• Makes ExtendsNode and inclusion_tag use Context.get_template().
• Adds an optional kwarg loader to Context, which should be a templateloader (something with a get_template(name) method). This fixes #2949.
• Alters {% with %} to accept multiple assignments: {% with foo as bar and x as y %} or {% with foo as bar, x as y %} (untestet, but normal tests pass).

Planned: I'd like to enable {% include "foo.html" with bar as foo, x as y %}, which would be equivalent to {% with ... %}{% include ... %}{% endwith %}.

Optional: Since {% include %} only needs something with a render(context) method it would work with nodelists. So it would be trivial to add something like

{% sub "my_sub" %}
    {{ foo }}
{% endsub %}

{% include "my_sub" with somevar as foo %}

[goliath.mailinglist@…>]

Patch sounds great, would like to see this in Django before 1.0 ;-)

[Johannes Dollinger]

typo ..

[Johannes Dollinger]

should have checked this "replace" checkbox ..

• {% include "foo" with value as name %} syntax
• separator_chars kwarg for smart_split
• tests for smart_split and extended {% with %} and {% include %} syntax
• attempt to fix docstrings ..

[Johannes Dollinger]

Since the patch already exceeds the scope of this ticket and I'm not going to make split patches that will be rotten by the time 1.0 is out (and there's a chance to get a design decision again) I'll just keep all my django.template changes at ​http://einfallsreich.net/projekte/thenewstuff/django/ until then.

The current version contains the (merged) latest patches for #7076 and #5756.

[Johannes Dollinger]

Latest patch is at #7815.

[Erik Allik]

What exactly is keeping the diff from being added to trunk? And meanwhile, is there a workaround for displaying recursive data structures? inclusion tags?

[Malcolm Tredinnick]

Accepted in principle, since recursive includes would be useful. We'll have to check things work well with 1.2 template changes. Patch will no doubt need refreshing/rewriting. The common path (non-self-recursive includes) should not be impacted (slowed down) at all by allowing recursive includes.

So, providing no adverse side-effects to existing users, this is something to be implemented.

[Rob Hudson]

I may be missing a lot about the goal, but recursive template inclusion is already possible, though perhaps not as efficient as would be nice? e.g. by setting {% with "comments.html" as template %}, you can trigger the use of IncludeNode instead of ConstantIncludeNode when calling {% include template %} inside the with block.

For a more spelled out example, see: ​http://blog.elsdoerfer.name/2008/01/22/recursion-in-django-templates/

[twoolie]

this should definitely be revisited. recursive includes are still sorely needed without with hacks.

p.s. i would LOVE include tags to get the new with notation patch. This is a seriously good thing, especially because almost every time you do an include, you probably need a with anyway.

[alex_dev]

to David Danier:

just let you know: you can implement recursion by means of django template tags.
Here is example of my ​implementation.

P.S.:
I think the idea of 'include' tag to allow recursive load is bad: it can outcome in unobvious errors.

[David Danier <david.danier@…>]

The current implementation does it right I think, it loads the template on compile time to check for syntax errors. This should definately stay, as this is a cool feature.

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

[Łukasz Rekucki]

Replying to David Danier <david.danier@team23.de>:

The current implementation does it right I think, it loads the template on compile time to check for syntax errors. This should definately stay, as this is a cool feature.

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

Isn't that what "django.template.loaders.cached.Loader" does ?

[twoolie]

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

This is obviously beneficial

Isn't that what "django.template.loaders.cached.Loader" does ?

Not really, the caching system returns Template objects, whereas if the include tag doesn't need to touch the entire template discovery stack every time, that would be awesome. an include tag local cache only costs one reference per included template so why not? if it speeds up include performance AND allows recursive imports then why not?
also, embedded with syntax is insanely cool

[David Danier <david.danier@…>]

Needed this again and decided, that you currently can use

{% with template_name="file/to_include.html" %}{% include template_name %}{% endwith %}

as an easy workaround.

[Camilo Nova]

We should include this patch or document the workaround, it is needed when you want to render information in a tree format, like the comments

[Preston Timmons]

I added a patch to #16147.

It also addresses the problem of this patch, and lets includes happen recursively.

[anonymous]

Any progress?

It seems quite demanded feature as soon as you deal with hierarchical trees rendering.

[Preston Timmons]

Updated the patch and added a pull request for review.

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

[FunkyBob]

Whilst I agree we no longer need the separate classes, as we have the caching template loader to cover the performance gain, I do question the need for the "quiet" flag.

Under what circumstances do you seen someone wanting to do an include on a template that may not exist?

[FunkyBob]

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

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

[Anssi Kääriäinen <akaariai@…>]

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