Template handling of undefined variables

434 views
Skip to first unread message

Tim Martin

unread,
Jan 4, 2017, 10:30:07 PM1/4/17
to Django developers (Contributions to Django itself)
Hi all,

I've been looking at bug #24977 (Template variables with a value of
None are considered to be == to non-existent properties). The problem
is that in a template:

{% if user.pk == some_object.invalid_property %}
... this gets rendered when the user is logged out
{% endif %}

This is because undefined variables get silently expanded to None,
which can lead to a silent application bug, potentially a security
hole, if an object is refactored so the attribute name in the
comparison is no longer valid.

The problem is that None has a perfectly valid meaning to the
application in some circumstances (such as when it indicates a
logged-out user's PK), so its meaning gets overloaded by using it to
mean an invalid variable (which IMO should never compare equal to
anything else).

The suggestion on the ticket is to use an instance of a special
`Undefined` class to indicate an undefined variable, which could never
compare equal to anything else.

This sounds sensible, but in implementing it I got to thinking about
whether this can generalise and simplify. It seems like it might be
possible to combine both Engine.string_if_invalid and the
ignore_failures parameter to FilterExpression.resolve:

* The Undefined object can have a __str__() that returns the
  string_if_invalid value, so it acts like a string for the purposes
  of rendering.

* Returning the Undefined value is also sufficient to provide the
  behaviour we need in the ignore_failures case: the calling code can
  check for Undefined in the same case where it currently checks for
  None.

Overall the code is simpler, because a bunch of code paths are
eliminated. However, there are 2 issues:

1) There are test cases where we have templates that should treat "x
   is y" as True where x and y are both undefined. This means that
   (unless we want to change the behaviour) we have to return multiple
   copies of a single Undefined instance, rather than a fresh instance
   each time. And this doesn't work in the case where
   string_if_invalid contains a %s and the string value should have
   the variable name expanded into it.

   I don't see any way to satisfy both these requirements. Is it
   reasonable to relax the requirement that "x is y" should be treated
   as True when both variables are undefined?

2) There appears to be an inconsistency in the default_if_none
   modifier. If I have a template with

   x|default_if_none:y = {{x|default_if_none:y}}
   {% if x|default_if_none:y %}
   x|default_if_none:y is apparently True
   {% endif %}

   with y defined to True and x undefined, then it produces:

   x|default_if_none:y =
   x|default_if_none:y is apparently True
  
   IOW it appears that the default_if_none doesn't cause y's value to
   be used in the case where the variable is being printed, even
   though it causes the expression to have y's value when evaluated in
   an if. I think this is something to do with the fact that the two
   ways of handling failures (ignore_failures and string_if_invalid)
   do different things.

   I don't see a way to make backward compatible code here.

   I think this is just a bug, does anyone think otherwise?

Sorry for the lengthy email. Thanks for any input.

Tim

Carl Meyer

unread,
Jan 4, 2017, 11:15:31 PM1/4/17
to django-d...@googlegroups.com
Hi Tim,

On 01/04/2017 03:39 PM, Tim Martin wrote:
> I've been looking at bug #24977 (Template variables with a value of
> None are considered to be == to non-existent properties).
...
> The suggestion on the ticket is to use an instance of a special
> `Undefined` class to indicate an undefined variable, which could never
> compare equal to anything else.
>
> This sounds sensible, but in implementing it I got to thinking about
> whether this can generalise and simplify. It seems like it might be
> possible to combine both Engine.string_if_invalid and the
> ignore_failures parameter to FilterExpression.resolve:
>
> * The Undefined object can have a __str__() that returns the
> string_if_invalid value, so it acts like a string for the purposes
> of rendering.
>
> * Returning the Undefined value is also sufficient to provide the
> behaviour we need in the ignore_failures case: the calling code can
> check for Undefined in the same case where it currently checks for
> None.
>
> Overall the code is simpler, because a bunch of code paths are
> eliminated.

Sounds great!

> However, there are 2 issues:
>
> 1) There are test cases where we have templates that should treat "x
> is y" as True where x and y are both undefined.

Really? Is this an accidental side effect of some historical change, or
really the intent of the test? That behavior seems buggy to me; more
likely to be a bug-magnet than useful.

...
> I don't see any way to satisfy both these requirements. Is it
> reasonable to relax the requirement that "x is y" should be treated
> as True when both variables are undefined?

Seems reasonable to me.

