#6510 closed (fixed)
get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode and IfChangedNode)
Reported by: | Owned by: | Chris Beaven | |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Keywords: | block ifequals | |
Cc: | ego@…, herbert.poul@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Block inheritance works fine within {% if %} tags, but not within {% ifequal %} or {% ifnotequal %} tags, as illustrated by the code below.
alpha.html
(the variable "t" is defined in the view as "true")
{% if t %} {% block stuff %}ALPHA{% endblock %} {% endif %} {% ifequal t "true" %} {% block stuff %}ALPHA{% endblock %} {% endifequal %} {% ifnotequal t "false" %} {% block stuff %}ALPHA{% endblock %} {% endifnotequal %}
beta.html
{% extends "alpha.html" %} {% block stuff %}BETA{% endblock %}
results
BETA ALPHA ALPHA
Attachments (3)
Change History (23)
comment:1 by , 17 years ago
Has patch: | set |
---|
comment:2 by , 17 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:3 by , 17 years ago
Triage Stage: | Ready for checkin → Design decision needed |
---|
Why not just put the conditional test inside the block? The existence of the block doesn't change based on the outcome of the conditional, only its contents, so it doesn't make sense to write it this way (and conditional definition of blocks is something Django has never really tried to support; AFAIK, if it works at all it works by accident).
comment:4 by , 17 years ago
This has nothing to do with conditionals or the "existence of a block", the issue is that a tag should be visible anywhere in a template by using the NodeList.get_nodes_by_type()
method. For example:
{% for i in list %} {% block forblock %}something{% endblock %} {% endfor %}
The behavior of the ForNode
and IfEqualNode
are obviously broken if you take a closer look at how the IfNode.get_nodes_by_type()
works (obviously not "accidental" code).
follow-up: 6 comment:5 by , 17 years ago
Your example doesn't make this any more clear to me; you can't recur a block of the same name multiple times, as that loop attempts to do. Similarly, you can't say "this block exists only if this condition is true".
You can say "inside this block, loop over this iterable and output that each time". And you can say, "inside this block, output this if that is true".
So I'm not sure why a block
tag inside a conditional or a loop should be expected to work.
comment:6 by , 17 years ago
Replying to ubernostrum:
Your example doesn't make this any more clear to me; you can't recur a block of the same name multiple times, as that loop attempts to do. Similarly, you can't say "this block exists only if this condition is true".
A {% block %}
tag just provides an extensible area for child templates. These {% block %}
replacements happen before any actual rendering, be it conditional or iterable.
As an aside, what if you were looking for another type of tag - this patch would also fix those situations.
comment:7 by , 16 years ago
Cc: | added |
---|
I've just been biten by this bug and I have to say it took me more than a day to find out why the child template didn't show up on the parent. If it won't be resolved at least it should be noted on the docs that you can't expect block tags to work inside ifequal.
comment:8 by , 16 years ago
Here's a use case for this, the view would pass a list of objects and some way to identify the active one. The parent template can render the list and set a block so the childs can modify only the active object with ease.
parent.html
<ol> {% for o in objects %}<li> {% ifequal o active %}{% block active %}Object {{o}}{% endblock %} {% else %}{% include 'object.html' %}{% endifequal %} </li>{% endfor %} </ol>
child1.html
{% extends 'parent.html' %} {% block active %}Action 1 for {{o}}{% endblock %}
child2.html
{% extends 'parent.html' %} {% block active %}Action 2 for {{o}}{% endblock %}
That way I don't have to write the for structure for every child
resutlt
Using child1.html with objects = ['a', 'b', 'c'], active = 'b'
<ol> <li>Object a</li> <li>Action 1 for b</li> <li>Object c</li> </ol>
Using child2.html with objects = ['a', 'b', 'c'], active = 'a'
<ol> <li>Action 2 for a</li> <li>Object b</li> <li>Object c</li> </ol>
by , 16 years ago
comment:9 by , 16 years ago
PS: I just updated the patch to latest trunk.
And to anyone reviewing this ticket - note that the ticket description is a red herring. This is a valid bug even without a repeated {% block %}
tag
comment:10 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Summary: | Block inheritance doesn't work within {% ifequal %} tags → get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode) |
comment:11 by , 16 years ago
Cc: | added |
---|
comment:12 by , 15 years ago
Hi guys. Any progress on this? I'm developing using Django v0.96 on the Google App Engine and this is coming up frequently. Any help would be much appreciated.
comment:13 by , 15 years ago
Did this finally get fixed? Somehow it is not yet working with Django 1.1.1
by , 15 years ago
Attachment: | 6510.2.diff added |
---|
comment:15 by , 15 years ago
Summary: | get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode) → get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode and IfChangedNode) |
---|
New patch against trunk, tests show the problem more clearly.
comment:16 by , 15 years ago
Summarizing a discussion with SmileyChris on IRC:
The original problem that is alluded to (inconsistency between if and ifequal) no longer exists. The tests from the first patch (6510.diff) pass without any code modifications.
The current proposal in this ticket is to refactor and clean up the way child nodes are handled in template nodes, especially with the get_nodes_by_type() call. Calls to get_nodes_by_type() currently fail on ifequal and ifchanged nodes due to some inconsistencies in implementation. This doesn't have any obvious external manifestations, but in the interests of everything working consistently and avoiding nasty surprises later on, SmileyChris' patch does some cleanup.
comment:17 by , 15 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Design decision needed → Accepted |
by , 15 years ago
Attachment: | 6510.3.diff added |
---|
comment:18 by , 15 years ago
Now with a test showing an external manifestation (probably what #10975 was talking about).
comment:19 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This patch fixes those (and
{% for %}
), factoring out some of the work back toNodeList
, complete with tests.