#24555 closed Bug (fixed)
Wrong content in inclusion templatetags with variable parent template
Reported by: | Benjamin Rigaud | Owned by: | |
---|---|---|---|
Component: | Template system | Version: | 1.8rc1 |
Severity: | Release blocker | Keywords: | templatetags extends block cache |
Cc: | Preston Timmons | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Attachments (1)
Change History (13)
comment:1 by , 10 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 10 years ago
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.
comment:4 by , 10 years ago
Cc: | added |
---|
Preston, honestly, you understand this code much better than I do. What is your recommendation?
comment:5 by , 10 years ago
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.
comment:6 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Great. Thank you so much for your help!
comment:7 by , 10 years ago
Has patch: | set |
---|
comment:8 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:11 by , 10 years ago
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")
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.
comment:12 by , 10 years ago
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.
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.