> 2) There appears to be an inconsistency in the default_if_none
> modifier. If I have a template with
>
> x|default_if_none:y = {{x|default_if_none:y}}
> {% if x|default_if_none:y %}
> x|default_if_none:y is apparently True
> {% endif %}
>
> with y defined to True and x undefined, then it produces:
>
> x|default_if_none:y =
> x|default_if_none:y is apparently True
>
> IOW it appears that the default_if_none doesn't cause y's value to
> be used in the case where the variable is being printed, even
> though it causes the expression to have y's value when evaluated in
> an if. I think this is something to do with the fact that the two
> ways of handling failures (ignore_failures and string_if_invalid)
> do different things.
>
> I don't see a way to make backward compatible code here.
>
> I think this is just a bug, does anyone think otherwise?

I agree.

Carl

signature.asc

Tim Martin

unread,
Jan 5, 2017, 12:51:39 PM1/5/17
to Django developers (Contributions to Django itself)


On Thursday, 5 January 2017 04:15:31 UTC, Carl Meyer wrote:
Hi Tim,

On 01/04/2017 03:39 PM, Tim Martin wrote:
 
> 1) There are test cases where we have templates that should treat "x
>    is y" as True where x and y are both undefined.

Really? Is this an accidental side effect of some historical change, or
really the intent of the test? That behavior seems buggy to me; more
likely to be a bug-magnet than useful.

There are a couple of test cases in test_if.py: test_if_is_both_variables_missing and test_if_is_not_both_variables_missing. These are verifying this specific case, so they haven't been introduced by accident. I haven't got as far as figuring out whether it was a fully thought through decision or whether someone just created these cases for completeness.
 
...
>    I don't see any way to satisfy both these requirements. Is it
>    reasonable to relax the requirement that "x is y" should be treated
>    as True when both variables are undefined?

Seems reasonable to me.

> 2) There appears to be an inconsistency in the default_if_none
>    modifier.
[...]
>    I think this is just a bug, does anyone think otherwise?

I agree.

OK. I think I'll open this as a separate bug and solve that first.

Tim

Alasdair Nicol

unread,
Jan 6, 2017, 5:15:22 AM1/6/17
to Django developers (Contributions to Django itself)
Hi,

On Thursday, 5 January 2017 17:51:39 UTC, Tim Martin wrote:


On Thursday, 5 January 2017 04:15:31 UTC, Carl Meyer wrote:
Hi Tim,

On 01/04/2017 03:39 PM, Tim Martin wrote:
 
> 1) There are test cases where we have templates that should treat "x
>    is y" as True where x and y are both undefined.

Really? Is this an accidental side effect of some historical change, or
really the intent of the test? That behavior seems buggy to me; more
likely to be a bug-magnet than useful.

There are a couple of test cases in test_if.py: test_if_is_both_variables_missing and test_if_is_not_both_variables_missing. These are verifying this specific case, so they haven't been introduced by accident. I haven't got as far as figuring out whether it was a fully thought through decision or whether someone just created these cases for completeness.

I added the tests in ticket 26479 (see comment 5 onwards) [1]. I believe I added them for consistency with == and != operators, which have similar tests [2], [3] (trickier to spot because they are numbered). I apologise if adding the tests has made it harder to improve the behaviour of the tag. 

cheers,
Alasdair

Adam Johnson

unread,
Jan 6, 2017, 5:28:14 AM1/6/17
to django-d...@googlegroups.com
I apologise if adding the tests has made it harder to improve the behaviour of the tag. 

I don't think you have anything to apologise for! More tests is always better. This has clarified the current behaviour 👌 

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/96f764fa-25c6-4104-9b6b-8226e3b6bd8f%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.



--
Adam

Tim Martin

unread,
Jan 7, 2017, 12:21:33 PM1/7/17
to Django developers (Contributions to Django itself)
On Friday, 6 January 2017 10:15:22 UTC, Alasdair Nicol wrote:
Hi,

On Thursday, 5 January 2017 17:51:39 UTC, Tim Martin wrote:


On Thursday, 5 January 2017 04:15:31 UTC, Carl Meyer wrote:
Hi Tim,

On 01/04/2017 03:39 PM, Tim Martin wrote:
 
> 1) There are test cases where we have templates that should treat "x
>    is y" as True where x and y are both undefined.

Really? Is this an accidental side effect of some historical change, or
really the intent of the test? That behavior seems buggy to me; more
likely to be a bug-magnet than useful.

There are a couple of test cases in test_if.py: test_if_is_both_variables_missing and test_if_is_not_both_variables_missing. These are verifying this specific case, so they haven't been introduced by accident. I haven't got as far as figuring out whether it was a fully thought through decision or whether someone just created these cases for completeness.

