Better decouple Storage and FileField

[Cristiano Coelho]

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.

[Cristiano Coelho]

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

[Tim Graham]

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.

[Cristiano Coelho]

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?

[Tim Graham]

Please draft something. See also our PatchReviewChecklist.

[Cristiano Coelho]

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.

[Cristiano Coelho]

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.

[Tim Graham]

Actually this looks like a duplicate of #26058

[Korijn van Golen]

Replying to timgraham:

Actually this looks like a duplicate of #26058

Well, I'm glad someone finally noticed... ;) looks like my efforts were in vain now though.