ifchanged no longer works within an included template

[Matthew Somerville]

If you use ifchanged within an included template, whereas in 1.6 and previous this worked fine, in 1.7 ifchanged no longer appears to remember its stored state between iterations and so is always true.

I have uploaded a minimal test project to show this at ​https://github.com/dracos/broken-ifchanged with the Travis test results for 1.6 (pass) and 1.7 (fail) at ​https://travis-ci.org/dracos/broken-ifchanged/builds/35620470

I assume this is presumably related to the recursion changes mentioned in the Django 1.7 release notes, but if so it's not totally clear and so I thought I'd report this so at least the documentation could perhaps be changed to mention this issue and that you can't use ifchanged inside an include inside a loop.

I'm not sure what we can do now to get our code (full code Travis with 1.7 failure can be seen at ​https://travis-ci.org/mysociety/sayit/builds/35613315 ) working on 1.7 - we don't want to inline the inner template, it's quite large and used in multiple places.

[Baptiste Mispelon]

Hi,

I can reproduce this regression and traced it back to e2f06226ea4a38377cdb69f2f5768e4e00c2d88e as you suspected.

Not sure if we can fix the code or if we should document the regression but I'm moving the ticket forward either way.

Thanks.

[Curtis Maloney]

I reworked the include tag significantly for 1.7.

Previously there were two actual tag nodes, depending on if you passed an immediate string for the template name or not.

However, this difference made it painful to do recursion in templates, forcing people to trip the dynamic include node lest they cause infinite recursion.

I'll take a closer look tonight and see if I can pin down the cause -- most likely it's the render context being pushed/popped.

[Duncan Parkes]

I've been doing a little bit of digging around in here, and the bug was already present in 1.6, just in slightly fewer circumstances. If you replace @dracos' template with

{% for item in list %}
  {% with bit='bit.html' %}
    {% include bit %}
  {% endwith %}
{% endfor %}

you'll see failures in both 1.6 and 1.7.

The problem is that IfChangedNode stores what it looked like last time in the context by setting it as state_frame[self]. We were only getting the right thing in 1.6 because it was using ConstantIncludeNode, which calls get_template in its __init__(). With a normal IncludeNode, which is all there is in 1.7, get_template is called in render() causing a new IfChangedNode object. When the new IfChangedNode is looked up as state_frame[self] it's not the same self!

Now I understand the problem, but I'm still not sure I have a solution. Perhaps IfChangedNode could be made to produce the same object rather than a new copy under the right circumstances, or perhaps we should store the previous version to compare to in state_frame using something other than self as the key.

[Curtis Maloney]

If the IfChanged tag had some identifying key, we could stash the details in the render_context and everything becomes simpler.

That would also open the opportunity to use IfChanged with a key name instead of an expression, expanding the scope of use and avoiding duplicated work...

The trick then becomes generating a key in cases where it's not specified but _should_ be consistent. Using template name + tag instance comes to mind, but won't work when a template is included in multiple places.... </thinkingaloud>

[Preston Timmons]

This may not be the right fix, but a potential solution is to make the include tag cache parsed template objects:

​https://github.com/prestontimmons/django/compare/ticket-23516

This also speeds up rendering when the same template is included multiple times. Some basic timing gave me these results:

cached: 1.96s
non-cached: 0.25s

This was from rendering a filesystem template 100 times that contained a forloop to include another template 100 times.

I thought about this solution when I noticed this behavior doesn't appear when using the cached loader.

[Matthew Somerville]

As someone who frequently turns on the cached template loader even in development in order to have acceptable performance of templates used in loops, that sounds like a great idea to me if it's okay :-) I'm at the DjangoCon EU sprints this week if it's possible for me to help/work on this in some way.

[Matthew Somerville]

I have submitted a PR at ​https://github.com/django/django/pull/4753 with comments outlined there, reworked to use render_context as suggested.

[Tim Graham]

Looks good pending some cosmetic comments.

[Tim Graham <timograham@…>]

In a391b17:

Fixed #23516 -- Added caching of include tag Template objects

This also speeds up for loops that render the same template
multiple times.