search

matthewwithanm / django-imagekit

Automated image processing for Django. Currently v4.0
http://django-imagekit.rtfd.org/
BSD 3-Clause "New" or "Revised" License
2.27k stars 275 forks source link

Upgrade project to latest versions of python and django #560

Closed Alexerson closed 1 year ago

Alexerson commented 1 year ago

The main issue to fix is that the lib is currently not properly getting the default STORAGE backend if the project is already using the new 4.2 settings.

I also removed unsupported versions from tox and added the new ones.

vstoykov commented 1 year ago

Thank you for your contribution!

Can you also fix the .github/workflows/python.yml to use the same python versions as defined in the tox.ini?

Alexerson commented 1 year ago

Done!

Edit: actually let me try to add a test.

Alexerson commented 1 year ago

Done. Hopefully they pass properly (tox seems happy locally)… It’s a bit tricky to test settings because AppConf runs only once by default, but I think what I did really tests what’s happening.

vstoykov commented 1 year ago

Thank you very much!

Alexerson commented 1 year ago

Actually, I did some more digging and the way I fixed it, it won’t properly support OPTIONS for the new STORAGES key. As long as it’s just the BACKEND, all good, but OPTIONS are dropped. It might require a slightly more involved fix to do that properly. I’ll try to look into it.

Alexerson commented 1 year ago

I think it will be hard to do this without actually dropping the current setting and replacing by a setting that look like the new Django one. Or alternatively, it could be a key that point to the STORAGES dict, but I’m not too sure if that’s standard.

Let me know if this is something you’d like me to try to look into. I might have some time by end of the week or early next week. Unfortunately, I don’t think it’s worth releasing what I did until then.

For now on my side, I’m setting cache_storage for each field separately to fix this.

vstoykov commented 1 year ago

On a second look about the new config for storages it really seems that will be better to make IMAGEKIT_DEFAULT_FILE_STORAGE to be configured with the corresponding key in the STORAGES (when using DJango 4.2+)

Basically IMAGEKIT_DEFAULT_FILE_STORAGE = None and IMAGEKIT_DEFAULT_FILE_STORAGE = 'default' on DJango 4.2+ to be equivalent. This will work similarly as IMAGEKIT_CACHE_BACKEND. We can have only a fallback if the value is not in settings.STORAGES to try to load it as a class.

This behavior can be in the documentation and I think that will not be a problem. What you think. Do you will have time to propose new PR with these changes?

Alexerson commented 1 year ago

Sounds good. Do you really want to keep the same setting name, or would it make more sense to have a different setting name? I’ll try to find the time to work on this on Monday (if not before). Thanks for your responsivity.

Alexerson commented 1 year ago

See https://github.com/matthewwithanm/django-imagekit/pull/561