Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6238 enhancement closed wontfix (wontfix)

t.w._flatten._flattenElement should support t.p.urlpath.URLPath

Reported by: Tom Prince Owned by:
Priority: normal Milestone:
Component: web Keywords: web templates
Cc: jknight, borko Branch:
Author: borko


I think the right thing to do here is to turn it into a string.

Attachments (1)

6238_v1.diff (2.7 KB) - added by borko 4 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by borko

Author: borko
Cc: borko added

Changed 4 years ago by borko

Attachment: 6238_v1.diff added

comment:3 Changed 4 years ago by borko

Keywords: review added

comment:4 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed

Hi borko. Thanks for working on this.

Sticking to the high-level, it's fairly important that the URL be properly escaped in the output of flatten. This patch so far makes no attempt to apply the proper escaping. Proper escaping here is made more difficult, I think, by the fact that URLPath doesn't bother to document what the result of using str() on it will be, nor whether it its various attributes store encoded or decoded data.

I think perhaps it's a bad idea to try to write any code that uses URLPath, due to the shortcomings of URLPath. A better course would be to work on improving URLPath (or replacing it) to the point where it's possible to be confident in uses of it (in other words, I don't think it is possible to have a satisfactory resolution to this ticket at this time).

At the very minimum, the tests for this feature need to include coverage for the various special characters a URL can include, and the two different flattener cases - inAttribute = True and inAttribute = False.

comment:5 Changed 4 years ago by borko

Resolution: fixed
Status: newclosed

Ok. So I opt for creating new ticket for changing URLPath in a way it should be done with precise comment, setting this ticket as blocked and writing short note about this fact including a link to the ticket that must be done before. Thx.

comment:6 Changed 4 years ago by Glyph

Resolution: fixed
Status: closedreopened

comment:7 Changed 4 years ago by Glyph

Resolution: wontfix
Status: reopenedclosed

comment:8 Changed 4 years ago by borko

so what's the final status of this ticket? are we closing it without any changes to URLPath ?

comment:9 Changed 4 years ago by Glyph

Just to be clear about my changes, I was simply (dumbly) reacting to the fact that this was closed as "fixed". If it hasn't actually been fixed but it's just waiting to be reopened, 'wontfix' is a more appropriate status. I haven't had time to review it to see if further action is warranted :).

Note: See TracTickets for help on using tickets.