Code

Opened 6 years ago

Closed 4 years ago

#7251 closed (wontfix)

Template if/ifequal/ifnotequal evaluate despite condition

Reported by: Joshua 'jag' Ginsberg <jag@…> Owned by: nobody
Component: Template system Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The problem:

Django evaluates all branches of an if/else/endif regardless of the condition. Most of the time, given the forgiving nature of templates, this is fine. However sometimes this is really bad.

root@agilus:/srv/django/satchmo# ./manage.py shell
Python 2.5.1 (r251:54863, Mar  7 2008, 03:19:34) 
[GCC 4.1.2 (Ubuntu 4.1.2-0ubuntu4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from satchmo.shop.templatetags import satchmo_util
>>> satchmo_util.app_enabled("recentlist")
''
>>> from django.template import Template, RequestContext
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}Enabled{% endif %}""")
>>> from django.template import Context
>>> ctx = Context({})
>>> t.render(ctx)
u''
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}{% load satchmo_recentlist %}{% endif %}""")
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 166, in __init__
    self.nodelist = compile_string(template_string, origin)
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 187, in compile_string
    return parser.parse()
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 271, in parse
    compiled_result = compile_func(self, token)
  File "/usr/local/lib/python2.5/site-packages/django/template/defaulttags.py", line 764, in do_if
    nodelist_true = parser.parse(('else', 'endif'))
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 271, in parse
    compiled_result = compile_func(self, token)
  File "/usr/local/lib/python2.5/site-packages/django/template/defaulttags.py", line 857, in load
    (taglib, e))
TemplateSyntaxError: 'satchmo_recentlist' is not a valid tag library: Could not load template library from django.templatetags.satchmo_recentlist, No module named satchmo_recentlist

Django shouldn't have even evaluated the {% load satchmo_recentlist %} because the condition clearly fails. The same bug exists for ifequals and ifnotequals.

With the attached patch applied, we see the following behavior, which I believe is correct:

root@agilus:/srv/django/satchmo# ./manage.py shell
Python 2.5.1 (r251:54863, Mar  7 2008, 03:19:34) 
[GCC 4.1.2 (Ubuntu 4.1.2-0ubuntu4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from satchmo.shop.templatetags import satchmo_util
>>> satchmo_util.app_enabled("recentlist")
''
>>> from django.template import Template, Context
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}Enabled{% endif %}""")
>>> ctx = Context({})
>>> t.render(ctx)
u''
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}{% load satchmo_recentlist %}{% endif %}""")
>>> t.render(ctx)
u''
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}{% load satchmo_recentlist %}{% else %}{% load nonexistant_tags %}{% endif %}""")
>>> t.render(ctx)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 176, in render
    return self.nodelist.render(context)
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 751, in render
    bits.append(self.render_node(node, context))
  File "/usr/local/lib/python2.5/site-packages/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/usr/local/lib/python2.5/site-packages/django/template/defaulttags.py", line 251, in render
    raise self.nodelist_false
TemplateSyntaxError: 'nonexistant_tags' is not a valid tag library: Could not load template library from django.templatetags.nonexistant_tags, No module named nonexistant_tags
>>> t = Template("""{% load satchmo_util %}{% if not "recentlist"|app_enabled %}{% load satchmo_recentlist %}{% endif %}""")
>>> t.render(ctx)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 176, in render
    return self.nodelist.render(context)
  File "/usr/local/lib/python2.5/site-packages/django/template/__init__.py", line 751, in render
    bits.append(self.render_node(node, context))
  File "/usr/local/lib/python2.5/site-packages/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/usr/local/lib/python2.5/site-packages/django/template/defaulttags.py", line 246, in render
    raise self.nodelist_true
TemplateSyntaxError: 'satchmo_recentlist' is not a valid tag library: Could not load template library from django.templatetags.satchmo_recentlist, No module named satchmo_recentlist
>>> t = Template("""{% load satchmo_util %}{% if "recentlist"|app_enabled %}{% load satchmo_recentlist %}{% else %}Disabled{% endif %}""")
>>> t.render(ctx)
u'Disabled'
>>> t = Template("""{% load satchmo_util %}{% ifequal 3 4 %}{% load satchmo_recentlist %}{% else %}Disabled{% endifequal %}""")
>>> t.render(ctx)

Thanks!

-jag

Attachments (2)

django-if-tag.patch (3.3 KB) - added by Joshua 'jag' Ginsberg <jag@…> 6 years ago.
django-if-tag.2.patch (2.0 KB) - added by Joshua 'jag' Ginsberg <jag@…> 6 years ago.

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by Joshua 'jag' Ginsberg <jag@…>

Changed 6 years ago by Joshua 'jag' Ginsberg <jag@…>

comment:1 Changed 6 years ago by jshaffer

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

comment:2 Changed 6 years ago by ericholscher

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by jdunck

IMHO, this isn't as bad as it sounds; summarizing the thread, what's really going on here is that the load tag has a side-effect in the *parse* phase of template usage; this affects the example because app_enabled returns true because of that side effect.

Introducing a {% loadif %} tag (as suggested in the thread) is a bit late for 1.0, I think, and not backwards incompatible.

comment:4 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 to post-1.0

This would require a fairly intrusive change to the template parsing code from a first look at it. Not appropriate for right now.

Adding new tags after 1.0 is tricky, too, since it potentially introduces a name clash with a third-party tag, breaking existing code (and hence not being backwards-compatible). That part needs some thought. Bumping the main issue to post-1.0 in any case. The current workaround is "don't do that" and/or writing your own loading tag.

comment:5 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:6 Changed 5 years ago by SmileyChris

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

I'm marking as wontfix because the entire template is parsed no matter what conditionals there are, and this shouldn't change.
As already stated, the problem the user is having here is that {% load %} behaviour happens at parse time.

From the discussion in the list:

However, I don't believe loading a non-existent library should be a syntax error any more than accessing a non-existent key in a dictionary.

That's a fair suggestion. Something like {% load mylibrary fail-silently %} perhaps? (with my suggestion being fail-silently since isn't a valid module name, avoiding any chance of a clash)
But that's a separate issue and if someone feels strongly about it, they can open a new ticket.

comment:7 Changed 4 years ago by astopy

  • Resolution wontfix deleted
  • Status changed from closed to reopened

This should be fixed, because it really can cause problems. For example, I have the following in a template:

{% if category %}
{% block backtolink %}<a href="{% url community.views.category slug=category.slug page=frompage%}">&larr; Back to &quot;{{ category }}&quot;</a>{% endblock %}
{% endif %}

This raises NoReverseMatch when "category" is not set, so I'm now left having to add extra cruft to my views to get around it.

comment:8 Changed 4 years ago by lrekucki

From my understanding, blocks are replaced before conditions are evaluated by design. You can move the if tag to the block like this:

{% block backtolink %}
{% if category %} {# replace block with new content if there is a category #}
    <a href="{% url community.views.category slug=category.slug page=frompage%}">&larr; Back to &quot;{{ category }}&quot;</a>{% endblock %}
{% else %}{{ block.super }}{% endif %} {# leave the block alone #}
{% endblock %}

comment:9 Changed 4 years ago by SmileyChris

  • Resolution set to wontfix
  • Status changed from reopened to closed

Yes, it's by design and very fundamental to how the template system works.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.