Self-referenced template recursion handling

[unai]

Hi!

Pablo Martín (goinnn[0]) and I have been working on self-referenced template
recursion handling for the past few weeks. The idea is that when a template
references itself when extending, recursion is avoided by skipping the current
template loader (ticket #15053[1]).

This new feature would be totally backwards compatible, have no performance
penalties[2] (measured with djangobench) and could, in my opinion, be one of the
main features of the 1.7 release:

Lot of app/CMS creators create base templates for their apps. Currently, if one
of those templates needs some kind of change, the user needs to copy the
template all over again, making it difficult to update their templates when
they update their apps (they would need to copy the new template and then make
their customizations again).

Skipping the current template loader means that they would be able to create
their own template (with the same path as the app template), extend it to
itself and only change the blocks that need to be changed. For example:

App loader: some_app/item_list.html

{% block title %}Item list{% endblock title %}

{% block content %}
{% for item in items %}
...
{% endfor %}
{% endblock content %}


Filesystem loader: some_app/item_list.html

{% extends 'some_app/item_list.html' %}

{% block title%}Custom title!{% endblock title %}

This feature has also the added benefit of opening the doors to self-reference
handling inside app and egg loaders. We are already giving some thought to this
idea.

We are aware that this is not a small change in the template engine so reviews
and suggestions are highly appreciated. The pull request can be found at:
https://github.com/django/django/pull/2042

Greetings,

Unai Zalakain

References
----------

[0] https://github.com/goinnn/
[1] https://code.djangoproject.com/ticket/15053
[2] Benchmark:

Running benchmarks: template_compilation template_render template_render_simple
Control: Django 1.7.dev20131205221130 (in git branch master)
Experiment: Django 1.7.dev20131205224944 (in git branch ticket_15053)

Running 'template_compilation' benchmark ...
Min: 0.000716 -> 0.000733: 1.0240x slower
Avg: 0.000981 -> 0.000996: 1.0150x slower
Not significant
Stddev: 0.00245 -> 0.00245: 1.0002x smaller (N = 100)

Running 'template_render' benchmark ...
Min: 0.014449 -> 0.014756: 1.0213x slower
Avg: 0.015204 -> 0.015336: 1.0087x slower
Not significant
Stddev: 0.00296 -> 0.00298: 1.0053x larger (N = 100)

Running 'template_render_simple' benchmark ...
Min: 0.000135 -> 0.000134: 1.0071x faster
Avg: 0.000385 -> 0.000385: 1.0016x faster
Not significant
Stddev: 0.00245 -> 0.00245: 1.0021x smaller (N = 100)

[German Larrain]

On Friday, December 6, 2013 11:36:00 AM UTC-3, unai wrote:

Lot of app/CMS creators create base templates for their apps. Currently, if one
of those templates needs some kind of change, the user needs to copy the
template all over again, making it difficult to update their templates when
they update their apps (they would need to copy the new template and then make
their customizations again).

Skipping the current template loader means that they would be able to create
their own template (with the same path as the app template), extend it to
itself and only change the blocks that need to be changed.

 I would love this feature because I've faced the mentioned problems.

Germán

[J. Pablo Martín Cobos]

2013/12/6 German Larrain <germanl...@gmail.com>

I really like it this feature. I have used this feature from 3 years ago, through an external app [1], in a lot of projects and never I had any problem with it. The more relevant project have been the Malaga University website [2].

Without this feature looks like that Django despises the template code, because If you want to update a little thing of a reusable app (django.contrib.admin, django-mptt, django-tables2 etc) in your project (The most common usecase for this feature) you have to overwrite the complete template [3]. With this feature you will be able to do something like the before example, you will be able to overwrite only some blocks (or one block) of a template.

REF's

1. https://pypi.python.org/pypi/django-smart-extends/

2. http://www.uma.es/

3. http://www.agnostic-library.com/ma/wp-content/uploads/2012/01/Dont-Copy-Paste.png

 

Best regards,

--

Pablo Martín

[Florian Apolloner]

Hi,

there is no need to convince us that this feature would be nice to have; the ticket is accepted… I left a comment on the ticket page; and I think we should do this in one patch instead of two, so we can get the API right (Especially since I am not sure if skipping the loaders is the correct approach).

Cheers,
Florian

[Jonathan Slenders]

Do we ever intend to implement something like collect_templates in the future? Similar to collect_static?
If so, implementing this would break collect_templates or the similarity that we currently have in how app directories are processed.
This is actually the monkey-patching way of writing templates.

Probably a weak argument. I'm still against, but practically beats purity.

[J. Pablo Martín Cobos]

2013/12/7 Florian Apolloner <f.apo...@gmail.com>

Hi,

there is no need to convince us that this feature would be nice to have; the ticket is accepted…

Yes this ticket is accepted, but this is accepted from 3 years ago...  I think that everybody knows that the current solution needs to improve in the self-referenced case. But Unai and me want to convince you of this solution is right.

 

I left a comment on the ticket page; and I think we should do this in one patch instead of two,

There are two patches because the cache template loader is "very special", Unai asked in the irc channel if we could do two commits for this reason and they said that yes. But for me this is a minor question, although I would like don't have to change it.

 

so we can get the API right (Especially since I am not sure if skipping the loaders is the correct approach).

This subject is very hard to understand, but when you understand it you will be able to see that this solution is a great and the best solution. This solution only skip a template when the template extends itself. This subject is very hard to explain and understand, but the solution is very very simple. If you want I could send you an example project with a typical use case.

[J. Pablo Martín Cobos]

2013/12/8 Jonathan Slenders <jonathan...@gmail.com>

Do we ever intend to implement something like collect_templates in the future? Similar to collect_static?

I think that for this reason exists the cache template loader, it is something similar. With this template loader every template is only compiled one time.

https://docs.djangoproject.com/en/dev/ref/templates/api/#django.template.loaders.cached.Loader

 But I think it is another subject.

 

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d7060744-95c5-464c-a5bd-1a7d38b30432%40googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Pantelis Petridis]