I added the tests in ticket 26479 (see comment 5 onwards) [1]. I believe I added them for consistency with == and != operators, which have similar tests [2], [3] (trickier to spot because they are numbered). I apologise if adding the tests has made it harder to improve the behaviour of the tag.

Thanks for the background info Alasdair, that saved me lots of time hunting around.

I agree you were right to add the tests, which were useful since without them I wouldn't have realised the potential behaviour change I was introducing. As I see it, these tests don't in themselves form a reason not to change the behaviour (since AIUI you originally added them for completeness, rather than because this specific behaviour was desired).

Does anyone think changing the behaviour of {% if a is None %} is the wrong thing to do? I realise there can potentially be template code out there relying on this, but after a quick scout through the documentation I can't see anywhere that the behaviour on undefined variables is specified officially.

For what it's worth, the same issue doesn't come up with ==, because although the existing template behaviour has the same pattern, it's possible to override == and != so that the Undefined object gives the same behaviour as None did before. It's only for 'is' that this can't be achieved. I don't know how for to go with this: preserving the existing == behaviour but changing it for 'is' leaves the two operations superficially inconsistent (though there's nothing fundamentally wrong with two things that satisfy equality but not 'is'). I can see a couple of alternatives (in all cases all variables are undefined):

* "x is y" is false, but "x == y" is true and "x != y" is false (minimal difference from the current behaviour)
* "x is y" and "x == y" are both false, "x != y" is true (probably the most mutually consistent?)
* "x is y", "x == y" and "x != y" are all false (the SQL NULL alternative - I'm not sure if I like this, but it has the merit that wrongly skipping sections of a template is usually less bad than wrongly including parts of a template)

Tim
 

Tim Graham

unread,
Feb 17, 2017, 6:34:27 PM2/17/17
to Django developers (Contributions to Django itself)
After reviewing the pull request, I wonder if it would be better to raise exceptions when comparing nonexistent variables in {% if %} rather than altering the behavior. For existing projects, this would prevent possible inadvertent information leakage if some {% if %} starts evaluating differently than it did before. While the current behavior may not be documented, it seems risky to silently change it.

There are examples of that behavior change on the PR: https://github.com/django/django/pull/7901

Adam Johnson

unread,
Feb 19, 2017, 4:56:24 AM2/19/17
to django-d...@googlegroups.com
+1 for more obvious errors, silently changing the behaviour could indeed lead to unconsidered security holes like

{% if user is None %}
non-sensitive information
{% else %}
sensitive information
{% endif %}

...which doesn't seem like an unrealistic template snippet. We all know variables can go missing in refactorings.

Another option, perhaps not feasible to implement, would be deprecating the old behaviour, similar to the previous change in url with something like:

{% load undefined_vars from future %}

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

For more options, visit https://groups.google.com/d/optout.



--
Adam

Luke Plant

unread,
Feb 20, 2017, 6:42:59 AM2/20/17
to django-d...@googlegroups.com
On 19/02/17 12:55, Adam Johnson wrote:
+1 for more obvious errors, silently changing the behaviour could indeed lead to unconsidered security holes like

{% if user is None %}
non-sensitive information
{% else %}
sensitive information
{% endif %}

...which doesn't seem like an unrealistic template snippet. We all know variables can go missing in refactorings.

Another option, perhaps not feasible to implement, would be deprecating the old behaviour, similar to the previous change in url with something like:

{% load undefined_vars from future %}

I agree there are a lot of potential security/correctness issues with this, it is potentially quite a big change (though very helpful IMO).

A different approach to introducing it might be a setting, possibly an option to the Django template engine instead - https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-TEMPLATES-OPTIONS . I think this would be more appropriate for something that is more of a global behaviour issue, more practical than having to add hundreds of 'load from future' tags, plus it would then parallel other similar settings like 'string_if_invalid'. In the next version of Django the option would default to False (i.e. old behaviour), but raise a deprecation warning, in future versions it would simply be True, and raise an error if someone tries to pass False (but allow True, for the sake of apps that are spanning multiple Django versions).

This would allow people to test their site with the new mechanism and have time to fix issues, which can be especially important for 3rd party Django apps.

Ideally there would be some way to instrument code and see if the output would be different with the new behaviour, but I can't think of an easy way to do this.

Luke

Marc Tamlyn

unread,
Feb 20, 2017, 6:54:37 AM2/20/17
to django-d...@googlegroups.com
+1 to not having to add (and then remove later) a {% load %} tag to every template - that was seriously dull with the URL change.

Marc

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

Tim Martin

unread,
Feb 25, 2017, 9:54:10 AM2/25/17
to Django developers (Contributions to Django itself)
Actually, I can imagine that the option might be worth keeping permanently. I think both the "exception on use of undefined" and "treat undefined as different from all other objects" would both be valid modes. Treating undefined as None is probably only justifiable for backward compatibility, though. I'll rework the patch to support a setting unless anyone comes up with a better idea.

I'm not sure I like the proposal of throwing an exception on `if` but not in other cases. It seems more consistent to just raise an exception on any use of an undefined variable.

Tim


On Monday, 20 February 2017 11:54:37 UTC, Marc Tamlyn wrote:
+1 to not having to add (and then remove later) a {% load %} tag to every template - that was seriously dull with the URL change.

Marc
On 20 February 2017 at 11:42, Luke Plant <L.Pla...@cantab.net> wrote:
On 19/02/17 12:55, Adam Johnson wrote:
+1 for more obvious errors, silently changing the behaviour could indeed lead to unconsidered security holes like

{% if user is None %}
non-sensitive information
{% else %}
sensitive information
{% endif %}

...which doesn't seem like an unrealistic template snippet. We all know variables can go missing in refactorings.

Another option, perhaps not feasible to implement, would be deprecating the old behaviour, similar to the previous change in url with something like:

{% load undefined_vars from future %}

I agree there are a lot of potential security/correctness issues with this, it is potentially quite a big change (though very helpful IMO).

A different approach to introducing it might be a setting, possibly an option to the Django template engine instead - https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-TEMPLATES-OPTIONS . I think this would be more appropriate for something that is more of a global behaviour issue, more practical than having to add hundreds of 'load from future' tags, plus it would then parallel other similar settings like 'string_if_invalid'. In the next version of Django the option would default to False (i.e. old behaviour), but raise a deprecation warning, in future versions it would simply be True, and raise an error if someone tries to pass False (but allow True, for the sake of apps that are spanning multiple Django versions).

This would allow people to test their site with the new mechanism and have time to fix issues, which can be especially important for 3rd party Django apps.

Ideally there would be some way to instrument code and see if the output would be different with the new behaviour, but I can't think of an easy way to do this.

Luke

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Tim Graham

unread,
Feb 25, 2017, 2:10:17 PM2/25/17
to Django developers (Contributions to Django itself)
I think any use of undefined template variables should raise an exception. In the long run, keeping a setting to allow some other behavior seems confusing and, considering the case of templates that might be reused in different projects with different settings, even dangerous.

Karen Tracey

unread,
Feb 25, 2017, 4:44:30 PM2/25/17
to django-d...@googlegroups.com
On Sat, Feb 25, 2017 at 2:10 PM, Tim Graham <timog...@gmail.com> wrote:
I think any use of undefined template variables should raise an exception. In the long run, keeping a setting to allow some other behavior seems confusing and, considering the case of templates that might be reused in different projects with different settings, even dangerous.

I think I'm confused...Django templates have allowed use of undefined variables and documented their use as evaluating to the empty string for as long as I recall. Wouldn't a change to instead raise exceptions be a major backwards-incompatibility?

https://docs.djangoproject.com/en/1.7/topics/templates/#variables said "If you use a variable that doesn’t exist, the template system will insert the value of the TEMPLATE_STRING_IF_INVALID setting, which is set to '' (the empty string) by default."

https://docs.djangoproject.com/en/dev/ref/templates/api/#invalid-template-variables has refined that doc to note that the behavior is slightly different in some tags.

Are we really considering changing this behavior to now raise exceptions?

Tim Graham

unread,
Feb 25, 2017, 7:28:18 PM2/25/17
to Django developers (Contributions to Django itself)
My proposal was only for the use of undefined variables in template tags. I didn't realize that the behavior of undefined variables in some tags resolving to None is documented, but I still think it's a useful change to raise an exception instead. The philosophy that template tags shouldn't raise exceptions is more unhelpful than helpful in my experience. I think the change would be consistent with the deprecation that starts in Django 1.11 to change {% include %} not to silencing exceptions and render an empty string, for example.

Melvyn Sopacua

unread,
Feb 25, 2017, 7:51:20 PM2/25/17
to django-d...@googlegroups.com

On Saturday 25 February 2017 16:28:17 Tim Graham wrote:

> My proposal was only for the use of undefined variables in template

> tags. I didn't realize that the behavior of undefined variables in

> some tags resolving to None is documented, but I still think it's a

> useful change to raise an exception instead. The philosophy that

> template tags shouldn't raise exceptions is more unhelpful than

> helpful in my experience.

 

I sincerely hope that the change only affects DEBUG mode (including the include change). But once the templates are validated and working as they should, they should not generate exceptions unless Django provides a way to catch them. For example, if a service is unavailable that makes up a small part of a page, I do not want the whole page to crash or information to leak.

 

--

Melvyn Sopacua

Fred Stluka

unread,
Feb 25, 2017, 8:21:14 PM2/25/17
to django-d...@googlegroups.com
Tim and others,

+1 for raising an exception.

Specifically...

I agree that use of undefined variables should raise an exception.
The incompatibility with previous versions will mostly catch errors
that have been going undetected.

Personally, I find it very hard to detect such bugs when reviewing
code written by myself or other team members, especially since we
make heavy use of template inheritance, and we reuse the same
templates from multiple views.  So, it's not easy to spot a case
where we forgot to pass a value to a template.

I think Django should at least offer this as an option.

--Fred

Fred Stluka -- mailto:fr...@bristle.com -- http://bristle.com/~fred/
Bristle Software, Inc -- http://bristle.com -- Glad to be of service!
Open Source: Without walls and fences, we need no Windows or Gates.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

Karen Tracey

unread,
Feb 25, 2017, 9:37:20 PM2/25/17
to django-d...@googlegroups.com
On Sat, Feb 25, 2017 at 8:21 PM, Fred Stluka <fr...@bristle.com> wrote:
I agree that use of undefined variables should raise an exception.
The incompatibility with previous versions will mostly catch errors
that have been going undetected.

I disagree, unless you are limiting this change specifically to arguments to non-builtin template tags. (Which I thought already raised exceptions, so I'm still confused here as to what change is being proposed.)

Given the documented behavior of evaluating undefined variables to empty strings its been common practice to use:

{% if var_that_may_not_exist %}
...stuff that should only show when var exists...
{% endif %}

or to include {{ var_that_may_not_exist }} somewhere in a template and rely on it evaluating to an empty string. (admin itself was documented as not working correctly if you set the setting to evaluate undefined to something other than empty string).

Changing either of these now to raise exceptions would be a huge backwards incompatibility going against previously documented behavior.  Please don't do that.

It may well be friendlier to developers (I've never been a fan of the "templates shouldn't raise exceptions" philosophy) but the fact is for many years now it's been perfectly acceptable to use what might be undefined variables in templates and the behavior has been documented as to how it will work. Changing that now to start raising exceptions would be incredibly unfriendly to existing code that has been written to rely on it.

Karen

Florian Apolloner

unread,
Feb 26, 2017, 4:16:55 AM2/26/17
to Django developers (Contributions to Django itself)
As much as I'd like variables to raise an error if they cannot resolve, I am with Karen here -- the backwards compatibility considerations should take priority here. At least we should have a possibility in the templates to check if variables are undefined before we start raising exceptions.

Cheers,
Florian

Luke Plant

unread,
Feb 27, 2017, 5:36:09 AM2/27/17
to django-d...@googlegroups.com

On 26/02/17 00:44, Karen Tracey wrote:
On Sat, Feb 25, 2017 at 2:10 PM, Tim Graham <timog...@gmail.com> wrote:
I think any use of undefined template variables should raise an exception. In the long run, keeping a setting to allow some other behavior seems confusing and, considering the case of templates that might be reused in different projects with different settings, even dangerous.

I think I'm confused...Django templates have allowed use of undefined variables and documented their use as evaluating to the empty string for as long as I recall. Wouldn't a change to instead raise exceptions be a major backwards-incompatibility?

https://docs.djangoproject.com/en/1.7/topics/templates/#variables said "If you use a variable that doesn’t exist, the template system will insert the value of the TEMPLATE_STRING_IF_INVALID setting, which is set to '' (the empty string) by default."

This behaviour applies only to what happens when the variable is rendered. In the context of `if` tags, undefined variables get converted to `None`. This behaviour is documented - https://docs.djangoproject.com/en/dev/ref/templates/api/#how-invalid-variables-are-handled but perhaps not in as much detail as necessary.

The question is about changing this, especially due to templates like this:

    {% if user.role == roles.staff %}
        [sensitive information here]
    {% endif %}

If:
1) you forget to provide both user and role to template context or
2) 'role' or 'staff' are invalid attributes, or
3) 'role' returns None but 'staff' is an invalid attribute, for example,
4) various combinations of these

