Opened 16 years ago

Closed 2 years ago

#6135 closed New feature (wontfix)

Introduce a short-cut for template filters that has needs_autoescape = True

Reported by: anonymous Owned by: Chinmoy
Component: Template system Version: dev
Severity: Normal Keywords: autoescape
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

After the autoescaping was introduced I had to rewrite all filters to comform new requirements: put is_safe=True and needs_autoescape=True where needed.

I have written about 25 filters that have needs_autoescape=True and absolutely of them contained contained the same strings

def filter(text, autoescape=None): 
    if autoescape: 
        esc = conditional_escape 
    else: 
        esc = lambda x: x 
    text = esc(text)

   # Then goes the part which is different for all filters
   #
   #

    return mark_safe(result)

I have checked the filters in django. All the same.
I think it is way to much of writting.

I propose to split property needs_autoescape = True on two properties:

needs_manual_autoescape = True, then Do all the stuff above, manually.

needs_autoescape = True, then all above steps will do template system for user. All this thinks can do a simple decorator as well.

Change History (22)

comment:1 by anonymous, 16 years ago

Forgot to mention that above code, which very common and annoying to write, can be transformed to

@automatic_autoescape
def filter(text, autoescape=None): 
filter.needs_autoescape = True

Or even simplier version

def filter(text): 
   #Actually the jod done
filter.needs_automatic_autoescape = True

comment:2 by Jacob, 16 years ago

I think the decorator approach is the best way. I'd expect it to work like this:

@autoescaped
def filter(text):
    ...

comment:3 by Eratothene, 16 years ago

In case if

@autoescaped
def filter(text):

It will be inconsistent with filter.is_safe = True
In one case we have decorator (autoescaped), in other a property (filter.is_safe).
I think we should stick to one syntax, either decorator or propertry.

One way (Backward incompatible):

@autoescaped
def filter(text):

@safe
def filter(text):

@manually_autoescaped
def filter(text, autoescape=None):

Another way (Backward Compatible):

def filter(text):
filter.needs_autoescape = True


def filter(text):
filter.is_safe = True

def filter(text, autoescape=None):
filter.needs_manual_autoescape = True

comment:4 by Eratosfen, 16 years ago

I prefer the decorator syntax more than existing one

comment:5 by Simon G <dev@…>, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:6 by Malcolm Tredinnick, 16 years ago

Triage Stage: Design decision neededAccepted

Comment 3 is mistaken. The is_safe property performs quite a different function to the needs_autoescape attribute (which is really just a flag). There's no inconsistency imposed by changing needs_autoescape functionality into a decorator. Also, the decorator is probably fully backwards compatible, since all it will do is add the needs_autoescape attribute to the resulting function and accept the extra argument (plus doing the initial processing). It's an addition to needs_autoescape (turning that into an implementation detail, for the most part), not a replacement. Filters are possible which don't subscribe to the pattern mentioned, so the decorator approach shouldn't be the only way to do this -- don't remove needs_autoescape, just implement a helper, in other words.

Jacob's decorator name doesn't feel right, though. Perhaps autoescape_aware is a better name. The function itself isn't a past-tense of anything.

comment:7 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: New feature

comment:8 by Jannis Leidel, 13 years ago

Easy pickings: unset
UI/UX: unset

#16726 was a dupe.

comment:9 by Johannes Maron, 4 years ago

Needs tests: set
Patch needs improvement: set

comment:10 by Seth Thoburn, 4 years ago

Needs tests: unset
Owner: changed from nobody to Seth Thoburn
Patch needs improvement: unset
Status: newassigned

comment:11 by Jacob Walls, 3 years ago

Has patch: set

comment:12 by Carlton Gibson, 3 years ago

Needs documentation: set
Needs tests: set

comment:13 by David Smith, 3 years ago

Easy pickings: set
Owner: Seth Thoburn removed
Status: assignednew

comment:16 by Chinmoy, 3 years ago

Owner: set to Chinmoy
Status: newassigned

This is a rather old ticket. But the patch was not finalized. It's still open to work right?

comment:18 by Chinmoy, 3 years ago

Needs documentation: unset
Needs tests: unset

comment:19 by Chinmoy, 3 years ago

Can someone have a look at the PR. The doc tests seem to complain about a spelling mistake.

in reply to:  19 comment:20 by Ayush Joshi, 2 years ago

Patch needs improvement: set

Replying to Chinmoy:

Can someone have a look at the PR. The doc tests seem to complain about a spelling mistake.

I requested some changes there. From my side the doc test is unable to download the logs so I can't help in that failing action.

Last edited 2 years ago by Ayush Joshi (previous) (diff)

comment:21 by Chinmoy, 2 years ago

Patch needs improvement: unset

I've applied the necessary changes.

comment:22 by Carlton Gibson, 2 years ago

I think we should wontfix this. Copying my comment from the PR:

I have to say that after 14 years I think we should probably close this as wontfix. 😬

I think the new decorator is likely to cause more confusion that it'll solve.

Specifically, it's only going to be appropriate for filters already marked @stringfilter — that's fine, in theory, but that's not documented at all — and it's going to be anything but clear to document — and we're going to see a whole load of reports saying @autoescape_aware doesn't work with my filter.

There's already a way to do this, and the proposed decorator isn't sufficiently robust to cover all cases.

Also, as evidenced by those 14 years, I'm not convinced the saving is all that great:

    if autoescape: 
        esc = conditional_escape 
    else: 
        esc = lambda x: x 

… OK, yes. It's slightly repetitive. If it really bothered me I could write a make_escape() factory function to remove the duplication. But, on the other hand, it's clear: there's no question of what the behaviour is, and I don't have to go looking into the source to see how the parameters get applied.

Summary, I'm not at all convinced the proposed decorator pays its way.
🤔

Last edited 2 years ago by Carlton Gibson (previous) (diff)

comment:23 by Mariusz Felisiak, 2 years ago

I think we should wontfix this.

Agreed.

comment:24 by Carlton Gibson, 2 years ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top