Opened 4 years ago

Closed 2 years ago

#18400 closed Bug (fixed)

Unexpected {% if %} behavior

Reported by: Florian Apolloner Owned by: Susan Tan
Component: Template system Version: 1.4
Severity: Normal Keywords: template, if, length
Cc: luyikei, susan.tan.fleckerl@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 ;))

Change History (12)

comment:1 Changed 4 years ago by Florian Apolloner

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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 :(

comment:2 Changed 4 years ago by Aymeric Augustin

Triage Stage: UnreviewedDesign decision needed

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?

comment:3 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

I don't know what the right solution is, but it's definitely a bug.

comment:4 Changed 3 years ago 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

comment:5 Changed 3 years ago by luyikei

Cc: luyikei added

comment:6 Changed 3 years ago by Florian Apolloner

@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?

comment:7 Changed 3 years ago 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.

comment:8 Changed 3 years ago by Susan Tan

@luyikei I tested your PR against the test suites in tests/templates and tests/test_templates. A few of the test_template tests fail. Specifically, template_tests/test.py in L620.

I made my own PR here: https://github.com/django/django/pull/1746/ Tests pass, but I am unsure if this fixes the bug. Please feel free to review and provide feedback.

Last edited 3 years ago by Susan Tan (previous) (diff)

comment:9 Changed 3 years ago by Susan Tan

Cc: susan.tan.fleckerl@… added
Owner: changed from nobody to Susan Tan
Status: newassigned

comment:10 Changed 3 years ago by anonymous

Thanks you susan

comment:11 Changed 3 years ago by Aymeric Augustin

Needs documentation: set

Since this is a backwards-incompatible change, it should be documented.

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 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.

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