then this results in the sensitive info being shown, because `None == None`. The proposal is to introduce a value that doesn't not compare equal to itself and avoid this kind of issue.

Having thought about it more, I've realised that this really isn't going to work at all.

First attempt:

    class Undefined1(object):
        def __eq__(self, other):
            return False
   
If we use this, then consider the following template code:

    {% if user.role != roles.staff %}
        This info is private, sorry
    {% else %}
        [sensitive information here]
    {% endif %}


The default implementation of != means we will again get the sensitive data shown for some of the error situations given above (e.g. 1 and 2). Second attempt:

    class Undefined2(object):
        def __eq__(self, other):
            return False
        def __new__(self, other):
            return False

This object is neither equals nor not-equals to itself or anything else. But consider this template:


    {% if user.role == roles.customer %}
        This info is private, sorry
    {% else %}
        [sensitive information here]
    {% endif %}

With `Undefined2` being used for invalid attributes, we again get the sensitive information being shown in the case of developer error and missing attributes. Plus, we now have the situation that `{% if a != b %}` is not the same as `{% if not a == b %}`, which is very confusing behaviour for most people.

In other words, we are getting into the territory of needing a value that behaves like NULL in SQL. Even if there were no implementation issues (and there will be lots, because operators like 'or' 'and' etc. would need to cope with this, not to mention 3rd party code), and there were no backwards compatibility issues (there are lots), this would be extremely dubious to me, because ternary logic is extremely tricky.

