Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursion

[Pablo Martín]

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.

[Luke Plant]

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

[Pablo Martín]

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

[Luke Plant]

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.

[Pablo Martín]

Ok, I need some time, but I will upload a nice patch.

Thanks

[Pablo Martín]

Sorry for the delay. Here is the patch​ to the revision 15426 of trunk. I hope you like it.

[ris]

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.

[Pablo Martín]

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

[Wouter]

Do something about this please!

[Anssi Kääriäinen]

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.

[Pablo Martín]

Ok I will do, sorry for the delay

[Pablo Martín]

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

[Pablo Martín]

I hope that you like ​it

[Pablo Martín]

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

[Unai Zalakain]

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 ;-)

[Unai Zalakain]

PR sent: ​https://github.com/django/django/pull/1890

Any comment, suggestion or correction appreciated!

[Pablo Martín]

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

[Unai Zalakain]

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!

[Pablo Martín]

Ok I will try to do a pull request the 16th or 17th November.

[Pablo Martín]

Replying to pmartin:

Ok I will try to do a pull request the 16th or 17th November.

Done!

• ​https://github.com/unaizalakain/django/pull/1
• ​https://github.com/unaizalakain/django/pull/2

A promise is a promise

[Pablo Martín]

​https://github.com/unaizalakain/django/pull/3

[Unai Zalakain]

Sorry for the delay, suggestions noted at the PR ;)

[Pablo Martín]

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.

[Unai Zalakain]

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.

[Unai Zalakain]

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.

• The cache loader needs to be able to specify the loaders to try in find_template. So we appended just an other parameter to find_template making them to: skip_template and loaders.

I noticed that the only thing that skip_template is useful for is for
skipping loaders. So we could squash those two parameters into a single one:
loaders. If that parameter isn't given the global loaders are loaded.

The loader skipping could be done outside find_template: in
get_template so this last function is the one that now gets the template
to skip.

• I've also added the skip_template parameter to the select_template function for the cohesion.

• Docs has been updated (only documented functions are get_template and skip_template.

@goinnn As a consequence of all this changes, you must first filter the cached
loaders through skip_loaders and then pass them to find_template. This
is an extra line of code but I think it enhances clarity. Before I forget it,
we must update the docs that refer to loaders and tell about the new
never_skip and use_skip_template attributes (I'd be glad to help with
that if needed).

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.

[Unai Zalakain]

PR sent: ​https://github.com/django/django/pull/2042
Mailing list discussion: ​https://groups.google.com/forum/#!topic/django-developers/0kFgCCMXnpY

[Florian Apolloner]

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.

[Mitar]

I implemented also template recursion in this modified django-overextends:

​https://github.com/wlanslovenija/django-overextends/blob/nested-extends/overextends/templatetags/overextends_tags.py

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.

[Florian Apolloner]

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

[Mitar]

@apollo13: A question: my tag currently uses template context to trace each extended template loading paths. Is this OK?

[Florian Apolloner]

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

[rm_]

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.

[rm_]

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

[rm_]

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.

[Preston Timmons]

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.

[Preston Timmons]

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.

[Preston Timmons]

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.

[Preston Timmons]

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.

[rm_]

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

[Preston Timmons]

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.

[Preston Timmons]

I've updated my pull request with Aymeric's latest changes.

It now includes integration with engine.get_template, the debug view, and docs.

[Tim Graham]

Blocked on merging of multiple templates branch.

[Tim Graham]

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

[Preston Timmons <prestontimmons@…>]

In fc21471:

Fixed #15053 -- Enabled recursive template loading.

[Preston Timmons <prestontimmons@…>]

In 65a7a0d:

Improved display of template loader postmortem on debug page.

This now works for multiple Django engines and recursive loaders.
Support for non-Django engines is still pending.

Refs #15053.

[Preston Timmons <prestontimmons@…>]

In 8ae04e76:

Added docs for new template loader api.

Refs #15053.

[Tim Graham <timograham@…>]

In 5d8da09:

Refs #15053 -- Removed support for non-recursive template loading.

Per deprecation timeline.

[Mariusz Felisiak <felisiak.mariusz@…>]

In c70cd2a9:

Refs #15053 -- Clarified debug message when skipping templates to avoid recursion.