Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18526 closed Bug (wontfix)

{% with %} template tag shows strange behaviour if TEMPLATE_STRING_IF_INVALID is non-empty.

Reported by: gregmuellegger Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords: with TEMPLATE_STRING_IF_INVALID
Cc: regebro Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The value that should be assigned to a new one using the with template tag breaks if the value is not existend. It uses the value from TEMPLATE_STRING_IF_INVALID to set the new one. But this might break template code in unexpected ways since this usually non-existend variable now evaluates as True in an if statement.

See the following snippet:

# in settings.py
TEMPLATE_STRING_IF_INVALID = '<INVALID>'
In [16]: Template('{% with not_exist as value %}{% if value %}Hello {{ value }}{% endif %}{% endwith %}').render(Context())
Out[16]: u'Hello &lt;INVALID&gt;'

Attachments (2)

t18526__failing_test_case.diff (851 bytes) - added by gregmuellegger 3 years ago.
t18526__failing_test_case.2.diff (1.9 KB) - added by gregmuellegger 3 years ago.
This problem affects even more tags …

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by gregmuellegger

comment:1 Changed 3 years ago by gregmuellegger

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

Infact this issue has an even broader problem.

The problem lies in the FilterExpression class that takes care of most argument handling of template tags.
This will set all non-existing variables to TEMPLATE_STRING_IF_INVALID before passing it into any filter.

This breaks a few other tags as well:

{% cycle does_not_exist|default:"empty" %}

will output:
<INVALID> (the TEMPLATE_STRING_IF_INVALID setting)

Changed 3 years ago by gregmuellegger

This problem affects even more tags …

comment:2 Changed 3 years ago by gregmuellegger

The desired behaviour can be achieved if the TEMPLATE_STRING_IF_INVALID value evaluates to False, like:

class Invalid(unicode):
    def __nonzero__(self):
        return False

TEMPLATE_STRING_IF_INVALID = Invalid(u'<INVALID>')

The issue is already documented here:
https://docs.djangoproject.com/en/dev/ref/templates/api/#invalid-template-variables

Propably thats some legacy from eeearly versions. I don't like that and think
thats broken. The template runtime semantics shouldn't change depending on the
TEMPLATE_STRING_IF_INVALID setting. Maybe thats worth documenting even better.

I think using variables in the template that are not present in the context is
a valid use case. Especially for the "override defaults" example from above:

{% with help_text|default:field.help_text %}
    {% if help_text %}<span class="help">{{ help_text }}{% endif %}
{% endif %}

(Taken from django-floppyforms sources)

This breaks for bool(TEMPLATE_STRING_IF_INVALID) == True.

comment:3 Changed 3 years ago by kmtracey

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

Using variables in the template that are not present in the context is certainly a valid use case. That is why the big warning box on the use of TEMPLATE_STRING_IF_INVALID exists. It's only useful to set this to a non-empty value in very narrow circumstances, when you are looking to ensure that all variables a template uses are in fact explicitly set in the context. ALL uses of an unset variable trigger replacing the empty string with the TEMPLATE_STRING_IF_INVALID value. I would be very surprised if with and other tags/filters quietly ignored that setting, and it would be unhelpful behavior if in fact I was using that setting to try to validate that all variables used by a template were in fact set in the context. It's not a setting that is useful for much if you are making use of the fact that unset variables are automatically turned into the empty string, and I don't think fiddling with its behavior will improve the situation. If you are generally using unset variables in templates, then TEMPLATE_STRING_IF_INVALID is really not something you should be trying to use.

comment:4 Changed 3 years ago by anonymous

The problem is that it's really hard to debug a missing template variable, if setting TEMPLATE_STRING_IF_INVALID suddenly makes other non-existing variables exist, and sets their value to whatever you set TEMPLATE_STRING_IF_INVALID to. For example, you get the bizarre effect that a variable both evaluates to "True and doesn't exist. Instead of helping me debug the template, I instead had to spent three hours figuring out why an expression was both invalid and True at the same time.