The only sensible alternative to current behaviour is to raise exceptions, but huge backwards incompatibility issues I think rule this out. If I were to start again, I think I would make the template system not silence any errors you would normally get in Python, but just have some special syntax for converting non-existent data or attributes into None. But I don't have that time machine.

Regards,

Luke

Luke Plant

unread,
Feb 27, 2017, 5:43:02 AM2/27/17
to django-d...@googlegroups.com



On 05/01/17 02:39, Tim Martin wrote:
2) There appears to be an inconsistency in the default_if_none
   modifier. If I have a template with

   x|default_if_none:y = {{x|default_if_none:y}}
   {% if x|default_if_none:y %}
   x|default_if_none:y is apparently True
   {% endif %}

   with y defined to True and x undefined, then it produces:

   x|default_if_none:y =
   x|default_if_none:y is apparently True
  
   IOW it appears that the default_if_none doesn't cause y's value to
   be used in the case where the variable is being printed, even
   though it causes the expression to have y's value when evaluated in
   an if. I think this is something to do with the fact that the two
   ways of handling failures (ignore_failures and string_if_invalid)
   do different things.

   I don't see a way to make backward compatible code here.

   I think this is just a bug, does anyone think otherwise?

This seems to be working exactly as it should.

In the context of template variable interpolation i.e. `{{ }}` syntax, it is defined that `x` in this case will get converted to the empty string (or your 'string_if_invalid' setting) - https://docs.djangoproject.com/en/dev/ref/templates/api/#how-invalid-variables-are-handled - unlike the case for `if` tags.

