Opened 9 years ago

Closed 7 years ago

Last modified 5 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: master
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: UI/UX:

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 8 years ago.
6510.2.diff (5.3 KB) - added by Chris Beaven 7 years ago.
6510.3.diff (6.0 KB) - added by Chris Beaven 7 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by Chris Beaven

Has patch: set

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

comment:2 Changed 9 years ago by Simon Greenhill <dev@…>

Triage Stage: UnreviewedReady for checkin

comment:3 Changed 9 years ago by James Bennett

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 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by James Bennett

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 in reply to:  5 Changed 9 years ago by Chris Beaven

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 Changed 8 years ago by Hystrix

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 Changed 8 years ago by Hystrix

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>

Changed 8 years ago by Chris Beaven

Attachment: 6510.diff added

comment:9 Changed 8 years ago by Chris Beaven

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 Changed 8 years ago by Chris Beaven

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 Changed 8 years ago by anonymous

Cc: herbert.poul@… added

comment:12 Changed 8 years ago by mMetry

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 Changed 7 years ago by xdcdx

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

comment:14 Changed 7 years ago by Alexandre Garnier

Changed 7 years ago by Chris Beaven

Attachment: 6510.2.diff added

comment:15 Changed 7 years ago by Chris Beaven

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 Changed 7 years ago by Russell Keith-Magee

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 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Triage Stage: Design decision neededAccepted

Changed 7 years ago by Chris Beaven

Attachment: 6510.3.diff added

comment:18 Changed 7 years ago by Chris Beaven

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

comment:19 Changed 7 years ago by Russell Keith-Magee

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 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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