Opened 5 years ago

Closed 3 months ago

Last modified 3 months ago

#15053 closed New feature (fixed)

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

Reported by: pmartin Owned by: unaizalakain
Component: Template system Version: master
Severity: Normal Keywords:
Cc: andybak, kmike84@…, apollo13, 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 pmartin 5 years ago.
patch.15426.diff (5.9 KB) - added by pmartin 4 years ago.

Download all attachments as: .zip

Change History (62)

Changed 5 years ago by pmartin

comment:1 Changed 5 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

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 Changed 5 years ago by pmartin

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 Changed 5 years ago by lukeplant

  • Has patch unset
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to 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.

comment:4 Changed 5 years ago by pmartin

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

Thanks

Changed 4 years ago by pmartin

comment:5 Changed 4 years ago by pmartin

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

comment:6 Changed 4 years ago by andybak

  • Cc andybak added

comment:7 Changed 4 years ago by jezdez

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

comment:8 follow-up: Changed 4 years ago by jezdez

  • Patch needs improvement set

comment:9 Changed 4 years ago by anonymous

  • Severity set to Normal
  • Type set to New feature

comment:10 Changed 4 years ago by kmike

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

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

comment:12 in reply to: ↑ 8 Changed 4 years ago by pmartin

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 Changed 3 years ago by Wouter

Do something about this please!

comment:14 Changed 3 years ago by akaariai

  • 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 Changed 3 years ago by pmartin

Ok I will do, sorry for the delay

comment:16 Changed 3 years ago by pmartin

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 Changed 3 years ago by pmartin

I hope that you like it

comment:18 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:19 Changed 21 months ago by unaizalakain

  • Owner changed from nobody to unaizalakain
  • Status changed from new to assigned
  • Version changed from 1.2 to master

comment:20 Changed 21 months ago by pmartin

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 Changed 21 months ago by unaizalakain

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 Changed 21 months ago by unaizalakain

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

Any comment, suggestion or correction appreciated!

comment:23 Changed 21 months ago by unaizalakain

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:24 Changed 21 months ago by pmartin

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 Changed 21 months ago by unaizalakain

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 follow-up: Changed 21 months ago by pmartin

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

comment:27 in reply to: ↑ 26 Changed 21 months ago by pmartin

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 follow-up: Changed 21 months ago by unaizalakain

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

comment:30 in reply to: ↑ 29 Changed 21 months ago by pmartin

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 Changed 21 months ago by unaizalakain

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 Changed 20 months ago by unaizalakain

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 20 months ago by unaizalakain (previous) (diff)

comment:34 Changed 20 months ago by apollo13

  • Cc apollo13 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 Changed 19 months ago by mitar

  • 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 Changed 19 months ago by apollo13

@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 Changed 19 months ago by mitar

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

comment:38 Changed 19 months ago by apollo13

@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 Changed 13 months ago by 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.

comment:40 Changed 13 months ago by rm_

  • Cc riccardo.magliocchetti@… added

comment:41 Changed 12 months ago by julen

  • Cc julenx@… added

comment:42 Changed 12 months ago by 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

comment:43 Changed 12 months ago by 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.

comment:44 Changed 9 months ago by prestontimmons

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 Changed 9 months ago by prestontimmons

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 Changed 9 months ago by prestontimmons

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 Changed 9 months ago by prestontimmons

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 Changed 9 months ago by 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.

Last edited 9 months ago by rm_ (previous) (diff)

comment:49 Changed 9 months ago by prestontimmons

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 Changed 8 months ago by prestontimmons

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 Changed 8 months ago by prestontimmons

  • Patch needs improvement unset

comment:52 Changed 7 months ago by timgraham

  • Keywords 1.8-alpha added
  • Patch needs improvement set

Blocked on merging of multiple templates branch.

comment:53 Changed 7 months ago by timgraham

  • 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 Changed 6 months ago by prestontimmons

  • Patch needs improvement unset

comment:55 Changed 5 months ago by prestontimmons

  • Patch needs improvement set

comment:56 Changed 5 months ago by prestontimmons

  • Patch needs improvement unset

comment:57 Changed 4 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:58 Changed 3 months ago by Preston Timmons <prestontimmons@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In fc21471:

Fixed #15053 -- Enabled recursive template loading.

comment:59 Changed 3 months ago by 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.

comment:60 Changed 3 months ago by Preston Timmons <prestontimmons@…>

In 8ae04e76:

Added docs for new template loader api.

Refs #15053.

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