Luckily the fix is completely trivial. It also breaks no tests and seems to work fine in practice (I'm using it locally).
I've made a pull request: https://github.com/django/django/pull/729

comment:5 Changed 3 years ago by regebro

  • Cc regebro added

comment:6 Changed 3 years ago by regebro

  • Resolution wontfix deleted
  • Status changed from closed to new

As there is a trivial bugfix that doesn't break any tests and seem to work fine in practice, I humbly reopen this in hope to get it fixed, and thereby make TEMPLATE_STRING_IF_INVALID more usable and make it easier to debug templates.

comment:7 Changed 3 years ago by carljm

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

I'm still confused about what exactly this is supposed to fix, since nobody has addressed the reason kmtracey closed this ticket in the first place.

If you set TEMPLATE_STRING_IF_INVALID, you are asking that all uses of a non-existent variable be replaced by that string instead. This has several entirely logical consequences (within the overall framework of the Django template engine's design choices, which I don't necessarily agree with and you don't have to either):

1) Uses of that non-existent variable will now evaluate to True, because you have specifically asked for uses of a non-existent variable to be replaced by a string, and that (non-empty) string evaluates to True.

2) If you use a non-existent variable as the source variable in a {% with %} tag (or the with clause of an {% include %} tag), then anywhere you use the dest variable, you will get your TEMPLATE_STRING_IF_INVALID string, because those are (indirectly) also uses of the original non-existent variable.

I can't see how (2) is buggy or wrong at all. The closest thing to a bug I see in this thread is the behavior described in comment 1 with filters like default, which is a consequence of (1). That behavior is consistent with "non-existent variable replaced by TEMPLATE_STRING_IF_INVALID immediately", but I agree that it's undesirable. I don't think fixing it would be simple, though; doing it right would require introducing a new "undefined value" marker object that evaluates to False in boolean context, and only converting that value to TEMPLATE_STRING_IF_INVALID at the moment of final rendering. There would be some tricky backwards-compatibility issues there, since the "undefined" object would need to pass through filters safely that currently expect a string. If someone wants to pursue this angle, I think it should be opened as a new ticket; this one is too muddled with a variety of not-clearly-defined issues.

All that said, I'm still not clear which particular behavior the linked pull request is even trying to fix, since on inspection it seems to only impact {% include %}, not {% with %} or boolean evaluation. Unless I'm missing something, it seems to be a quite-incomplete fix, leaving things in an even less consistent state than currently.

I'm re-closing this. Usual procedure for Django if you'd like to contest a core developer's wontfix decision on a ticket is to take it to the django-developers mailing list rather than reopening, since at that point it's clear that there is either a misunderstanding or substantive disagreement (or both), and either one is best sorted out on the mailing list where there can be broader input.

comment:8 Changed 3 years ago by regebro

OK, we need to find some other way of making it possible to have reasonable debugging of Django templates then. TEMPLATE_STRING_IF_INVALID now works like action-on-a-distance, where things break and start giving behaviours like non-existent expressions evaluating to true, and as such it sometimes makes it harder to debug a template. This is a fundamental problem with Django, but we need some other way of solving it, clearly.

comment:9 Changed 3 years ago by carljm

I don't use TEMPLATE_STRING_IF_INVALID to debug templates, and I agree that it isn't very useful; I think it's an attractive nuisance that probably should be deprecated and removed. Generally I debug templates by reverting to rather primitive techniques; the equivalent of "temporarily add a print statement". I agree it would be nice to have something better, but I don't know exactly what that should look like. https://github.com/codysoyland/django-template-repl was an interesting idea, I haven't used it for a few years and don't know if it still works with current Django.

comment:10 Changed 3 years ago by gregmuellegger

I don't use it either to debug. This bug report arose from a bug reported for django-floppyforms: https://github.com/brutasse/django-floppyforms/issues/37 . We rely heavily on the default filter in floppyforms for the form rendering. It's very handy for following occations:

# in hello.html
Hello {{ name|default:"John Doe" }}.

# in index.html
{% include "hello.html" with name="Sam" %} # overwriting the default
{% include "hello.html" %} # using the default

I think TEMPLATE_STRING_IF_INVALID is kind of bad-practice debugging. So a +1 for deprecating it. What would be more elegant (but outside of django core) would be to have a django-debug-toolbar panel that lists all the "invalid" template variables used in the templates of the current page. But I don't know if an implementation is sanely possible at the moment (if there exist any usefull hooks in the template engine).

However this would also leave out the debugging of templates that are rendered out of the request-response cycle.

Last edited 3 years ago by gregmuellegger (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top