Opened 10 years ago
Closed 10 years ago
#24664 closed Cleanup/optimization (fixed)
TemporaryFileUploadHandler new_file() does not match its parent class
Reported by: | David Danier | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | 1.8 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
TemporaryFileUploadHandler.new_file(self, file_name, *args, kwargs)
vs.
FileUploadHandler.new_file(self, field_name, file_name, content_type, content_length, charset=None, content_type_extra=None)
TemporaryFileUploadHandler.new_file will take "file_name" as its first argument and pass this (still as first argument) to FileUploadHandler.new_file. So the argument will end up FileUploadHandler.field_name, which is wrong.
I think a valid fix would be to remove file_name from TemporaryFileUploadHandler.new_file as the parameter is not used besides for calling the parent class. It may have been used for passing it to TemporaryUploadedFile, but this must be history. I'll attach a patch to change this.
Attachments (1)
Change History (4)
by , 10 years ago
Attachment: | TemporaryFileUploadHandler_new_file_fix_parent_call.patch added |
---|
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 10 years ago
[...] but this shouldn't lead to problems as the parameter is not used
It does lead to problems if you call TemporaryFileUploadHandler.new_file() with named parameters (kwargs). In this case you will pass mutiple values for the same argument when calling super(). Hit me here:
https://github.com/ddanier/django-git-lfs/blob/master/django_git_lfs/views.py#L130
(Fixed this by only passing normal args)
Indeed, the parameter name is misleading, but this shouldn't lead to problems as the parameter is not used (this issue is there from 2008 and [d725cc9734272f867d41f7236235c28b3931a1b2]).
The patch looks good.