Hello,

given this approach, what if the third party app wants to self-extend a django admin template for example?

The application should provide a separate template directory  and force users load it in their TEMPLATE_DIRS. Then what happens if the developer wants to self-extend that third party app's template? He can't use the filesystem loader because it will be ignored, but using the app loader wouldn't result in an infinite loop? (the last template is found by the app_loader -given the installed_apps order is correct-, then the third-party app's template is found which in turn leads back to the app loader and tries to override template for which we started).

What I'm trying to say is that maybe this approach means you're stuck with one self-extension per app-loader. Sorry if this thought is wrong, it's just my initial reaction.

Pantelis

--
Pantelis Petridis
Software Architect / Engineer
e-mail: ppe...@yawd.eu
url: Yawd web applications

[unai]

Hello,

> given this approach, what if the third party app wants to self-extend
> a django admin template for example?

I'm working on an other solution that instead of relying on loader skipping
relies on template skipping.

Imagine you extend to a self-reference from within a template. All the
templates are ignored until the very same template is skipped and it then
continues normally.

This allows for apps to extend other apps and for filesystem templates to
extend other TEMPLATE_DIR roots while order is respected.

Now, the tricky part is to identify a template uniquely. I went for hashing
but, as Apollo13 said on IRC, that's just too expensive. After looking for a
while, it seems that both filesystem and app loaders work with absolute paths.
It seems to me the best way to identify a template uniquely.

The beauty of the solution is that template skipping is totally relegated to
app and filesystem loaders. Of course, some changes in django.template.loader
are needed but they are minor.

I'm currently having some debugging issues and I'm quite busy with other things
but I'll try to provide a complete PR this following week.

If you can think of any problems that this solution would arise don't hesitate
in telling ;)


Best wishes,

Unai Zalakain

[Alex Hill]

Hi all,

If anybody's interested in some background reading, there is an implementation of this in Mezzanine, using an "overextends" tag.

https://github.com/stephenmcd/mezzanine/blob/1.4.16/mezzanine/template/loader_tags.py

Cheers,
Alex

[Curtis Maloney]

I tend to warn people off thinking of template names as paths. It's an implementation detail that they are treated as such by the two default loaders.

Case in point, my gnocchi-cms app is not the only one I've encountered where template names are used to look up a table for the contents.