When this value (the empty string) is passed to `default_if_none`, the value is not changed, since it is not None. See https://docs.djangoproject.com/en/1.10/ref/templates/builtins/#default-if-none

The behaviour here follows 100% logically from the documented behaviour of the two components.

Regards,

Luke



Sorry for the lengthy email. Thanks for any input.

Tim
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

Shai Berger

unread,
Feb 27, 2017, 8:26:21 PM2/27/17
to django-d...@googlegroups.com
On Sunday 26 February 2017 11:16:54 Florian Apolloner wrote:
> As much as I'd like variables to raise an error if they cannot resolve, I
> am with Karen here -- the backwards compatibility considerations should
> take priority here. At least we should have a possibility in the templates
> to check if variables are undefined before we start raising exceptions.
>

I concur.

Adam's {% load undefined_vars from future %} sounds like a good idea -- with
two caveats:

1) We shouldn't make it part of a deprecation plan (at least, not yet)

2) We should consider how to enable it to affect many templates from one place.
My first thought: If included in a template, it should apply (recursively) to
any template which {% extends %} it, and to any {% include %}'d template.

Then it's not "modify each template" to use, and it isn't all-or-nothing
unless you want it to be.

My 2 cents,
Shai

Marten Kenbeek

unread,
Feb 28, 2017, 7:24:13 AM2/28/17
to Django developers (Contributions to Django itself)
What about adding a filter |defined that returns True if a variable is defined, False otherwise? It may not solve any problems when it's left implicit, but at least it allows users the explicitly define the behaviour for non-existing template variables, and it makes the intention of the template code crystal clear. And of course it's fully backwards compatible. The drawback is perhaps the resulting verbosity of if statements:

    {% if user.role|defined and roles.staff|defined and user.role == roles.staff %}

        [sensitive information here]
    {% endif %}

Marten

Luke Plant

