Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#16676 closed Bug (fixed)

The 'add' filter should stringify value or argument if the other value is a string

Reported by: dtrebbien Owned by: Aymeric Augustin
Component: Template system Version: 1.3
Severity: Normal Keywords:
Cc: dtrebbien@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The 1.3 documentation for the add filter states:

This filter will first try to coerce both values to integers. If this fails, it'll attempt to add the values together anyway. This will work on some data types (strings, list, etc.) and fail on others. If it fails, the result will be an empty string.

When the value and argument represent the second case ("If this fails, it'll attempt to add the values together anyway.") and one is a string while the other is a number, the result of the add filter is simply the value.

Similar to the fix for ticket #393, I think that for this second case and if either value or argument to the filter is a string, the other value should be stringified.

Test cases

{{ 'test'|add:2 }}

Result is: test
Expected result: test2

{{ 2|add:'test' }}

Result is: 2
Expected result: 2test

Workarounds

For the first test case, one workaround is to use a {% with %} tag:

{% with 2|stringformat:'d' as arg_as_str %}
{{ 'test'|add:arg_as_str }}
{% endwith %}

For the second test case, the stringformat filter can be used to convert the number to a string before add is applied:

{{ 2|stringformat:'d'|add:'test' }}

Related tickets

  • Ticket #393 [patch] Filters don't take the str() value of a var
  • Ticket #8088 Template system improvement: "cat" filter, "include" tag with filters
  • Ticket #11687 The 'add' template filter only works for integers, and can fail noisily

Attachments (2)

16676.patch (1.5 KB ) - added by Aymeric Augustin 13 years ago.
16676-docs.patch (548 bytes ) - added by Aymeric Augustin 13 years ago.
alternative solution

Download all attachments as: .zip

Change History (15)

comment:1 by Aymeric Augustin, 13 years ago

Triage Stage: UnreviewedAccepted

In Python it's an error to add a string and an int:

>>> 'a' + 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot concatenate 'str' and 'int' objects
>>> 1 + 'a'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'int' and 'str'

In this case, the proper output is an empty string, as the docs says. I could confirm that Django returns the first value. It's a bug.


However, |add doesn't really make sense with strings. + on strings is just concatenation, and concatenation is natively provided by the template engine.

Instead of {{ strval|add:intval }}, just write {{ strval }}{{ intval }}.

For this reason, I don't like the idea of converting both values to strings.


I'm attaching a patch with tests (actually, the tests existed, but they contradicted the documentation; I think the documentation has precedence). I also fixed a naked except -- we never want to catch BaseException.

by Aymeric Augustin, 13 years ago

Attachment: 16676.patch added

comment:2 by Aymeric Augustin, 13 years ago

Has patch: set

comment:3 by Russell Keith-Magee, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:4 by Chris Beaven, 13 years ago

Triage Stage: Ready for checkinDesign decision needed

Although the documentation has stated (for quite some time) that it returns an empty string, the current behavior of returning the original input is an acceptable response to an error. The docs say:

In case of error, they should return either the original input or an empty string -- whichever makes more sense.

There could be minor elements of backwards incompatibility if we change this. Perhaps just changing the docs to match the behavior would be alright to do here?

comment:5 by Aymeric Augustin, 13 years ago

In theory, I find it slightly better to display nothing that to display a wrong result. For instance, if someone adds total price and shipping cost with this filter (an horrible idea for sure, and a strong argument against the existence of the add filter), and accidentally has one of these values represented as a string, the resulting price would be wrong.

In practice, I understand the value of preserving backwards compatibility.

I'm attaching an alternative patch that changes the docs to match the code. I'm still slightly in favor of the initial patch. But what really matters is to resolve the inconsistency between the code and the docs, one way or another.

by Aymeric Augustin, 13 years ago

Attachment: 16676-docs.patch added

alternative solution

comment:6 by dtrebbien, 13 years ago

I prefer Aymeric's first patch. Preserving backward compatibility with behavior that contradicts the docs does not seem preferable to making the behavior conform to current wording. Also, I think that returning an empty string is more consistent with the template system's tendency to return nothing if there was an error (e.g.: attempting to print an undefined variable).

comment:7 by Aymeric Augustin, 13 years ago

Owner: changed from nobody to Aymeric Augustin

comment:8 by Julien Phalip, 13 years ago

An approach would be to return settings.TEMPLATE_STRING_IF_INVALID (which defaults to an empty string). However, I'd be happy enough with the doc being modified to reflect the current behaviour.

comment:9 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

The fix here is to change the template to return the empty string as the documentation says. Returning the left-hand-side on a failure is weird. The auto-stringifying-behavior suggested doesn't sound like it matches Django very wel, so let's not do that.

comment:10 by Aymeric Augustin, 13 years ago

Triage Stage: AcceptedReady for checkin

The initial patch was reviewed and marked as RFC by Russell. Since you just confirmed that the solution it uses is correct, I'm marking this ticket as RFC again (even though I'm the author).

The patch that must be committed is the first one, "16676.patch".

comment:11 by Julien Phalip, 13 years ago

Question: should settings.TEMPLATE_STRING_IF_INVALID be returned instead of a systematic hardcoded empty string, or should that setting be reserved for invalid variables?

comment:12 by Jacob, 13 years ago

No, TEMPLATE_STRING_IF_INVALID is for variables; there's no precedent in using it for filters, and I don't think we should expand it at all. It already causes enough problems!

comment:13 by Julien Phalip, 13 years ago

Resolution: fixed
Status: newclosed

In [16851]:

Fixed #16676 -- Corrected the behavior of the 'add' template filter to return an empty string instead of the given value unchanged in the case of an invalid use. Thanks to dtrebbien for the report and to Aymeric Augustin for the patch.

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