Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#13444 closed (fixed)

{% cycle %} no longer works within inclusion tags and included templates

Reported by: awmcclain Owned by: nobody
Component: Documentation Version: 1.2-beta
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Cycle only works within the template that contains the for loop -- if you include a subtemplate or inclusion tag, cycle doesn't work.

This is a regression from 1.1.1.

def test(request):
    return render_to_response('test.html', {'bar':range(4)})

----- test.html
{% for i in bar %}
{% cycle 'alpha' 'bravo' %} Tag:{% foo %} Include:{% include 'foo.html' %}
{% endfor %}

@register.inclusion_tag('foo.html', takes_context=True)
def foo(context):
    return context

-------- foo.html
{% cycle 'alpha' 'bravo' %}

alpha Tag:alpha Include:alpha
bravo Tag:alpha Include:alpha 
alpha Tag:alpha Include:alpha 
bravo Tag:alpha Include:alpha 

Change History (7)

comment:1 Changed 6 years ago by awmcclain

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 follow-up: Changed 6 years ago by Alex

Honestly, the previous behavior sounds like a bug.

comment:3 Changed 6 years ago by russellm

  • Component changed from Uncategorized to Template system
  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Design decision needed

Putting on the 1.2 milestone; Alex's comment may be right, but given that this is potentially a regression, it needs to be resolved before 1.2 gets to release candidate.

comment:4 in reply to: ↑ 2 Changed 6 years ago by awmcclain

Replying to Alex:

Honestly, the previous behavior sounds like a bug.

I disagree. Subtemplates within a {% for %} tag still have access to the forloop template variables (forloop.counter, for example), so it doesn't make sense that they wouldn't be able to use {% cycle %}. They're still within the for loop.

comment:5 Changed 6 years ago by russellm

  • Component changed from Template system to Documentation
  • Triage Stage changed from Design decision needed to Accepted

@awmcclain: After discussion with Jannis and Jacob on IRC, we've come to the conclusion that Alex is correct in saying that previous behavior is a bug.

When you use {% cycle %} in the way you describe, you're not setting a context variable. The state of the cycle tag isn't held in context, it's part of the state of the cycle tag itself. By cycling, you're updating the state of a specific node in the template tree. The only time that state reaches context is if you name the cycle (using {% cycle ... as foo %}), and that only gives you the ability to read, not modify the cycle contents.

The use case you demonstrate depends on two bugs in the v1.1 template engine:

1) The include tag didn't establish a clean environment for each template load (and each include should be a clean template load)
2) The cycle tag in v1.1 wasn't threadsafe, so multiple template loaders could access the same cycle states.

These bugs have both been corrected as part of the cleanup required to allow for cached template loaders.

Another way of looking at the point (1): Although the end user documentation isn't clear on this point, the implementation (both current and historical) makes it clear that {% include %} should be interpreted as "Render this entire template and insert the HTML", not as "insert the node hierarchy parsed from this subtemplate". Since each included template should be an independent rendering, it doesn't make sense that cycle states should leak between renderings.

One way to restore the "old" behaviour (depending on exactly how you're using cycle) relies on using the part of cycle that *is* pushed into context. If you define the cycle tag in test.html as:

{% for i in bar %}
{% cycle 'alpha' 'bravo' as testvalue %} Tag:{% foo %} Include:{% include 'foo.html' %}
{% endfor %}

and change your foo.html to read:

{{ testvalue }}

you will get the output you expected to see.

I'll formally accept this on the basis that extra documentation is required. In particular:

  • Extra notes for the backwards compatibility guide (since it isn't immediately obvious why this is a bug, not a regression). We already have a section on tags that might be affected by the rendering changes; this is an excellent example of how those changes could affect people in practice.
  • A note to the {% include %} clarifying the intention of that tag.

comment:6 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [13063]) Fixed #13444 -- Improved the documentation around the backwards compatibility quirks of the cycle and include tags. Thanks to awmcclain for the report.

comment:7 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top