[Django] #23913: = comparison does work in templates although it shouldn't
Django
---------------------------------+--------------------
Reporter: googol7 | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
There's no = operator documented:
https://docs.djangoproject.com/en/1.7/ref/templates/builtins/#operator
For comparisons we use the == operator as = does assign values in most
languages. In JavaScript the = assignment for example always returns true
here:
<script language="javascript">
a = 'foo';
b = 'bar';
if (a=b) {
alert('assignment always returns true here')
}
</script>
BUT in the Django templates = does work like a comparison operator:
{% if "a" = "a" %}TRUE but Django should throw an error because there's no
= operator{% endif %}
{% if "a" = "b" %}Django doesn't return True here{% endif %}
I think this is highly misleading and confusing.
--
Ticket URL: <https://code.djangoproject.com/ticket/23913>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: googol7 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: => 0
* needs_tests: => 0
* version: 1.7 => master
* needs_docs: => 0
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Hi,
I was surprised to see that the `=` operator does appear to work so I did
some digging:
* It only works when surrounded by spaces (eg `{% if 1 = 1 %}` works but
not `{% if 1=1 %}`
* It's sortof documented in the `{% if %}` tag's docstring [1]
* it's not actually documented publicly (as you mentionned)
* It's not tested (removing the parsing code for it [2] doesn't break any
test)
Given all this, I'm in favor of removing that "feature".
We still should go through the deprecation policy though, just in case
anyone might be relying on this undocumented and untested feature.
Thanks!
[1]
https://github.com/django/django/blob/c1552af1fe5832011e3b1c3e5b40c20ff3dbe6f9/django/template/defaulttags.py#L988
[2]
https://github.com/django/django/blob/c1552af1fe5832011e3b1c3e5b40c20ff3dbe6f9/django/template/smartif.py#L102
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:1>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by olasitarska):
* owner: nobody => olasitarska
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:2>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I'm not convinced we should make this change. It looks like something that
could break existing templates on stable websites.
I understand that the reporter was confused but I don't remember other
similar reports. The advantages seem quite small to me in comparison with
the drawbacks.
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:3>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
For the record, this feature was introduced with the "smart if"
(2c2f5aee4d44836779fcd74c7782368914f9cfd1) which landed in 1.2. Before
that, you had to use `{% ifequal %}`.
I'll also note that this syntax is invalid in jinja2.
However, if we want to keep it around for stability's sake, we should at
least have tests for it.
I remain doubtful about the need to document it though as I don't think we
should encourage the practice.
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:4>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by googol7):
BTW - I used this grep now to find occurrences in our code:
grep -e "if \w*[[:space:]]*=\{1\}[^=]" . -H -r -i -n -s -C 1
--include=*.{htm*,}
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:5>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by kezabelle):
Replying to [comment:3 aaugustin]:
> I'm not convinced we should make this change. It looks like something
that could break existing templates on stable websites.
> I understand that the reporter was confused but I don't remember other
similar reports.
I believe '''a''' change is required:
- Deprecate and eventually pull out the functionality (earliest
opportunity being 1.9) '''or'''
- Stabilise the parsing (`{% if 1 = 1 %}` vs `{% if 1=1 %}`) test and
document it, so that the principle of least surprise is minimised.
The fact that there may be code out there that might accidentally work
because of the right combination of typos (one equals sign, enough spaces)
seems a mis-step that could be rectified (or made authoritatively
correct).
It's also a quirk that doesn't fit with equality vs assignment in Python
(and thus Django).
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:6>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by kezabelle):
* cc: django@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:7>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by olasitarska):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/3660
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:8>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
At work we use " = " a bunch in our code for comparisons. I didn't realize
it wasn't documented, but I always found it annoying that it worked both
ways. Upgrading will be annoying, but deprecating it sounds like a good
idea to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:9>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:10>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* keywords: => 1.8-alpha
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/3887 Updated PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:11>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:12>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Component: Template system | Version: master
Keywords: 1.8-alpha | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d563e3be68369694a3bac1efd7779d8e03bb6a51"]:
{{{
#!CommitTicketReference repository=""
revision="d563e3be68369694a3bac1efd7779d8e03bb6a51"
Fixed #23913 -- Deprecated the `=` comparison in `if` template tag.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:13>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.8-alpha | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"9f51d0c86d9348349d921c393d6ffcbbfd521192"]:
{{{
#!CommitTicketReference repository=""
revision="9f51d0c86d9348349d921c393d6ffcbbfd521192"
Fixed test from refs #23913 when running tests in reverse.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:14>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.8-alpha | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by googol7):
https://docs.djangoproject.com/en/1.8/releases/1.8/#as-comparison-
operator-in-if-template-tag
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:15>
Django
Reporter: googol7 | Owner:
Type: | olasitarska
Cleanup/optimization | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.8-alpha | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2ccfac1a651ff35cd243cd272b2202ae1ccd5740" 2ccfac1a]:
{{{
#!CommitTicketReference repository=""
revision="2ccfac1a651ff35cd243cd272b2202ae1ccd5740"
Refs #23913 -- Removed support for a single equals sign in {% if %} tag.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23913#comment:16>