Opened 10 years ago

Closed 7 years ago

#12742 closed New feature (wontfix)

File upload handler in Comments framework

Reported by: Sebastian Żurek Owned by: nobody
Component: contrib.comments Version: master
Severity: Normal Keywords: files
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no


I've recently find out some annoying django.contrib.comments limitations-chain. It came out after unsuccessful custom comment form (that included FileField) submission.

This is what I mean by limitations-chain:

I could not use comment model having file field (CommentSeciurityForm does not use *args, kwargs when supering forms.Form and thus files=request.FILES was not valid init kword argument in forms that base on CommentSeciurityForm)

In consequence comment form post handler (view function named 'post_comment') is not using request.FILES

Which is related with form template issue: check for form.is_multipart is missing (hence multipart/form-data enctype is not set)

Are those file-handling limitation intentionally created (some security reasons?)? I'm attaching a .diff with some cosmetic changes, that allow for files upload.

While browsing django.contrib.comments code and using the framework in my project I can see there's still some work to be done (there's plenty of room for some generalizations - I found API being nice, however we could make it more flexible with e.g. allowing more than one comment form definition).

However, to stay focused on one issue: could You please comment this problem (or not) with files?

Attachments (2)

file_upload.diff (2.3 KB) - added by Sebastian Żurek 10 years ago.
12742.diff (1.8 KB) - added by Thejaswi Puthraya 9 years ago.
generic patch that will also handle both #12742 and #10838

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by Sebastian Żurek

Attachment: file_upload.diff added

comment:1 Changed 10 years ago by Alex Gaynor

Triage Stage: UnreviewedAccepted

comment:2 Changed 10 years ago by Alex Gaynor

milestone: 1.2

comment:3 Changed 10 years ago by James Bennett

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:4 Changed 9 years ago by Matt McClanahan

Severity: Normal
Type: Bug

comment:5 Changed 9 years ago by Julien Phalip

Needs documentation: set
Needs tests: set
Type: BugNew feature

comment:6 Changed 9 years ago by Thejaswi Puthraya

Easy pickings: unset
UI/UX: unset

Related (slightly duplicate) to #10838.

Changed 9 years ago by Thejaswi Puthraya

Attachment: 12742.diff added

generic patch that will also handle both #12742 and #10838

comment:7 Changed 9 years ago by Thejaswi Puthraya

The reason for adding args and kwargs to the post_comment and then passing these to the form would be to allow users to send additional data and (say) from there on to the model (could be a custom comment).

For example, with the deprecation of the RemoteAddrMiddleware, there is no standard way to fetch the IP address. The IP address could be passed as a kwarg to the form. This could prevent hacks like

Another case, could be where the form's rendering depends on the request (or the session) and these could be passed through the args and/or kwargs (they can't be handled in the current case).

For this ticket, the post_comment could be passed the request from which the request.FILES can be accessed.

This patch will also solve quite a lot of #10838 and which requires the user to be passed.

Just for the record, Alex (from a conversation on IRC) is not happy with passing args and kwargs to the post_comment view function. I have placed my arguments for the reason to include the args and kwargs and now the decision is left to you :)

comment:8 Changed 7 years ago by Jacob

Resolution: wontfix
Status: newclosed

django.contrib.comments has been deprecated and is no longer supported, so I'm closing this ticket. We're encouraging users to transition to a custom solution, or to a hosted solution like Disqus.

The code itself has moved to; if you want to keep using it, you could move this bug over there.

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