Opened 17 years ago

Closed 10 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@…> 16 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 15 years ago.
test for 3544.contexttemplatecache.diff
3544.contexttemplatecache.2.diff (10.5 KB) - added by Johannes Dollinger 15 years ago.
3544.contexttemplatecache.3.diff (19.4 KB) - added by Johannes Dollinger 15 years ago.
3544.contexttemplatecache.3.2.diff (19.4 KB) - added by Johannes Dollinger 15 years ago.
typo ..

Download all attachments as: .zip

Change History (43)

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

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 Changed 17 years ago by 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.

comment:3 Changed 17 years ago by (removed)

Cc: ferringb@… added

comment:4 Changed 16 years ago by Flavio Curella <flavio.curella@…>

Cc: flavio.curella@… added

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

Attachment: rec_include.diff added

comment:5 Changed 16 years ago by Flavio Curella <flavio.curella@…>

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

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

Attachment: rec_include-r6399.diff added

patch updated with new variable resolution

comment:6 Changed 16 years ago by 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.

comment:7 Changed 16 years ago by 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.

Changed 16 years ago by Johannes Dollinger

comment:8 Changed 16 years ago by 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.

Changed 15 years ago by Johannes Dollinger

test for 3544.contexttemplatecache.diff

Changed 15 years ago by Johannes Dollinger

comment:9 Changed 15 years ago by 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 %}

comment:10 Changed 15 years ago by goliath.mailinglist@…>

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

Changed 15 years ago by Johannes Dollinger

Changed 15 years ago by Johannes Dollinger

typo ..

comment:11 Changed 15 years ago by 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 ..

comment:12 Changed 15 years ago by 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.

comment:13 Changed 15 years ago by Johannes Dollinger

Keywords: tplrf-patched added

Latest patch is at #7815.

comment:14 Changed 14 years ago by (removed)

Cc: ferringb@… removed

comment:15 Changed 14 years ago by Erik Allik

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 Changed 13 years ago by Malcolm Tredinnick

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 Changed 13 years ago by 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/

comment:18 Changed 13 years ago by 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.

comment:19 Changed 13 years ago by 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.

comment:20 Changed 13 years ago by 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.

comment:21 in reply to:  20 Changed 13 years ago by Ł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 ?

comment:22 in reply to:  20 Changed 13 years ago by 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

comment:23 Changed 13 years ago by David Greaves

Cc: david@… added

comment:24 Changed 13 years ago by Mike Fogel

Cc: Mike Fogel added

comment:25 Changed 13 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:26 Changed 12 years ago by Michael Newman

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

comment:27 Changed 12 years ago by 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.

comment:28 Changed 12 years ago by 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

comment:29 Changed 12 years ago by Preston Timmons

I added a patch to #16147.

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

comment:30 Changed 11 years ago by cvrebert

Cc: cvrebert added

comment:31 Changed 11 years ago by anonymous

Any progress?

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

comment:32 Changed 11 years ago by Preston Timmons

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 Changed 10 years ago by 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?

comment:34 Changed 10 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:35 Changed 10 years ago by loic84

Triage Stage: AcceptedReady for checkin

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

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