[Django] #24555: Wrong content in inclusion templatetags with variable parent template
Django
----------------------------+----------------------------------------------
Reporter: | Owner: nobody
benjaminrigaud |
Type: Bug | Status: new
Component: Template | Version: 1.8rc1
system |
Severity: Normal | Keywords: templatetags extends block cache
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------------
If two custom inclusion tags specify their parent template through their
context and use the same block name, the rendered content of the second
tag is replaced by the content of the first one
Sandbox project: https://github.com/benjaminrigaud/extends-templatetags-
bug
Gist: https://gist.github.com/benjaminrigaud/e646f51e1a6dfe232c68
It works fine if we replace the custom tags with standard include tags.
It is a regression from 1.7 (tested with 1.7.7).
It may be related to the cache mechanism of the BlockContext:
https://github.com/django/django/blob/master/django/template/loader_tags.py#L115
--
Ticket URL: <https://code.djangoproject.com/ticket/24555>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: benjaminrigaud | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_docs: => 0
* needs_better_patch: => 0
* severity: Normal => Release blocker
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Hi,
I can indeed confirm the regression and I bisected it down to commit
e53495ba3352c2c0fdb6178f2b333c30cb6b5d46.
Bumping to release blocker status because of the regression.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:1>
Django
Reporter: benjaminrigaud | Owner: nobody
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* Attachment "repro-24555.diff" added.
Reproduction testcase
Django
Reporter: benjaminrigaud | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* owner: nobody => aaugustin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:2>
Django
Reporter: benjaminrigaud | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
The problem here happens on line 1325:
{{{
return self.nodelist.render(new_context)
}}}
Unlike `Template.render`, `NodeList.render` doesn't push the context
stack. Hence, blocks stored in the `render_context` persist where they
should not.
There are a few solutions:
1) Wrap this line in `with new_context.render_context.push():`.
2) Remove the internal `InclusionNode` caching and just call
`t.render(new_context)`.
3) Cache the template object rather than the nodelist and use that.
The easiest solution is to remove internal caching and leave it to the
loaders. That also fixes #23441 that's caused by `InclusionNode` setting
self.nodelist at render time. The downside is a performance regression if
``InclusionNode`` is used in a for loop.
If internal caching is kept, it should be done in the render_context
rather than on the node.
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:3>
Django
Reporter: benjaminrigaud | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* cc: prestontimmons (added)
Comment:
Preston, honestly, you understand this code much better than I do. What is
your recommendation?
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:4>
Django
Reporter: benjaminrigaud | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I'll write a patch for option 3 so we don't introduce a performance
regression. Preferably, we can fix this at the engine level later so the
node doesn't have to take care of it's own caching.
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:5>
Django
Reporter: benjaminrigaud | Owner:
Type: Bug | Status: new
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* owner: aaugustin =>
* status: assigned => new
Comment:
Great. Thank you so much for your help!
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:6>
Django
Reporter: benjaminrigaud | Owner:
Type: Bug | Status: new
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
Keywords: templatetags | Triage Stage: Accepted
extends block cache |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:7>
Django
Reporter: benjaminrigaud | Owner:
Type: Bug | Status: new
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution:
extends block cache | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:8>
Django
Reporter: benjaminrigaud | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed
Component: Template system | Version: 1.8rc1
Keywords: templatetags | Triage Stage: Ready for
extends block cache | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* owner: => Tim Graham <timograham@…>
* resolution: => fixed
Comment:
In [changeset:"0808ccce3808235c5b5a56e3f689cec0d4bc0ebf" 0808ccc]:
{{{
#!CommitTicketReference repository=""
revision="0808ccce3808235c5b5a56e3f689cec0d4bc0ebf"
Fixed #23441, #24555 -- Improved the behavior of InclusionNode.
This change:
* Makes the InclusionNode cache-safe by removing render-time side effects
to its nodelist.
* Ensures the render_context stack is properly scoped and reset by
updating
the render call to use Template.render rather than Nodelist.render.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:9>
Django
Reporter: benjaminrigaud | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution: fixed
Keywords: templatetags | Triage Stage: Ready for
extends block cache | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5c63c45512eeca1f9c2dbdb0c46cbc7d9c550bdc" 5c63c45]:
{{{
#!CommitTicketReference repository=""
revision="5c63c45512eeca1f9c2dbdb0c46cbc7d9c550bdc"
[1.8.x] Fixed #23441, #24555 -- Improved the behavior of InclusionNode.
This change:
* Makes the InclusionNode cache-safe by removing render-time side effects
to its nodelist.
* Ensures the render_context stack is properly scoped and reset by
updating
the render call to use Template.render rather than Nodelist.render.
Backport of 0808ccce3808235c5b5a56e3f689cec0d4bc0ebf from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:10>
Django
Reporter: benjaminrigaud | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution: fixed
Keywords: templatetags | Triage Stage: Ready for
extends block cache | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by nealtodd):
Although the ticket is closed I'm adding this comment in case anyone
stumbles across the same esoteric gotcha I came across related to this
change:
After updating a project from 1.8c1 to 1.8 a bunch of tests that had been
running cleanly started failing on `assertFormError` in the format of
`AssertionError: The form 'form' in context X does not contain the field
'foo'`.
Long story short, the template had a 'main' form that was supplied via a
context variable named 'form', but the base template also used an
inclusion tag to render a site search form, and the inclusion tag's
template was also supplied via a context variable named 'form'. The
caching introduced in this commit meant that the inclusion tag's template
now also appeared in the `response.context`, and so, when calling
`self.assertFormError(response, "form", "foo", "expected error message")`
[https://github.com/django/django/blob/master/django/test/testcases.py#L386
assertFormError] looped over the context and found a form (the search
form) without the intended field `foo` (as well as finding it as expected
on the main form).
The simple fix in this case was to scope the context key name of the
search form to, e.g. 'search_form' for the inclusion tag's template.
However as, ordinarily, a clash of key names used in the contexts for
different templates shouldn't matter, I thought I'd note it here as it
left me scratching my head for a while until I tracked in down.
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:11>
Django
Reporter: benjaminrigaud | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed
Component: Template system | Version: 1.8rc1
Severity: Release blocker | Resolution: fixed
Keywords: templatetags | Triage Stage: Ready for
extends block cache | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I see. This behavior change isn't due to caching. Instead it's due to the
change from `self.nodelist.render()` to `template.render()`. This causes
the `template_rendered` signal to be sent for inclusion tags, whereas it
wasn't before. The listeners on this signal update `response.context`,
`response.templates`, etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/24555#comment:12>