#15053 closed New feature (fixed)
Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursion
Reported by: | Pablo Martín | Owned by: | Unai Zalakain |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Andy Baker, kmike84@…, Florian Apolloner, mmitar@…, riccardo.magliocchetti@…, julenx@… | 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
Many times when we use a django application we need to adapt it a little bit. Often this adaptation is just a little change in the html.
So we have to copy the application template into our project templates.
It would be awesome that the template loading algorithm let us to overwrite just the template blocks we want to change.
For example: If we use django profile and we want to modify the title of the login template from "User login" to "Login" we have to copy and paste 51 lines of code. If we want to add a new javascript resource we have to copy and paste 51 lines of code and so on.
But if we could adapt the template loading system so it could use the order of the loaders to avoid extending cycles, we could do the above example with a template (in our project directory) like this:
Template: userprofile/account/login.html
{% extends "userprofile/account/login.html"%} {% comment %} this doesn't extend itself but the application template {% endcomment %} {% load i18n%} {% block title%} {% trans "Login"%} {% endblock%}
TEMPLATE_LOADERS = ( 'django.template.loaders.filesystem.Loader', 'django.template.loaders.app_directories.Loader', # 'django.template.loaders.eggs.Loader', )
We have an implementation that works on Django 1.2 and Django 1.1. But if we set TEMPLATE_DEBUG = True in settings.py we need to patch Django:
- Django 1.2 (trunk r15173) attachment
- Django 1.2
- Django 1.1
This also works with others template loaders.
Attachments (2)
Change History (64)
by , 14 years ago
Attachment: | patch1.2.r15173 added |
---|
comment:1 by , 14 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
comment:2 by , 14 years ago
I think this ticket is set on duplicate when it should not be. The scope of this ticket is greater than the #8158 (the name of the ticket does not agree with the comments).
What do you think needs cleaning?
Thanks for you suggestions
comment:3 by , 14 years ago
Has patch: | unset |
---|---|
Resolution: | duplicate |
Status: | closed → reopened |
Triage Stage: | Unreviewed → Accepted |
OK, I will re-open this due to broader scope.
The first thing that needs to be improved is to actually supply a patch :-) In fact, I'm going to set 'Has patch' to false because the patch attached is just a patch to get your external project to work - the actual implementation is not included in the patch.
Once you do that, I think many things will have to change. As I said, it is hard to analyse because it isn't a patch.
(BTW, there is no chance that we will include the file patch1.2.r15173
on its own - it would make absolutely no sense without the external implementation you reference).
I'm marking 'Accepted' conditionally - it will depend on how nice the actual patch is whether this is included. We won't hack the template system to pieces just to get this to work.
by , 14 years ago
Attachment: | patch.15426.diff added |
---|
comment:5 by , 14 years ago
comment:6 by , 14 years ago
Cc: | added |
---|
comment:7 by , 14 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Summary: | [patch] Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursion → Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursion |
follow-up: 12 comment:8 by , 14 years ago
Patch needs improvement: | set |
---|
comment:9 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:10 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:11 by , 13 years ago
This feature would save me a lot of time and needlessly duplicated code. I think it would also encourage more modular template (and even application) design as applications could modify the templates of applications they depend on in slightly more bulletproof ways than currently exist.
Was about to open this as a separate bug until I found this.
comment:12 by , 13 years ago
Replying to jezdez:
I'm sorry. I don't see the "Patch needs improvement". I thought that this was not taken care of. But please, tell me. What improvements need to be?
The first, I think that I have to update the patch. Tell me something
comment:14 by , 12 years ago
Needs documentation: | set |
---|
To move this patch forward the patch would need some comments. Currently you need to do some reverse engineering to understand the patch. In addition this needs tests and possibly some doc changes along the lines of "When extending a template the currently used template is skipped. This allows to extend a template which has same path as the currently used template. This is useful when...".
I trust there is no use case for extending "self" currently - it leads always to infinite recursion and thus there is no backwards compatibility issues here. What if the extended template name is a variable? It probably is impossible to change that variable, and there is still infinite recursion.
I haven't done the reverse engineering mentioned above, so it might be the current approach as whole isn't correct.
comment:16 by , 12 years ago
Now works with the last version of django. I want to write comments, analize the code and I will try to clean the code. I coded this 18 month ago...
comment:18 by , 12 years ago
Status: | reopened → new |
---|
comment:19 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.2 → master |
comment:20 by , 11 years ago
Hi unaizalakain,
I am working in an external application to do it:
https://github.com/goinnn/django-smart-extends
I have to finish this work... I will finish it at December (I hope it).
Now I have a little error with python 2.6. But this app will work with django 1.1, 1.2, 1.3, 1.4 and 1.5. I have a branch to every version of django.
To move the code of this app to django repository is something very very easy... I could do it when I finish it.
I want this feature from 3 years ago... if you want this feature too please tell me. We can work together. I could do a pull request, and you update some details... And we could have this feature in master branch in January 2014
comment:21 by , 11 years ago
Hi!
I've already worked out, tested and more or less documented a working solution for this though I must improve the docs a bit more. I will look at your code but my solution is based on your legacy PR so there shouldn't be much difference. I'll make a PR today or tomorrow, I'd appreciate any comment or suggestion ;-)
comment:22 by , 11 years ago
PR sent: https://github.com/django/django/pull/1890
Any comment, suggestion or correction appreciated!
comment:23 by , 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:24 by , 11 years ago
Unai I improved the code of my pull request in my repo, if you want I could do a pull request to you the next weekend fixing some details. E.g.: Your pull request does not work with cache loader...
I had worked a lot of in this, and we could help us... but now I have not any time... :-(
Thanks you for your mention
comment:25 by , 11 years ago
Yep, it would be nice to work together! I read your code but found it a bit difficult to understand because of the support for different django versions and the app and the patch being coupled together. It would be really nice if you worked out a way for the change to work also with the cache loader and submitted it to my branch! Any other proposal is also welcome!
follow-up: 27 comment:26 by , 11 years ago
Ok I will try to do a pull request the 16th or 17th November.
comment:27 by , 11 years ago
Replying to pmartin:
Ok I will try to do a pull request the 16th or 17th November.
Done!
A promise is a promise
comment:30 by , 11 years ago
Replying to unaizalakain:
Sorry for the delay, suggestions noted at the PR ;)
Me too. This saturday I will update this PR. I really would like if we could to do a PR to django soon.
comment:31 by , 11 years ago
Before opening the PR we need to document and test all the loader related functions that we had changed exhaustively. I'll do it before this weekend.
comment:32 by , 11 years ago
I've recently been aware of some changes that could improve the API clearness so
I made an other commit on top of the last one (I'll squash them before issuing
the PR):
- Global template source loader loader is now outside of
find_template
making it easier to reuse it.
- I've also added the
skip_template
parameter to theselect_template
function for the cohesion.
- Docs has been updated (only documented functions are
get_template
andskip_template
.
Currently, the cache test fails (that's good), check that that they pass
correctly after your commit.
Let me know about any pitfall I made or any suggestion you have ;-)
I think the solution to this rather big issue is evolving correctly, we should
be able to issue a PR and address the django-dev mailing list asking for
comments in a short period of time.
comment:33 by , 11 years ago
PR sent: https://github.com/django/django/pull/2042
Mailing list discussion: https://groups.google.com/forum/#!topic/django-developers/0kFgCCMXnpY
comment:34 by , 11 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
I think this patch needs a bit of improvement:
- The errors are completely unintuitive: http://i.imgur.com/LQq65S5.png -- it can't find index.html although it obviously exists in the app-template loader
- Loaders shouldn't get skipped, just templates; the reasoning for that is simple: One of the most common usecases for this feature will be that I have the admin and some app extending the admin; both of them will be in INSTALLED_APPS but should be able to extend each other.
comment:35 by , 11 years ago
Cc: | added |
---|
I implemented also template recursion in this modified django-overextends:
It supports all possible combinations. For example, not just "base.html" extending "base.html", but also "user.hml" extending "base.html" which extends "base.html" (example).
In fact, it allows arbitrary nesting and combinations. It works by keeping a list of possible sources for each template and as template is rendered it removes it from the list, so that next time it is not loaded anymore.
Currently it is implemented as a custom tag and monkey-patches Django to provide template origin to tokens also when TEMPLATE_DEBUG
is
false. So #17199 would be needed.
comment:36 by , 11 years ago
@mitar: Fixing #17199 would be no problem if extends needs it; feature freeze is on 20.1, till then a patch which enables extend to perform like overextends could go in.
comment:37 by , 11 years ago
@apollo13: A question: my tag currently uses template context to trace each extended template loading paths. Is this OK?
comment:38 by , 11 years ago
@mitar: Preferably not, but I am not sure what our other options are -- also instead of constructing a list via app_directories you should go over the template loaders somehow.
comment:39 by , 10 years ago
So if the agreement is that overextends is the way to go and mitar does not have time to push this changes i can give it a try in the next weeks, I'd very much like to have a way to extends admin templates in 1.8.
comment:40 by , 10 years ago
Cc: | added |
---|
comment:41 by , 10 years ago
Cc: | added |
---|
comment:42 by , 10 years ago
Started playing with this: imported overextend changes and tests. overextends tests fails, added a FIXME wrt the loading failing with cache loader adding an hack to make django template_test pass, tree available here https://github.com/xrmx/django/tree/15053
comment:43 by , 10 years ago
Updated the branch: cleaned up overextend tests by extending the admin templates instead of creating random apps, fixed tests by adding template/base.py changes that were monkeypatched in overextends. Help on how to handle the FIXME appreciated.
comment:44 by , 10 years ago
I added a new pull request for this feature here:
https://github.com/django/django/pull/3475/
The approach I took is most similar to the one take by django-overextends, except this one doesn't unravel the cached loader.
Any feedback is appreciated.
comment:45 by , 10 years ago
I've added additional updates to my pull request. This now accounts for the eggs and cached loaders.
After attempting multiple ways to accomplish this, I'm convinced this feature can't happen without at least some changes to the template and loader apis.
In my current patch, I propose the following changes:
Update the template loaders
Add a get_sources
method to each loader. This replaces load_template_sources
. Instead of returning the first template found, it yields templates until no more are found.
Add a get_template
method to the base loader. This replaces load_template
. By default, this calls get_sources
and returns the first matching template. This takes an optional skip
argument. This is a list of paths to ignore. These paths are formatted as the full identifying path for the template. For filesystem and app loaders, this is the full path of the template.
Deprecate the load_template_sources
and load_template
apis. In the meantime, 3rd-party template loaders will work as usual.
Update the extend tag
In order to handle cases such as:
filesystem/base.html -> app1/base.html -> app2/base.html -> app3/base.html
a history of which templates have already been extended needs to be kept. The only place that has persistent access to this information is the ExtendsNode.
Update the ExtendsNode to track the history through a context variable and pass it as the skip
argument to loader.get_template.
Add template origin always, instead of only when TEMPLATE_DEBUG is false
By setting the origin, the ExtendsNode can know what the original template is, and avoid extending itself during a recursive sequence. Without this information, it's hard to avoid redundant recursion like:
app1/base.html -> app1/base.html -> app2/base.html -> app3/base.html
This piece of information isn't critical, but it is nice to have. Adding it in is not unprecedented. For instance, if we were using jinja2 templates, the template source path is readily available without needing a debug mode.
Update the cache loader
This one is problematic for allowing recursive includes. It currently assumes that a template name maps to only a single template, and keeps it stored in it's cache that way.
In the cached.Loader.get_template
method, I modified the cache to store the value as a list of template instead. This is calculated once when a new template is passed in.
I think this is reasonable but it could use some review.
comment:46 by , 10 years ago
I pushed some more updates to the patch.
The template loader api is now simpler and more efficient, thanks to apollo13's suggestions. I'm open to input if this can be simplified even more.
Loader API
get_template_sources
yields all paths the loader will look at for a given template name. This already existed for the filesystem and app loaders. They now also exist for the egg and cached loaders.
get_internal
takes a source argument as returned by get_template_source
and returns the template contents, or raises TemplateDoesNotExist
BaseLoader.get_template
iterates through sources and returns the first matching one, unless a skip argument is passed. The skip argument is a set of sources to ignore. If no template is found, TemplateDoesNotExist
is raised.
The next thing I want to look into is updating the template debug post-mortem message to accurately reflect which templates were skipped or not found. I would also like to improve the message when circular extending happens as well.
I checked with Aymeric. He agreed these updates are mostly orthogonal to his recent DEP. I think the main place these would overlap is if get_template
/select_template
is updated to also use the Loader.get_template
api instead of the current Loader.load_template
. I don't plan to propose any changes to those just yet.
comment:47 by , 10 years ago
I found some interesting behavior while working on the debug messages. Currently, the debug view is coupled to django.template.loader. To generate a post-mortem, it loops through the loaders and calls their get_template_sources api. This means there is no post-mortem output for the cached or egg loader. Further, the post-mortem status messages assumes the return values are filesystem paths.
I think we can do better than this. The information used in the post-mortem is already available in Loader.get_template. I'm going to see if we can pass that through instead as an attribute of TemplateDoesNotExist, rather than recalculating it. This would remove the coupling and fix things for the egg and cached loader.
comment:48 by , 10 years ago
@prestontimmons thanks a lot for your work! Would be nice to have all this high level description in the commit mesage so it is not lost.
comment:49 by , 10 years ago
I'm waiting for Aymeric's work, now. Once the template system is refactored as a library it will simplify the peripheral changes in this patch.
comment:50 by , 10 years ago
I've updated my pull request with Aymeric's latest changes.
It now includes integration with engine.get_template, the debug view, and docs.
comment:51 by , 10 years ago
Patch needs improvement: | unset |
---|
comment:52 by , 10 years ago
Keywords: | 1.8-alpha added |
---|---|
Patch needs improvement: | set |
Blocked on merging of multiple templates branch.
comment:53 by , 10 years ago
Keywords: | 1.8-alpha removed |
---|
Indications are that this will not make it into 1.8. From Preston on the PR, "There are some details still up in the air about the origin api. My changes will formalize this to an extent, but it will need further definition for multiple template engines. "
comment:54 by , 10 years ago
Patch needs improvement: | unset |
---|
comment:55 by , 10 years ago
Patch needs improvement: | set |
---|
comment:56 by , 10 years ago
Patch needs improvement: | unset |
---|
comment:57 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Basically, this is a duplicate of #8158. I agree that the best solution is to make the template finding process skip the current template. It may need to go deeper, to prevent recursion in the case of multiple level of extending/overriding. The implementation indicated is very hard to analyse, since it is not a patch, and looks like it would need significant work to make it clean enough for inclusion. (find_template should not know anything about the extends tag or the structure of template objects, for example).