[Django] #18400: Unexpected {% if %} behavior
Django
---------------------------------+----------------------------------
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Severity: Normal | Keywords: template, if, length
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+----------------------------------
Given this simple template string:
{{{
{{ asd|length }}{% if asd|length > 1 %}Hi from an non existant variable!{%
endif %}
}}}
The output will be (if asd is NOT in the context):
{{{
0Hi from an non existant variable!
}}}
The reason is as follows:
* ''{{ asd|length }}'' resolves to ''!''|length'' which is 0
* ''asd|length'' inside the if resolves to ''None|length'' which results
in !'' (length returns !'' for every exception, len(None) isn't valid ;))
and !'' > 1 returns True in python :(
The easiest fix I can see is to make the if stuff perform usual template
variable resolving which would result in the following patch:
https://github.com/apollo13/django/commit/3782fc5862578951b7a6eb1eb97dca3a63267d44
The change breaks the template tests for me, hence I guess this ticket
might need a design decission since it breaks behavior (a bit at least
apperently ;))
--
Ticket URL: <https://code.djangoproject.com/ticket/18400>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, | Triage Stage:
length | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by apollo13):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Hmm, I have an odd issue with the testsuite here:
{{{
PYTHONPATH=.. ./runtests.py --settings=test_sqlite --verbosity 2
--failfast templates.Templates
}}}
returns without failures for the patch in question, running the full
testsuite results in errors in the testcase in question :(
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:1>
Django
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, | Triage Stage: Design
length | decision needed
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* stage: Unreviewed => Design decision needed
Comment:
It would be interesting to investigate why the `ignore_failures` argument
was added and what it does. Apparently it triggers two different
resolution modes. Isn't that inconsistency a bad idea in itself? If so,
shouldn't we get rid of this argument?
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:2>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
I don't know what the right solution is, but it's definitely a bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:3>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by luyikei):
I think that when TypeError happen, length should return 0.
I made pull request at https://github.com/django/django/pull/1633.
In shell if use my patch.
>>> from django import template
>>> t = template.Template("{{ asd|length }}{% if asd|length > 1 %}Hi from
an non existant variable!{% endif %}")
>>> c = template.Context({})
>>> print t.render(c)
0
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:4>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by luyikei):
* cc: luyikei (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:5>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by apollo13):
@luyikei the question is how and if that's better than my patch… How many
errors do you get in the testsuite with that change? Also any thoughts to
the comments from Aymeric?
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:6>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by luyikei):
I think it is natural that {{ asd|length }} returns 0.
because actually {{ asd|length }} returns 0.But in {% if asd|length > 1 %}
, asd|length returns "".
So it should return 0 I think.
And my patch returns no failures.
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:7>
Django
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by susan):
@luyikei I tested your PR against the test suites in tests/templates and
tests/test_templates and the test_template tests fail. Specifically,
template_tests/test.py in L620. I'll poke at the existing tests more later
today to see how I start changing them.
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:8>
Django
Reporter: apollo13 | Owner: susan
Type: Bug | Status: assigned
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by susan):
* cc: susan.tan.fleckerl@… (added)
* owner: nobody => susan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:9>
Django
Reporter: apollo13 | Owner: susan
Type: Bug | Status: assigned
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by anonymous):
Thanks you susan
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:10>
Django
Reporter: apollo13 | Owner: susan
Type: Bug | Status: assigned
Keywords: template, if, length | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* needs_docs: 0 => 1
Comment:
Since this is a backwards-incompatible change, it should be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:11>
Django
Reporter: apollo13 | Owner: susan
Component: Template system | Version: 1.4
Keywords: template, if, length | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"484f3edf1e79388f73dcb07e39d79d0c5029ae9e"]:
{{{
#!CommitTicketReference repository=""
revision="484f3edf1e79388f73dcb07e39d79d0c5029ae9e"
Fixed #18400 -- Modified length template filter to return 0 for unknown
variables.
Thanks Florian for the bug report, luyikei for the initial code patch, and
Bouke for the code review feedback.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18400#comment:12>