Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7415 closed (fixed)

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

Reported by: edgars.jekabsons@… Owned by: Gulopine
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)

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

Download all attachments as: .zip

Change History (19)

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

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

comment:3 Changed 6 years ago by ramiro

  • Description modified (diff)

comment:4 Changed 6 years ago by garcia_marc

  • milestone set to 1.0

comment:5 Changed 6 years ago by edgarsj

  • Keywords bug added
  • Triage Stage changed from Unreviewed to Accepted

I guess milestone 1.0 means accepted.

comment:6 Changed 6 years ago by julien

  • Owner changed from nobody to julien

comment:7 Changed 6 years ago by julien

  • 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 6 years ago by julien

Patch and tests

comment:8 Changed 6 years ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from newforms-admin to SVN

comment:9 Changed 6 years ago by anonymous

  • Has patch set
  • Needs tests unset

comment:10 Changed 6 years ago by julien

Oops, that was me above...

comment:11 Changed 6 years ago by Gulopine

  • Keywords fs-rf added
  • milestone changed from 1.0 to 1.0 beta
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 6 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 6 years ago by Gulopine

  • Keywords fs-rf-fixed added; fs-rf removed
  • Owner changed from julien to Gulopine
  • Status changed from new to assigned

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 6 years ago by julien

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 6 years ago by Gulopine

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 6 years ago by Gulopine

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 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.