Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 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: aaugustin
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 aaugustin 3 years ago.
16676-docs.patch (548 bytes) - added by aaugustin 3 years ago.
alternative solution

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

  • Has patch set

comment:3 Changed 3 years ago by russellm

  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 3 years ago by SmileyChris

  • Triage Stage changed from Ready for checkin to Design 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 Changed 3 years ago by aaugustin

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.

Changed 3 years ago by aaugustin

alternative solution

comment:6 Changed 3 years ago by dtrebbien

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 Changed 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:8 Changed 3 years ago by julien

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 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready 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 Changed 3 years ago by julien

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 Changed 3 years ago by jacob

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 Changed 3 years ago by julien

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.