unread,
Feb 28, 2017, 8:39:21 AM2/28/17
to django-d...@googlegroups.com



On 28/02/17 15:24, Marten Kenbeek wrote:
What about adding a filter |defined that returns True if a variable is defined, False otherwise? It may not solve any problems when it's left implicit, but at least it allows users the explicitly define the behaviour for non-existing template variables, and it makes the intention of the template code crystal clear. And of course it's fully backwards compatible. The drawback is perhaps the resulting verbosity of if statements:

    {% if user.role|defined and roles.staff|defined and user.role == roles.staff %}
        [sensitive information here]
    {% endif %}


The only way to do it would be to hard code this filter into the template engine, because otherwise the 'defined' filter runs too late to know whether the variable is defined.  This is really hacky and magic.

The alternative would be for this filter to take a string e.g.:



    {% if "user.role"|defined and "roles.staff"|defined and user.role == roles.staff %}
        [sensitive information here]
    {% endif %}

As an analogy, in Python you can't write:

    if hasattr(foo.bar)

you have to write:

    if hasattr(foo, 'bar')

Another alternative would be to have some special syntax that could check for defined variables e.g.

   {% if user.role? and roles.staff? and user.role == roles.staff %}

My own preference, if I had a time machine, would be to have missing data/attributes just raise NameError/KeyError/AttributeError, and then have special syntax for replacing missing values with None - like https://msdn.microsoft.com/en-us/library/dn986595(v=vs.140).aspx .

However, I think that any option involving special syntax here has already got way too complex for the design philosophy of Django templates - to be simpler than normal Python code, and more designer friendly - this would take us in the opposite direction.

Luke

Tim Martin

unread,
Feb 28, 2017, 5:53:36 PM2/28/17
to Django developers (Contributions to Django itself)


On Tuesday, 28 February 2017 13:39:21 UTC, Luke Plant wrote:



On 28/02/17 15:24, Marten Kenbeek wrote:
What about adding a filter |defined that returns True if a variable is defined, False otherwise? It may not solve any problems when it's left implicit, but at least it allows users the explicitly define the behaviour for non-existing template variables, and it makes the intention of the template code crystal clear. And of course it's fully backwards compatible. The drawback is perhaps the resulting verbosity of if statements:

    {% if user.role|defined and roles.staff|defined and user.role == roles.staff %}
        [sensitive information here]
    {% endif %}


The only way to do it would be to hard code this filter into the template engine, because otherwise the 'defined' filter runs too late to know whether the variable is defined.  This is really hacky and magic.


Is that true? In my patch, the undefined variable gets expanded into an UndefinedVariable sentinel object, which can be detected by the defined modifier, and can compare == to None (or not, if you wish). Other than the result of the 'is' operator, I think you can make this fully backward compatible if you choose. Whether or not this is desirable is another question, of course.

Tim

Tim Martin

unread,
Feb 28, 2017, 6:07:36 PM2/28/17
to Django developers (Contributions to Django itself)


On Monday, 27 February 2017 10:43:02 UTC, Luke Plant wrote:



On 05/01/17 02:39, Tim Martin wrote:
2) There appears to be an inconsistency in the default_if_none
   modifier. If I have a template with

   x|default_if_none:y = {{x|default_if_none:y}}
   {% if x|default_if_none:y %}
   x|default_if_none:y is apparently True
   {% endif %}

   with y defined to True and x undefined, then it produces:

   x|default_if_none:y =
   x|default_if_none:y is apparently True
  
   IOW it appears that the default_if_none doesn't cause y's value to
   be used in the case where the variable is being printed, even
   though it causes the expression to have y's value when evaluated in
   an if. I think this is something to do with the fact that the two
   ways of handling failures (ignore_failures and string_if_invalid)
   do different things.

   I don't see a way to make backward compatible code here.

   I think this is just a bug, does anyone think otherwise?

This seems to be working exactly as it should.

In the context of template variable interpolation i.e. `{{ }}` syntax, it is defined that `x` in this case will get converted to the empty string (or your 'string_if_invalid' setting) - https://docs.djangoproject.com/en/dev/ref/templates/api/#how-invalid-variables-are-handled - unlike the case for `if` tags.

When this value (the empty string) is passed to `default_if_none`, the value is not changed, since it is not None. See https://docs.djangoproject.com/en/1.10/ref/templates/builtins/#default-if-none

