Wrong content in inclusion templatetags with variable parent template

[Benjamin Rigaud]

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

[Baptiste Mispelon]

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.

[Baptiste Mispelon]

Reproduction testcase

[Preston Timmons]

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.

[Aymeric Augustin]

Preston, honestly, you understand this code much better than I do. What is your recommendation?

[Preston Timmons]

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.

[Aymeric Augustin]

Great. Thank you so much for your help!

[Tim Graham <timograham@…>]

In 0808ccc:

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.

[Tim Graham <timograham@…>]

In 5c63c45:

[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

[Neal Todd]

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.

[Preston Timmons]

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.