Opened 17 years ago

Closed 11 years ago

#3544 closed New feature (fixed)

Fix {% include %} to allow recursive includes

Reported by: David Danier <goliath.mailinglist@…> Owned by: nobody
Component: Template system Version: dev
Severity: Normal Keywords: tplrf-patched
Cc: flavio.curella@…, Erik Allik, david@…, Mike Fogel, newmaniese@…, cvrebert, 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

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)

Attachments (7)

rec_include.diff (3.6 KB ) - added by Flavio Curella <flavio.curella@…> 17 years ago.
rec_include-r6399.diff (3.2 KB ) - added by Flavio Curella <flavio.curella@…> 16 years ago.
patch updated with new variable resolution
3544.contexttemplatecache.diff (2.6 KB ) - added by Johannes Dollinger 16 years ago.
3544.recursiveincludetest.diff (1.6 KB ) - added by Johannes Dollinger 16 years ago.
test for 3544.contexttemplatecache.diff
3544.contexttemplatecache.2.diff (10.5 KB ) - added by Johannes Dollinger 16 years ago.
3544.contexttemplatecache.3.diff (19.4 KB ) - added by Johannes Dollinger 16 years ago.
3544.contexttemplatecache.3.2.diff (19.4 KB ) - added by Johannes Dollinger 16 years ago.
typo ..

Download all attachments as: .zip

Change History (43)

comment:1 by Michael Radziej <mir@…>, 17 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

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

comment:2 by David Danier <goliath.mailinglist@…>, 17 years ago

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.

comment:3 by (removed), 17 years ago

Cc: ferringb@… added

comment:4 by Flavio Curella <flavio.curella@…>, 17 years ago

Cc: flavio.curella@… added

by Flavio Curella <flavio.curella@…>, 17 years ago

Attachment: rec_include.diff added

comment:5 by Flavio Curella <flavio.curella@…>, 17 years ago

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

by Flavio Curella <flavio.curella@…>, 16 years ago

Attachment: rec_include-r6399.diff added

patch updated with new variable resolution

comment:6 by Jacob, 16 years ago

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.

comment:7 by David Danier <goliath.mailinglist@…>, 16 years ago

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.

by Johannes Dollinger, 16 years ago

comment:8 by Johannes Dollinger, 16 years ago

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.

by Johannes Dollinger, 16 years ago

test for 3544.contexttemplatecache.diff

by Johannes Dollinger, 16 years ago

comment:9 by Johannes Dollinger, 16 years ago

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 %}

comment:10 by goliath.mailinglist@…>, 16 years ago

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

by Johannes Dollinger, 16 years ago

by Johannes Dollinger, 16 years ago

typo ..

comment:11 by Johannes Dollinger, 16 years ago

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 ..

comment:12 by Johannes Dollinger, 16 years ago

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.

comment:13 by Johannes Dollinger, 16 years ago

Keywords: tplrf-patched added

Latest patch is at #7815.

comment:14 by (removed), 14 years ago

Cc: ferringb@… removed

comment:15 by Erik Allik, 14 years ago

Cc: Erik Allik added

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

comment:16 by Malcolm Tredinnick, 14 years ago

Has patch: unset
Triage Stage: Design decision neededAccepted

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.

comment:17 by Rob Hudson, 14 years ago

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/

comment:18 by twoolie, 13 years ago

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.

comment:19 by alex_dev, 13 years ago

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.

comment:20 by David Danier <david.danier@…>, 13 years ago

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.

in reply to:  20 comment:21 by Łukasz Rekucki, 13 years ago

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 ?

in reply to:  20 comment:22 by twoolie, 13 years ago

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

comment:23 by David Greaves, 13 years ago

Cc: david@… added

comment:24 by Mike Fogel, 13 years ago

Cc: Mike Fogel added

comment:25 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

comment:26 by Michael Newman, 13 years ago

Cc: newmaniese@… added
Easy pickings: unset
UI/UX: unset

comment:27 by David Danier <david.danier@…>, 13 years ago

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.

comment:28 by Camilo Nova, 12 years ago

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

comment:29 by Preston Timmons, 12 years ago

I added a patch to #16147.

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

comment:30 by cvrebert, 12 years ago

Cc: cvrebert added

comment:31 by anonymous, 11 years ago

Any progress?

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

comment:32 by Preston Timmons, 11 years ago

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Updated the patch and added a pull request for review.

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

comment:33 by FunkyBob, 11 years ago

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?

comment:34 by FunkyBob, 11 years ago

Cc: FunkyBob added

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

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

comment:35 by loic84, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:36 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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