#26382 closed Cleanup/optimization (duplicate)
Better decouple Storage and FileField
Reported by: | Cristiano Coelho | Owned by: | nobody |
---|---|---|---|
Component: | File uploads/storage | Version: | 1.9 |
Severity: | Normal | Keywords: | storage filefield |
Cc: | Cristiano Coelho | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Hello, following this: https://groups.google.com/forum/#!topic/django-developers/CjoPZq8lZms
It seems like the Storage and FileField classes are doing too much work that should be actually be delegated to the storage implementation, making it currently a bit more complicated to implement custom storages since the base Storage and FileField classes perform some os dependent work and based on assumptions like files should be always be saved with forward slashes or os dependant semantics like folders and paths.
In particular, the methods get_directory_name, get_filename and generate_file_name from FileField (https://github.com/django/django/blob/master/django/db/models/fields/files.py#L297) and the method save from Storage (https://github.com/django/django/blob/master/django/core/files/storage.py#L57) should be changed a bit so they delegate their work to the actual storage system.
Finally for Storage.save, the below line should be moved to the ._save method of FileSystemStorage
# Store filenames with forward slashes, even on Windows return force_text(name.replace('\\', '/'))
I would move get_directory_name, get_filename and generate_file_name to the Storage class, however those methods also rely on the actual instance and upload_to value or callable and having those parameters sent to the storage class might not be a good idea. I'm not sure what would be the best way to remove any 'os.path' usage of FileField into the actual storage class without breaking everything.
Change History (9)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Has patch: | set |
---|
comment:3 by , 9 years ago
Patch needs improvement: | set |
---|---|
Summary: | Improve Storage and FileField → Better decouple Storage and FileField |
Triage Stage: | Unreviewed → Accepted |
Please send a pull request so you can use the automated checks from our CI setup. Any backwards-incompatible changes should be added to the release notes.
comment:4 by , 9 years ago
Looks like I missed to add changes in the release notes as well, is it something I should do or the actual person that accepts the change?
comment:6 by , 9 years ago
I have looked into the docs usages of django.db.models.fields.files.FileField.get_filename and django.db.models.fields.files.FileField.get_directory_name and found none, seems like they were never supposed to be used, should I still add a deprecation documentation?
All this documentation is taking me more than expected, I will try to have something in a while.
comment:7 by , 9 years ago
Well if everything was right, here's the pull request https://github.com/django/django/pull/6321
I have removed the related work to #26367 and once this is done I will get back to that again. Waiting on feedback.
follow-up: 9 comment:8 by , 9 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Actually this looks like a duplicate of #26058
I have commited to my repository a first attempt to improve this which seems to be working very well. No tests failed, no tests required changes and only one possible backwards incompatible change which might affect only a very limited amount of users, basically the ones that are using a custom Storage class, running on windows and relying on the default .save method of Storage. All info at the commit notes.
https://github.com/cristianocca/django/commit/4be936ba3031f95d34101a919648f4e8f7d7856a