Opened 14 years ago

Closed 10 years ago

Last modified 4 years ago

#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:

This also works with others template loaders.

Attachments (2)

patch1.2.r15173 (1.6 KB ) - added by Pablo Martín 14 years ago.
patch.15426.diff (5.9 KB ) - added by Pablo Martín 14 years ago.

Download all attachments as: .zip

Change History (64)

by Pablo Martín, 14 years ago

Attachment: patch1.2.r15173 added

comment:1 by Luke Plant, 14 years ago

Resolution: duplicate
Status: newclosed

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

comment:2 by Pablo Martín, 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 Luke Plant, 14 years ago

Has patch: unset
Resolution: duplicate
Status: closedreopened
Triage Stage: UnreviewedAccepted

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.

comment:4 by Pablo Martín, 14 years ago

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

Thanks

by Pablo Martín, 14 years ago

Attachment: patch.15426.diff added

comment:5 by Pablo Martín, 14 years ago

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

comment:6 by Andy Baker, 14 years ago

Cc: Andy Baker added

comment:7 by Jannis Leidel, 14 years ago

Has patch: set
Needs tests: set
Summary: [patch] Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursionMake templates more reusable by Improving template loading algorithm to avoid extending infinite recursion

comment:8 by Jannis Leidel, 14 years ago

Patch needs improvement: set

comment:9 by anonymous, 14 years ago

Severity: Normal
Type: New feature

comment:10 by Mikhail Korobov, 13 years ago

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

comment:11 by ris, 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.

in reply to:  8 comment:12 by Pablo Martín, 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:13 by Wouter, 12 years ago

Do something about this please!

comment:14 by Anssi Kääriäinen, 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:15 by Pablo Martín, 12 years ago

Ok I will do, sorry for the delay

comment:16 by Pablo Martín, 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:17 by Pablo Martín, 12 years ago

I hope that you like it

comment:18 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:19 by Unai Zalakain, 11 years ago

Owner: changed from nobody to Unai Zalakain
Status: newassigned
Version: 1.2master

comment:20 by Pablo Martín, 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 Unai Zalakain, 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 Unai Zalakain, 11 years ago

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

Any comment, suggestion or correction appreciated!

comment:23 by Unai Zalakain, 11 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:24 by Pablo Martín, 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 Unai Zalakain, 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!

comment:26 by Pablo Martín, 11 years ago

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

in reply to:  26 comment:27 by Pablo Martín, 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:29 by Unai Zalakain, 11 years ago

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

in reply to:  29 comment:30 by Pablo Martín, 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 Unai Zalakain, 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 Unai Zalakain, 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 the select_template function for the cohesion.
  • Docs has been updated (only documented functions are get_template and skip_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.

Last edited 11 years ago by Unai Zalakain (previous) (diff)

comment:34 by Florian Apolloner, 11 years ago

Cc: Florian Apolloner 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 Mitar, 11 years ago

Cc: mmitar@… added

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.

comment:36 by Florian Apolloner, 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 Mitar, 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 Florian Apolloner, 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 rm_, 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 rm_, 10 years ago

Cc: riccardo.magliocchetti@… added

comment:41 by julen, 10 years ago

Cc: julenx@… added

comment:42 by rm_, 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 rm_, 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 Preston Timmons, 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 Preston Timmons, 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 Preston Timmons, 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 Preston Timmons, 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 rm_, 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.

Last edited 10 years ago by rm_ (previous) (diff)

comment:49 by Preston Timmons, 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 Preston Timmons, 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 Preston Timmons, 10 years ago

Patch needs improvement: unset

comment:52 by Tim Graham, 10 years ago

Keywords: 1.8-alpha added
Patch needs improvement: set

Blocked on merging of multiple templates branch.

comment:53 by Tim Graham, 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 Preston Timmons, 10 years ago

Patch needs improvement: unset

comment:55 by Preston Timmons, 10 years ago

Patch needs improvement: set

comment:56 by Preston Timmons, 10 years ago

Patch needs improvement: unset

comment:57 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:58 by Preston Timmons <prestontimmons@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In fc21471:

Fixed #15053 -- Enabled recursive template loading.

comment:59 by Preston Timmons <prestontimmons@…>, 10 years ago

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.

comment:60 by Preston Timmons <prestontimmons@…>, 10 years ago

In 8ae04e76:

Added docs for new template loader api.

Refs #15053.

comment:61 by Tim Graham <timograham@…>, 8 years ago

In 5d8da09:

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

Per deprecation timeline.

comment:62 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In c70cd2a9:

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

Note: See TracTickets for help on using tickets.
Back to Top