Opened 18 years ago
Closed 11 years ago
#3544 closed New feature (fixed)
Fix {% include %} to allow recursive includes
Reported by: | 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)
Change History (43)
comment:1 by , 18 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 18 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 , 18 years ago
Cc: | added |
---|
comment:4 by , 17 years ago
Cc: | added |
---|
by , 17 years ago
Attachment: | rec_include.diff added |
---|
comment:5 by , 17 years ago
attached snippets from Daniel Davier (including cleaning cache by signals) as .diff
by , 17 years ago
Attachment: | rec_include-r6399.diff added |
---|
patch updated with new variable resolution
comment:6 by , 17 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 , 17 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 , 17 years ago
Attachment: | 3544.contexttemplatecache.diff added |
---|
comment:8 by , 17 years ago
3544.contexttemplatecache.diff does the following:
- Removes
ConstantIncludeNode
.{% include %}
is always lazy. - Adds
Template
caching toContext
(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 , 16 years ago
Attachment: | 3544.recursiveincludetest.diff added |
---|
test for 3544.contexttemplatecache.diff
by , 16 years ago
Attachment: | 3544.contexttemplatecache.2.diff added |
---|
comment:9 by , 16 years ago
3544.contexttemplatecache.2.diff:
- Adds a
template_cache
toContext
, accessible viaget_template(name)
andselect_template(name_list)
. - Removes
ConstantIncludeNode
,{% include %}
always usesContext.get_template()
. This fixes #3544. - Makes
ExtendsNode
andinclusion_tag
useContext.get_template()
. - Adds an optional kwarg
loader
toContext
, which should be a templateloader (something with aget_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 %}
by , 16 years ago
Attachment: | 3544.contexttemplatecache.3.diff added |
---|
comment:11 by , 16 years ago
should have checked this "replace" checkbox ..
{% include "foo" with value as name %}
syntaxseparator_chars
kwarg forsmart_split
- tests for
smart_split
and extended{% with %}
and{% include %}
syntax - attempt to fix docstrings ..
comment:12 by , 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:14 by , 15 years ago
Cc: | removed |
---|
comment:15 by , 15 years ago
Cc: | 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 , 14 years ago
Has patch: | unset |
---|---|
Triage Stage: | Design decision needed → Accepted |
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 , 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 , 14 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 , 14 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.
follow-ups: 21 22 comment:20 by , 14 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.
comment:21 by , 14 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 ?
comment:22 by , 14 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 , 14 years ago
Cc: | added |
---|
comment:24 by , 14 years ago
Cc: | added |
---|
comment:25 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:26 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:27 by , 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 , 13 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 , 13 years ago
I added a patch to #16147.
It also addresses the problem of this patch, and lets includes happen recursively.
comment:30 by , 12 years ago
Cc: | added |
---|
comment:31 by , 12 years ago
Any progress?
It seems quite demanded feature as soon as you deal with hierarchical trees rendering.
comment:32 by , 12 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.
comment:33 by , 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:35 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:36 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I checked 'patch needs improvement' because we prefer a patch created by diff as attachment (see contribution docs).