Opened 16 years ago

Closed 14 years ago

Last modified 12 years ago

#6510 closed (fixed)

get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode and IfChangedNode)

Reported by: adam@… 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)

6510.diff (5.7 KB ) - added by Chris Beaven 15 years ago.
6510.2.diff (5.3 KB ) - added by Chris Beaven 14 years ago.
6510.3.diff (6.0 KB ) - added by Chris Beaven 14 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 by Chris Beaven, 16 years ago

Has patch: set

This patch fixes those (and {% for %}), factoring out some of the work back to NodeList, complete with tests.

comment:2 by Simon Greenhill <dev@…>, 16 years ago

Triage Stage: UnreviewedReady for checkin

comment:3 by James Bennett, 16 years ago

Triage Stage: Ready for checkinDesign 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 Chris Beaven, 16 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).

comment:5 by James Bennett, 16 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.

in reply to:  5 comment:6 by Chris Beaven, 16 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 Hystrix, 15 years ago

Cc: ego@… 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 Hystrix, 15 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 Chris Beaven, 15 years ago

Attachment: 6510.diff added

comment:9 by Chris Beaven, 15 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 Chris Beaven, 15 years ago

Owner: changed from nobody to Chris Beaven
Status: newassigned
Summary: Block inheritance doesn't work within {% ifequal %} tagsget_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode)

comment:11 by anonymous, 15 years ago

Cc: herbert.poul@… added

comment:12 by mMetry, 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 xdcdx, 14 years ago

Did this finally get fixed? Somehow it is not yet working with Django 1.1.1

by Chris Beaven, 14 years ago

Attachment: 6510.2.diff added

comment:15 by Chris Beaven, 14 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 Russell Keith-Magee, 14 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 Russell Keith-Magee, 14 years ago

milestone: 1.2
Triage Stage: Design decision neededAccepted

by Chris Beaven, 14 years ago

Attachment: 6510.3.diff added

comment:18 by Chris Beaven, 14 years ago

Now with a test showing an external manifestation (probably what #10975 was talking about).

comment:19 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: assignedclosed

(In [12655]) [1.1.X] Fixed #6510 -- Refactored the way child nodes are found in template nodes to avoid potential inconsistencies. Thanks to SmileyChris for the patch.

Backport of r12654 from trunk.

comment:20 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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