Opened 16 years ago

Closed 16 years ago

#6058 closed (fixed)

6776 mark_safe breaks newforms_admin

Reported by: Tom Vergote Owned by: jkocherhans
Component: contrib.admin Version: newforms-admin
Severity: Keywords: newforms-admin
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the latest changeset [6776] on newforms-admin breaks the admin interface

some return's are merged to use the mark_safe method, but the imports were not adapted/merged

see patch

Attachments (3)

nfa_marksafe.patch (428 bytes ) - added by Tom Vergote 16 years ago.
00-admin-autoescape.diff (5.0 KB ) - added by Petr Marhoun <petr.marhoun@…> 16 years ago.
autoescape-kmt.diff (5.2 KB ) - added by Karen Tracey <kmtracey@…> 16 years ago.

Download all attachments as: .zip

Change History (15)

by Tom Vergote, 16 years ago

Attachment: nfa_marksafe.patch added

comment:1 by Tom Vergote, 16 years ago

Owner: changed from tom to Tom Vergote
Triage Stage: UnreviewedReady for checkin

comment:2 by Tom Vergote, 16 years ago

Version: SVNnewforms-admin

by Petr Marhoun <petr.marhoun@…>, 16 years ago

Attachment: 00-admin-autoescape.diff added

comment:3 by Petr Marhoun <petr.marhoun@…>, 16 years ago

I have more problems - see the patch.

I don't understand autoescape fully so I am not sure with solutions. But problems are real.

comment:4 by Malcolm Tredinnick, 16 years ago

Comment 3 is a dupe of #6059. Let's keep one issue per ticket, please.

comment:5 by Karen Tracey <kmtracey@…>, 16 years ago

Yes, the change to django/contrib/admin/templates/admin/change_form.html is an alternative fix for #6059. I took the approach of using mark_safe on the variable before sending it to the template instead of using the safe filter in template...got the impression from other code this was a preferred way to handle it?

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

django/contrib/admin/templates/admin/includes/fieldset.html

django/newforms/widgets.py

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

in reply to:  5 ; comment:6 by Petr Marhoun <petr.marhoun@…>, 16 years ago

Replying to Karen Tracey <kmtracey@gmail.com>:

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

AdminSplitDateTimeWidget inherits from SplitDateTimeWidget and SplitDateTimeWidgets inherits from MultiWidget. I don't know which place is best for fixing - I have decided for MultiWidget.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

Maybe you haven't saw them only - could you check style for sorting? I sometimes use application/xhtml+xml and then I see (without patch):

XML Parsing Error: not well-formed
Location: http://hradec:8000/admin/auth/user/
Line Number 129, Column 11:<th class=&quot;sorted ascending&quot;>
----------^


django/contrib/admin/templates/admin/includes/fieldset.html

It is for validation errors.

django/newforms/widgets.py

It is the MultiWidget.

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

Yes, it was my idea (one big issue) and the time difference was only a few minutes - when I started to read my patches before uploading, #6059 and #6060 were not here. But I am afraid I have no time today for new tickets and patches.

in reply to:  6 comment:7 by Karen Tracey <kmtracey@…>, 16 years ago

Replying to Petr Marhoun <petr.marhoun@gmail.com>:

Replying to Karen Tracey <kmtracey@gmail.com>:

The changes to django/contrib/admin/widgets.py are similarly covered by #6060, though the #6060 patch also attempts to mark_safe the output of the AdminSplitDateTime widget, so there's a slight difference there.

AdminSplitDateTimeWidget inherits from SplitDateTimeWidget and SplitDateTimeWidgets inherits from MultiWidget. I don't know which place is best for fixing - I have decided for MultiWidget.

Ah, yes, that's probably a better place to do it.

I haven't yet run into the problems requiring the changes to:

django/contrib/admin/templates/admin/change_list_results.html

Maybe you haven't saw them only - could you check style for sorting? I sometimes use application/xhtml+xml and then I see (without patch):

XML Parsing Error: not well-formed
Location: http://hradec:8000/admin/auth/user/
Line Number 129, Column 11:<th class=&quot;sorted ascending&quot;>
----------^

OK, I see the escaping in the source, it just wasn't causing a visible problem in my environment/browser. A better (in that I don't see much use of the safe filter in the admin templates, but rather mark_safe used in the code) place to fix it may be in templatetags/admin_list.py, where the variable is set.


django/contrib/admin/templates/admin/includes/fieldset.html

It is for validation errors.

Hadn't gotten to validating input earlier. This one can be fixed at the source in options.py by calling mark_safe on the return value of errors for FieldLine.

django/newforms/widgets.py

It is the MultiWidget.

Gotcha.

so they might be needing their own tickets, since Malcolm indicates we should stick to one issue per ticket. Though I also see how this could be viewed as all one big issue -- autoescape and newforms-admin...

Yes, it was my idea (one big issue) and the time difference was only a few minutes - when I started to read my patches before uploading, #6059 and #6060 were not here. But I am afraid I have no time today for new tickets and patches.

Yeah, we were running into the same problems simultaneously (though I had seen your fix for the missing import when I started). I too am running out of time to spend on this today, so I am going to attach one last diff to this ticket, that contains all the fixes I am using. Similar to the 2nd patch except I used mark_safe at the source rather than the safe filter in the templates. I see Joseph is starting to commit fixes anyway so shortly a new checkout will probably resolve all of these.

by Karen Tracey <kmtracey@…>, 16 years ago

Attachment: autoescape-kmt.diff added

comment:8 by Tom Vergote, 16 years ago

I agree, there is a lot more to this, I just patched what i noticed this morning, and then was busy fixing some of my own code that broke after the changeset.

comment:9 by Tom Vergote, 16 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

comment:10 by jkocherhans, 16 years ago

Owner: changed from Tom Vergote to jkocherhans

I'll be working on tests shortly based on Karen's last patch. tvrg, if you are planning on writing the tests, and expect you'll be able to finish in the next few hours, please reassign yourself to this ticket.

comment:11 by Tom Vergote, 16 years ago

I won't finish any tests today (considering day and timezone :))

comment:12 by jkocherhans, 16 years ago

Resolution: fixed
Status: newclosed

Doh. This was fixed in [6782], but I put the wrong ticket number in the commit message.

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