Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7415 closed (fixed)

FileFields have wrong url in admin under Windows, when using upload_to path

Reported by: edgars.jekabsons@… Owned by: Marty Alchin
Component: contrib.admin Version: dev
Severity: Keywords: windows filefield fs-rf-fixed
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

Right now (revision 7612) AdminFileWidget is implemented this way:

if value:
    output.append('%s <a target="_blank" href="%s%s">%s</a> <br />%s ' % \
                (_('Currently:'), settings.MEDIA_URL, value, value, _('Change:')))

The value in database contains \ as a path separator. It is shown as a url instead of needed / separator for urls. Works as is under Unix, but clearly not on Windows.

Model get_<field>_url() method gives the url with correct path separator. Perhaps it should be used in the link instead of value.

Attachments (1)

7415-filefield-url-admin-windows.diff (1.9 KB ) - added by Julien Phalip 16 years ago.
Patch and tests

Download all attachments as: .zip

Change History (19)

comment:1 by rudyryk <alexey.rudy@…>, 16 years ago

As I know, it's better to define all paths with '/' not depending on platform - it is valid. I do so and have no such problem as you described.

comment:2 by edgarsj, 16 years ago

Django creates the file path with '\' itself in database after uploading the file via admin.

comment:3 by Ramiro Morales, 16 years ago

Description: modified (diff)

comment:4 by Marc Garcia, 16 years ago

milestone: 1.0

comment:5 by edgarsj, 16 years ago

Keywords: bug added
Triage Stage: UnreviewedAccepted

I guess milestone 1.0 means accepted.

comment:6 by Julien Phalip, 16 years ago

Owner: changed from nobody to Julien Phalip

comment:7 by Julien Phalip, 16 years ago

Keywords: windows filefield added; bug removed
Needs tests: set

I fixed it on Windows using 'url.pathname2url()'. See patch.
Could someone test it on other platforms?

by Julien Phalip, 16 years ago

Patch and tests

comment:8 by anonymous, 16 years ago

Triage Stage: AcceptedReady for checkin
Version: newforms-adminSVN

comment:9 by anonymous, 16 years ago

Has patch: set
Needs tests: unset

comment:10 by Julien Phalip, 16 years ago

Oops, that was me above...

comment:11 by Marty Alchin, 16 years ago

Keywords: fs-rf added
milestone: 1.01.0 beta
Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I'm not sure the proposed patch is the best approach for this, but I'll take a look at this as part of #5361, since most of the affected code is changing as part of that anyway.

comment:12 by jvtm, 16 years ago

IMHO, better approach is to always use / as path separator. This guarantees that the filesystem stuff works on different platforms (just getting the value from database and using it for something).

Fixed the same problem with fieldname = fieldname.replace(os.path.sep, "/") on the application side.

comment:13 by Marty Alchin, 16 years ago

Keywords: fs-rf-fixed added; fs-rf removed
Owner: changed from Julien Phalip to Marty Alchin
Status: newassigned

The updated patch on #5361 addresses this issue, explicitly converting all slashes to forward slashes when saving to the database, and doing the same when displaying the URL, just in case older data is stored that way.

comment:14 by Julien Phalip, 16 years ago

Cool, I agree that your approach of saving the paths with forward slashes in database is best here. One concern though, will that also work with pre-existing records, which have already been saved with backward slashes?

comment:15 by Marty Alchin, 16 years ago

That alone won't solve the problem with existing data, but the conversion to forward slashes now happens when retrieving the URL, so it should all work out quite well. Feel free to give it a shot if you have some time, if you'd like to verify.

comment:16 by Marty Alchin, 16 years ago

Correction. The existing patch on #5361 does not yet completely fix this issue. The next patch will have an update to AdminFileWidget to take care of this properly.

comment:17 by Jacob, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [8244]) File storage refactoring, adding far more flexibility to Django's file handling. The new files.txt document has details of the new features.

This is a backwards-incompatible change; consult BackwardsIncompatibleChanges for details.

Fixes #3567, #3621, #4345, #5361, #5655, #7415.

Many thanks to Marty Alchin who did the vast majority of this work.

comment:11 by Jacob, 12 years ago

milestone: 1.0 beta

Milestone 1.0 beta deleted

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