The missing piece of information for me (and even reading the docs it's far from obvious) is that undefined variables mean two different things in the different contexts.

The value *is* changed in the second case, because it *is* None. Undefined values are '' in {{ }} context but are None in {% %} context. Maybe that's obvious to you, but it's not to me. In fact, after a few minutes searching I still can't find where this is mentioned in the documentation. Only the vague statement that invalid variables are "generally" replaced with string_if_invalid.

Tim

Luke Plant

unread,
Mar 1, 2017, 1:56:07 PM3/1/17
to django-d...@googlegroups.com

In the context of template variable interpolation i.e. `{{ }}` syntax, it is defined that `x` in this case will get converted to the empty string (or your 'string_if_invalid' setting) - https://docs.djangoproject.com/en/dev/ref/templates/api/#how-invalid-variables-are-handled - unlike the case for `if` tags.

When this value (the empty string) is passed to `default_if_none`, the value is not changed, since it is not None. See https://docs.djangoproject.com/en/1.10/ref/templates/builtins/#default-if-none

The missing piece of information for me (and even reading the docs it's far from obvious) is that undefined variables mean two different things in the different contexts.

The value *is* changed in the second case, because it *is* None. Undefined values are '' in {{ }} context but are None in {% %} context. Maybe that's obvious to you, but it's not to me. In fact, after a few minutes searching I still can't find where this is mentioned in the documentation. Only the vague statement that invalid variables are "generally" replaced with string_if_invalid.

I meant above that in the {{ }} context 'default_if_none' doesn't return the default value, but just returns the empty string input value.

I also had to read the docs to understand the behaviour - it is mentioned in the 3rd paragraph down in the docs linked above, "How invalid variables are handled" - the `if`, `for` and `regroup` tags are specifically mentioned.  I think the docs could be clearly about how things work with variables passed to template tags etc. "In general" as you say is rather vague. Definitely room for improvement here!


Luke

Luke Plant

unread,
Mar 1, 2017, 2:52:22 PM3/1/17
to django-d...@googlegroups.com
I was talking about with the current behaviour of template engine i.e. without changing how undefined variables are handled. For this to work as you proposed, you would have to be passing the UndefinedVariable object into these filters. Having checked out your branch, I can see that is what you are doing.

This is a really big backwards incompatibility, as far as I can see. It means that any filter may now get passed `UndefinedVariable` instead of being passed `None` or string_if_invalid. So for example, the following template behaves oppositely with your patch, if variable 'missing' is undefined:

{% if missing|yesno == "maybe" %}true{% else %}false{% endif %}

There are likely many other filters that will behave differently e.g.:

{{ missing|add:"x" }}

Without the patch, if 'missing' is undefined this returns "x", but with the patch it returns "". It's not just builtin filters I'm thinking about, it's everyone else's that would also have to be made aware of this new false-y value that it will start receiving. This is a really big backwards incompatible change, I don't see how we could justify this. If we were starting from scratch it would be another matter.

Luke

Tim Martin

unread,
Mar 6, 2017, 4:19:36 PM3/6/17
to Django developers (Contributions to Django itself)


On Wednesday, 1 March 2017 19:52:22 UTC, Luke Plant wrote:

This is a really big backwards incompatibility, as far as I can see. It means that any filter may now get passed `UndefinedVariable` instead of being passed `None` or string_if_invalid. So for example, the following template behaves oppositely with your patch, if variable 'missing' is undefined:

{% if missing|yesno == "maybe" %}true{% else %}false{% endif %}

There are likely many other filters that will behave differently e.g.:

{{ missing|add:"x" }}

Without the patch, if 'missing' is undefined this returns "x", but with the patch it returns "". It's not just builtin filters I'm thinking about, it's everyone else's that would also have to be made aware of this new false-y value that it will start receiving. This is a really big backwards incompatible change, I don't see how we could justify this. If we were starting from scratch it would be another matter.

Yes, I can see the issue and I'm open to the idea of abandoning this patch. If we're not willing to tolerate substantial changes to the behaviour then I don't think there's a way forward with this patch - changing the semantics of undefined variables is pretty much the whole point of it.

In fairness, it's possible to change all the built-in filters to preserve existing behaviour (I've already done it for everything that's documented or covered by unit tests, though I guess I missed 'yesno', which should clearly return maybe for an undefined variable) and any third-party filters that are relying on the semantics of undefined variables (outside of 'if', 'for' and 'regroup') are based on undefined behaviour. Imposing changes on third-party filters in a major release of Django doesn't seem too unreasonable. But I don't know that there's a strong enough positive case for making the change. I don't care much about it, I only picked up this bug because it looked like an easy way to learn the template system.

For the record, I don't like the design of treating undefined values as None, and I really don't like the design of them having different values in different contexts. But I guess that's the design we're stuck with.

Tim
Reply all
Reply to author
Forward
0 new messages