Code

Opened 3 years ago

Last modified 4 months ago

#15053 assigned New feature

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@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 3 years ago.
patch.15426.diff (5.9 KB) - added by pmartin 3 years ago.

Download all attachments as: .zip

Change History (40)

Changed 3 years ago by pmartin

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

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

Thanks

Changed 3 years ago by pmartin

comment:5 Changed 3 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 3 years ago by andybak

  • Cc andybak added

comment:7 Changed 3 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 3 years ago by jezdez

  • Patch needs improvement set

comment:9 Changed 3 years ago by anonymous

  • Severity set to Normal
  • Type set to New feature

comment:10 Changed 3 years ago by kmike

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

comment:11 Changed 3 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 2 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 2 years ago by Wouter

Do something about this please!

comment:14 Changed 2 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 22 months ago by pmartin

Ok I will do, sorry for the delay

comment:16 Changed 21 months 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 21 months ago by pmartin

I hope that you like it

comment:18 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:19 Changed 5 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 5 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 5 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 5 months ago by unaizalakain

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

Any comment, suggestion or correction appreciated!

comment:23 Changed 5 months ago by unaizalakain

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

comment:24 Changed 5 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 5 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 5 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 5 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 5 months ago by unaizalakain

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

comment:30 in reply to: ↑ 29 Changed 5 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 5 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 5 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 4 months ago by unaizalakain (previous) (diff)

comment:34 Changed 4 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 4 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 4 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 4 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 4 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from unaizalakain to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.