Requiring each loader to include an unique id with each template (even if it's just a full path) seems the easiest solution.

Older 3rd party loaders which don't do this would not be able to participate, clearly.

[The other reason I discourage this thinking is it helps people to stop trying to us relative paths when referencing templates]


--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[unai]

Hello,

> I tend to warn people off thinking of template names as paths. It's
> an implementation detail that they are treated as such by the two
> default loaders.

> Requiring each loader to include an unique id with each template
> (even if it's just a full path) seems the easiest solution.

> Older 3rd party loaders which don't do this would not be able to
> participate, clearly.

The patch I'm working on relies on the loaders to do the skipping. That is,
code in ``django/template/loader.py`` does not know anything about template
skipping.

The benefit of that is that the default app and dirs loaders are the ones that
handle the skipping through the template's full path. Other loaders can handle
template skipping as they like or don't handle it at all.

Nearly the only change in ``django/template/loader.py`` would be to accept a
``skip_template`` parameter in ``find_template``, ``select_template`` and
``render_to_template`` funcs and pass that parameter on to the loaders.

Thanks for your concern!

Best wishes,

Unai Zalakain

[J. Pablo Martín Cobos]

2013/12/8 unai <un...@gisa-elkartea.org>

I think that this could have two problems, without see the code it's possible that when I will see the code I will see another problems or I like the complete solution:

 1. Efficiency: If this new solution slows the compilation/find/render template, I dislike it

 2. This solution will complicate the template development. e.g. if a application overwrite the "admin/change_form.html"  template and a developer wants to update this template, he will have to search this template in every app. He does not know what application is overwriting this template... With the current solution [1] if you overwrite the "admin/change_form.html" template this only (the most common usecase) will be in a place, this will be in the template project directory.

For these reasons (above all the second reason), a priori this is not a good solution for me.

REF's

1. https://github.com/django/django/pull/2042

[unai]

Hi!

> 1. Efficiency: If this new solution slows the compilation/find/render
> template, I dislike it

Well, if there is no self-referenced inheritance, no extra code (except for the
very same self-referenced inheritance check) is executed, so there is that.

If self-referenced inheritance is found, it enters a loop loading every
template with the *same path*. So, apart of loading the referenced templates
and parsing them, there's no extra cost that I can think of.

Of course, we will have to wait until I finish the implementation and
benchmarks are ran.

> 2. This solution will complicate the template development. e.g. if a
> application overwrite the "admin/change_form.html" template and a
> developer wants to update this template, he will have to search this
> template in every app. He does not know what application is
> overwriting this template... With the current solution [1] if you
> overwrite the "admin/change_form.html" template this only (the most
> common usecase) will be in a place, this will be in the template
> project directory.

If I understand you correctly, you are saying that if an app extends an other
app's template and the users want to extend this very same template, they don't
know what app they are extending.

Well, that depends on the order of apps in the ``INSTALLED_APPS`` setting. This
order is preserved when inheriting. The same happens with ``TEMPLATE_DIRS``,
its order is preserved.

BTW, the same would happen with our loader-skipping solution from before and
with any non self-referenced extension: if the user extends a template, the
template which is extended is the first template that the app loader finds
iterating over the ``INSTALLED_APPS`` setting.

Naturally, I don't think the overuse of this feature (A --> B --> C --> D)
would be recommendable but I couldn't imagine anyone doing that.

Sorry for keep you waiting, busy week :-/


Best wishes,

Unai Zalakain

[Shai Berger]

On Monday 09 December 2013 14:58:34 unai wrote:
>
> > 2. This solution will complicate the template development. e.g. if a
> >
> > application overwrite the "admin/change_form.html" template and a
> > developer wants to update this template, he will have to search this
> > template in every app. He does not know what application is
> > overwriting this template... With the current solution [1] if you
> > overwrite the "admin/change_form.html" template this only (the most
> > common usecase) will be in a place, this will be in the template
> > project directory.
>
> If I understand you correctly, you are saying that if an app extends an
> other app's template and the users want to extend this very same template,
> they don't know what app they are extending.
>

I read Pablo's concern as: If the skipping is done in each loader, does
"recursive" self-reference work across loaders?

If not, then you cannot use the feature from project templates to extend app-
provided templates -- you must do so in an app, and then the question of which
app and app order becomes important.

Shai.

[Florian Apolloner]

On Monday, December 9, 2013 12:43:04 PM UTC+1, Goinnn wrote:

1. Efficiency: If this new solution slows the compilation/find/render template, I dislike it

Lots of "ifs" which are not really worth discussing before we run actual benchmarks; also I think that it won't be slower since currently template resolving will iterate through all loaders anyways…
 

 2. This solution will complicate the template development. e.g. if a application overwrite the "admin/change_form.html"  template and a developer wants to update this template, he will have to search this template in every app.

That is already the case, you can't know that 'admin/change_form.html' will be in the admin app. I'd actually argue that your patch will make it even worse, since it's not clear on a first sight which loaders will be considered, to find that out you first have to figure out which loader loaded the template in the first place…
 

With the current solution [1] if you overwrite the "admin/change_form.html" template this only (the most common usecase) will be in a place, this will be in the template project directory.

The current solution severely limits the usefulness of loading templates from app folders; personally I never have TEMPLATE_DIRS set to anything, but put my project module into INSTALLED_APPS -- why would I need different mechanisms to load project templates after all.  

 
Cheers,
Florian

[unai]

> I read Pablo's concern as: If the skipping is done in each loader,
> does "recursive" self-reference work across loaders?

Uh, I didn't understand, of course you can ;-)

Imagine you are extending an app template from within a project template and
that the order of the loaders is [dirs, app]. Well, first the dirs loader will
be tried and it will not find anything because the current template is being
skipped. ``find_template`` will then continue (like it does now) with the next
loader, the app loader in this case. A template with the same name but with
different path will be found and returned by the app loader, concluding the
``find_template`` loop.


Cheers!

Unai Zalakain

[J. Pablo Martín Cobos]

2013/12/9 Florian Apolloner <f.apo...@gmail.com>

On Monday, December 9, 2013 12:43:04 PM UTC+1, Goinnn wrote:

1. Efficiency: If this new solution slows the compilation/find/render template, I dislike it

Lots of "ifs" which are not really worth discussing before we run actual benchmarks; also I think that it won't be slower since currently template resolving will iterate through all loaders anyways…
 

Unai said it: "Now, the tricky part is to identify a template uniquely. I went for hashing but, as Apollo13 said on IRC, that's just too expensive"... 

 

 2. This solution will complicate the template development. e.g. if a application overwrite the "admin/change_form.html"  template and a developer wants to update this template, he will have to search this template in every app.

That is already the case, you can't know that 'admin/change_form.html' will be in the admin app.

Yes but now only can happen it a time, with the current PR, two applications can not overwrite the 'admin/change_form.html' template. You could find this template with a simple command, at least if you use a linux OS.

IMHO I think that if we allow that an app template overwrite other app template we will have a system very complex to develop and to debug.... but this is only my opinion.

 

I'd actually argue that your patch will make it even worse, since it's not clear on a first sight which loaders will be considered, to find that out you first have to figure out which loader loaded the template in the first place…
 

With the current solution [1] if you overwrite the "admin/change_form.html" template this only (the most common usecase) will be in a place, this will be in the template project directory.

The current solution severely limits the usefulness of loading templates from app folders; personally I never have TEMPLATE_DIRS set to anything, but put my project module into INSTALLED_APPS -- why would I need different mechanisms to load project templates after all.

It is very very unusual for me, I don't understand why you do this. I always have TEMPLATE_DIRS set for the common templates: base.html, 404.html, 500.html and to overwrite the reusable app templates. You could see an example here:

https://github.com/OpenMOOC/moocng/tree/master/moocng/templates

[Florian Apolloner]

On Monday, December 9, 2013 5:18:16 PM UTC+1, Goinnn wrote:

2013/12/9 Florian Apolloner <f.apo...@gmail.com>

On Monday, December 9, 2013 12:43:04 PM UTC+1, Goinnn wrote:

1. Efficiency: If this new solution slows the compilation/find/render template, I dislike it

Lots of "ifs" which are not really worth discussing before we run actual benchmarks; also I think that it won't be slower since currently template resolving will iterate through all loaders anyways…

Unai said it: "Now, the tricky part is to identify a template uniquely. I went for hashing but, as Apollo13 said on IRC, that's just too expensive"... 

I also said that it "just doesn't feel right" :) But I'd wait for a patch before we start discussing it's theoretical performance implications.

 2. This solution will complicate the template development. e.g. if a application overwrite the "admin/change_form.html"  template and a developer wants to update this template, he will have to search this template in every app.

That is already the case, you can't know that 'admin/change_form.html' will be in the admin app.

 … two applications can not overwrite the 'admin/change_form.html' template. …

And that's not the behavior normal template loading guarantees, so it would be very very odd to change it for the edgecase of self referencing templates. So I'll argue again, that your suggestion is the one making the behavior more unexpected.

It is very very unusual for me, I don't understand why you do this. I always have TEMPLATE_DIRS set for the common templates: base.html, 404.html, 500.html and to overwrite the reusable app templates.

That's okay, there are many people for whom it's not unusual. Why we do it? Simply cause there is no reason to special case common templates or overrides; just put them in an app named like your project and put them on top of INSTALLED_APPS. The benefits are (among other things): You can also have project management commands, static files and templatetags/filters without having to change a few other variables like STATICFILES_DIRS. Think about it for a while and I guess you'll see the merits too.

Cheers,
Florian

[Florian Apolloner]

Just as a good example on why it's not a good idea to skip a loader: Grappelli could most likely strongly benefit from this feature and their preferred setup looks like this http://django-grappelli.readthedocs.org/en/latest/quickstart.html#setup -- Having to set TEMPLATE_DIRS to the grappelli templates just to get self referencing templates wouldn't be nice, I think we can agree on that?

[J. Pablo Martín Cobos]

Florian, I know that the benefits of this solution. But I see the detriments too. I think that if we allow that a app template overwrite with self-reference another app template the template development will be a chaos. I thought alot about it

2013/12/9 Florian Apolloner <f.apo...@gmail.com>

For me this is a peculiar solution, but now I see the merits of it, thanks you!

And of course if you only use the app_directories loader, the current solution does not work for you. 

[unai]

Hi there!

> Florian, I know that the benefits of this solution. But I see the
> detriments too. I think that if we allow that a app template
> overwrite with self-reference another app template the template
> development will be a chaos. I thought alot about it

Well, I agree that if miss used, this feature could be more of a burden than of
a feature. After giving it some thought, three possibilities come to mind. Here
they are, listed in what I think is from most to less preferable:

- Trust that the app developers aren't going to play nasty with it. Clear
miss use warning could be stated in the docs.

- Include (de)activation flags in the loaders. When, for example, the app
loader's flag is deactivated, no app could self-reference an other. For easy
of use, a subclass could be created for every loader class switching the flag
so that you could decide simply by (un)commenting loaders in settings.

- Change the "extends" template tag and add the possibility to provide kwargs
to it. Something like:

{% extends 'some/template.html' from app 'django.contrib.admin' %}
{% extends 'some/template.html' from dir 'some/template/dir' %}

"app" and "dir" here could be aliases defined as some loader's attribute. If
nothing specified, it could fall back to standard self-reference behaviour.
The positive part of this is that you could exactly decide what template to
extend. The negative one, that it is just a lot more of code on the template
layer and that it would (IMO) overcomplicate things.

My two cents! I'll work on the self-referencing code today and tomorrow.

Best regards,

Unai Zalakain

[unai]

Hi there!

> I'm working on an other solution that instead of relying on loader
> skipping relies on template skipping.

Sorry for keeping you waiting on updates for so long!

I was able to put some time into it today. Although it isn't nothing
definitive and testing, benchmarking and docs are missing, here is some actual
code for you to look at:

https://github.com/unaizalakain/django/compare/template_selfreference

You can totally ignore the testing commit. The loaders commit per se is
relatively small and the idea behind it is quite simple. As you already know,
any critic, suggestion or idea is highly appreciated ;-)

I'm going to be able to put some more effort into this tomorrow.

Best regards,

Unai Zalakain

[Mitar]

Hi!

I just discovered this thread on the mailing list. 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:
https://github.com/mitar/overextends-test).

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.

See https://code.djangoproject.com/ticket/17199#comment:9

Comments band benchmarks are welcome.


Mitar

--
http://mitar.tnode.com/
https://twitter.com/mitar_m

[unai]

Hi there!

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

I'm not entirely convinced about maintaining a dicts of lists in the template
context containing the template candidates. I didn't work on this issue lately
but I managed to write a little article about it[0], kind of for my own
purposes. In there I try to come with a way of knowing what would the next
template to be rendered be without storing continuously a list of templates. Of
course, the system I'm purposing could be totally wrong and don't have any
sense, so let me know what you think about it ;)

[0] http://unaizalakain.info/2013/12/19/self-referenced-template-inheritance-with-django/