Opened 8 years ago

Closed 6 years ago

Last modified 4 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: SmileyChris
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 SmileyChris 7 years ago.
6510.2.diff (5.3 KB) - added by SmileyChris 6 years ago.
6510.3.diff (6.0 KB) - added by SmileyChris 6 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by SmileyChris

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

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

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 8 years ago by ubernostrum

  • Triage Stage changed from Ready for checkin to 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 Changed 8 years ago by SmileyChris

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 follow-up: Changed 8 years ago by 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".

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

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 7 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 7 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 7 years ago by SmileyChris

comment:9 Changed 7 years ago by SmileyChris

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 7 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned
  • Summary changed from Block inheritance doesn't work within {% ifequal %} tags to get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode)

comment:11 Changed 7 years ago by anonymous

  • Cc herbert.poul@… added

comment:12 Changed 6 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 6 years ago by xdcdx

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

Changed 6 years ago by SmileyChris

comment:15 Changed 6 years ago by SmileyChris

  • Summary changed from get_nodes_by_type refactor for easier extensibility (fixes an underlying bug for IfEqualNode) to 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 6 years ago by russellm

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 6 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Design decision needed to Accepted

Changed 6 years ago by SmileyChris

comment:18 Changed 6 years ago by SmileyChris

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

comment:19 Changed 6 years ago by russellm

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

(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 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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