Opened 8 years ago

Closed 8 years ago

Last modified 5 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: master
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: UI/UX:

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 8 years ago.
Patch and tests

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by rudyryk <alexey.rudy@…>

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 Changed 8 years ago by edgarsj

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

comment:3 Changed 8 years ago by Ramiro Morales

Description: modified (diff)

comment:4 Changed 8 years ago by Marc Garcia

milestone: 1.0

comment:5 Changed 8 years ago by edgarsj

Keywords: bug added
Triage Stage: UnreviewedAccepted

I guess milestone 1.0 means accepted.

comment:6 Changed 8 years ago by Julien Phalip

Owner: changed from nobody to Julien Phalip

comment:7 Changed 8 years ago by Julien Phalip

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?

Changed 8 years ago by Julien Phalip

Patch and tests

comment:8 Changed 8 years ago by anonymous

Triage Stage: AcceptedReady for checkin
Version: newforms-adminSVN

comment:9 Changed 8 years ago by anonymous

Has patch: set
Needs tests: unset

comment:10 Changed 8 years ago by Julien Phalip

Oops, that was me above...

comment:11 Changed 8 years ago by Marty Alchin

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 Changed 8 years ago by jvtm

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 Changed 8 years ago by Marty Alchin

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 Changed 8 years ago by Julien Phalip

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 Changed 8 years ago by Marty Alchin

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 Changed 8 years ago by Marty Alchin

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 Changed 8 years ago by Jacob

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 Changed 5 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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