Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#22412 closed Cleanup/optimization (fixed)

Clarify expectation around template filters and exceptions

Reported by: carljm Owned by: nobody
Component: Documentation Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the custom template filters documentation, we say "Filter functions should always return something. They shouldn’t raise exceptions. They should fail silently. In case of error, they should return either the original input or an empty string – whichever makes more sense."

Then we immediately proceed to show (with approval, not as a counter-example) a template filter which fails to implement this recommendation. (If passed anything without a replace method, it will fail loudly.)

If we really mean to say that template filters should never raise exceptions, then a) we should provide a utility decorator that helps to implement that recommendation (something like sorl-thumbnail's safe_filter), and we should use it in all of our example filters.

Personally I think this is a bad recommendation and we should change it. Template filters, like any other code, should catch errors that they can reasonably expect and handle with a fallback value, but a blanket catching of all exceptions is just as bad in template filters as in any other code (because it is likely to hide bugs).

Change History (8)

comment:1 Changed 13 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

See #20962 where we softened the language for template tags. I guess the idea is that it's sometimes hard to anticipate all the possible data a given filter might get and you don't want bad data (especially data that comes from users) to crash your site.

comment:2 Changed 13 months ago by carljm

Right. I guess I don't see template tags/filters as different from any other code in a Django project in this respect. Lots of Django code, not just template tags/filters, might get data from users, and any code that might get data from an external source needs to be careful in how it handles that data. I think "handle exceptions you are expecting and know how to handle, and let anything else bubble up" is the correct advice for all code; the specifics of what that means will vary for different situations.

comment:3 Changed 13 months ago by aaugustin

I agree with Carl that this part of Django's philosophy has become less and less relevant.

The idea of making the DTL "designer-safe" becomes less necessary in a world where a designer that works directly in Django templates needs to be comfortable with git ;-)

comment:4 Changed 13 months ago by carljm

On further thought, I do recognize that there is a difference with template tags and filters; generally speaking they have no Python caller whom one can expect to handle some exceptions; most exceptions coming from a template tag/filter will result in a 500 server error. So that does call for more caution in raising exceptions, but I still think "never raise exceptions" is the wrong advice: if your template filter gets input that clearly represents a bug in a template which should be fixed, raising an exception may still be a better choice than failing silently.

Here is my proposed wording, which I will commit if it gets a +1:

Usually any exception raised from a template filter will be exposed as a
server error, so filter functions should attempt to return something,
and should avoid raising exceptions if there is a reasonable fallback
value to return. In case of input that clearly represents a bug in the
template, raising an exception may still be better than silent failure
which hides the bug.

comment:5 Changed 13 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

I might break the first part into two sentences: "Thus, filter functions should avoid raising exceptions if there is a reasonable fallback
value to return." (is the first "attempt to return something" in that phrase a bit redundant?)

Otherwise, I like it.

comment:6 Changed 13 months ago by Carl Meyer <carl@…>

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

In 7e3834adc993317bc691cc6edc0be13538dc39ad:

Fixed #22412 -- More nuanced advice re template filters and exceptions.

Thanks Tim for review.

comment:7 Changed 13 months ago by Tim Graham <timograham@…>

In 0b43308e2de5dcac67b530bb03376655dfb61cef:

[1.7.x] Fixed #22412 -- More nuanced advice re template filters and exceptions.

Thanks Tim for review.

Backport of 7e3834adc9 from master

comment:8 Changed 13 months ago by Tim Graham <timograham@…>

In 728fd27c005e98868a3be74949f306038e30e427:

[1.6.x] Fixed #22412 -- More nuanced advice re template filters and exceptions.

Thanks Tim for review.

Backport of 7e3834adc9 from master

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