Re: [Django] #15053: Make templates more reusable by Improving template loading algorithm to avoid extending infinite recursion
Django
avoid extending infinite recursion
---------------------------------------+---------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Milestone: | Component: Template system
Version: 1.2 | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+---------------------------------
Changes (by kmike):
* cc: kmike84@… (added)
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
avoid extending infinite recursion
Type: New feature | Status: reopened
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:11>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by pmartin):
Replying to [comment:8 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
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:12>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Wouter):
Do something about this please!
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:13>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* needs_docs: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:14>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by pmartin):
Ok I will do, sorry for the delay
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:15>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by pmartin):
Now works with the last version of
[https://github.com/goinnn/django/commit/ddb9fc152422743c18c1336413ad3255b5305385
django]. I want to write comments, analize the code and I will try to
clean the code. I coded this 18 month ago...
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:16>
Django
avoid extending infinite recursion
---------------------------------+------------------------------------
Reporter: pmartin | Owner: nobody
Type: New feature | Status: reopened
Component: Template system | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by pmartin):
I hope that you like [https://github.com/django/django/pull/217 it]
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:17>
Django
avoid extending infinite recursion
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* owner: nobody => unaizalakain
* status: new => assigned
* version: 1.2 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:19>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:20>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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 ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:21>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by unaizalakain):
PR sent: https://github.com/django/django/pull/1890
Any comment, suggestion or correction appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:22>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:23>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by pmartin):
Unai I improved the code of my pull request in [https://github.com/goinnn
/django-smart-extends 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
[https://github.com/django/django/blob/master/django/template/loaders/cached.py
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
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:24>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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!
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:25>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by pmartin):
Ok I will try to do a pull request the 16th or 17th November.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:26>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by pmartin):
Replying to [comment:26 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
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:27>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by pmartin):
https://github.com/unaizalakain/django/pull/3
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:28>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by unaizalakain):
Sorry for the delay, suggestions noted at the PR ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:29>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by pmartin):
Replying to [comment:29 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:30>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:31>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
- 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:32>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by unaizalakain):
PR sent: https://github.com/django/django/pull/2042
Mailing list discussion: https://groups.google.com/forum/#!topic/django-
developers/0kFgCCMXnpY
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:33>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* cc: apollo13 (added)
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:34>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* cc: mmitar@… (added)
Comment:
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" ([https://github.com/mitar/overextends-test 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:35>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:36>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by mitar):
@apollo13: A question: my tag currently uses template context to trace
each extended template loading paths. Is this OK?
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:37>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:38>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:39>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* cc: riccardo.magliocchetti@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:40>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* cc: julenx@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:41>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:42>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:43>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:44>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:45>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:46>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:47>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (by rm_):
@prestontimmons thanks a lot for your work! Would be nice to have all this
high level description in the commit is it is not lost.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:48>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:49>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:50>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:51>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* keywords: => 1.8-alpha
* needs_better_patch: 0 => 1
Comment:
Blocked on merging of multiple templates branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:52>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Changes (by timgraham):
* keywords: 1.8-alpha =>
Comment:
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. "
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:53>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:54>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Changes (by prestontimmons):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:55>
Django
avoid extending infinite recursion
---------------------------------+----------------------------------------
Reporter: pmartin | Owner: unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------------
Changes (by prestontimmons):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:56>
Django
avoid extending infinite recursion
Reporter: pmartin | Owner:
| unaizalakain
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:57>
Django
avoid extending infinite recursion
-------------------------------------+-------------------------------------
Reporter: pmartin | Owner:
| unaizalakain
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"fc2147152637e21bc73f991b50fa06254af02739" fc21471]:
{{{
#!CommitTicketReference repository=""
revision="fc2147152637e21bc73f991b50fa06254af02739"
Fixed #15053 -- Enabled recursive template loading.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:58>
Django
avoid extending infinite recursion
-------------------------------------+-------------------------------------
Reporter: pmartin | Owner:
| unaizalakain
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Preston Timmons <prestontimmons@…>):
In [changeset:"8ae04e76301bcc5f72a73b7d26876a650bdf18bf" 8ae04e76]:
{{{
#!CommitTicketReference repository=""
revision="8ae04e76301bcc5f72a73b7d26876a650bdf18bf"
Added docs for new template loader api.
Refs #15053.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:60>
Django
avoid extending infinite recursion
-------------------------------------+-------------------------------------
Reporter: pmartin | Owner:
| unaizalakain
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Preston Timmons <prestontimmons@…>):
In [changeset:"65a7a0d9eed4f4da9568cf0e7688e7a8c9f01957" 65a7a0d]:
{{{
#!CommitTicketReference repository=""
revision="65a7a0d9eed4f4da9568cf0e7688e7a8c9f01957"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:59>
Django
avoid extending infinite recursion
-------------------------------------+-------------------------------------
| Zalakain
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"5d8da093a974f41e08573bbe0d32d5ffeaadd0ad" 5d8da09]:
{{{
#!CommitTicketReference repository=""
revision="5d8da093a974f41e08573bbe0d32d5ffeaadd0ad"
Refs #15053 -- Removed support for non-recursive template loading.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:61>
Django
avoid extending infinite recursion
-------------------------------------+-------------------------------------
Reporter: Pablo Martín | Owner: Unai
| Zalakain
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c70cd2a926ffab47f6613e83e0c8828eb6c2c064" c70cd2a9]:
{{{
#!CommitTicketReference repository=""
revision="c70cd2a926ffab47f6613e83e0c8828eb6c2c064"
Refs #15053 -- Clarified debug message when skipping templates to avoid
recursion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15053#comment:62>