Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for Django 4.2 storages (Django 5.1 support) #626

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

PetrDlouhy
Copy link
Contributor

@PetrDlouhy PetrDlouhy commented Nov 16, 2023

Fix for #625 and #616

@PetrDlouhy PetrDlouhy marked this pull request as ready for review November 18, 2023 16:32
gbarredev pushed a commit to gbarredev/easy-thumbnails that referenced this pull request Mar 22, 2024
gbarredev pushed a commit to gbarredev/easy-thumbnails that referenced this pull request Mar 22, 2024
@anderspetersson
Copy link

Django 5.1 is up for release this August, how can I help to get this merged?

@PetrDlouhy PetrDlouhy force-pushed the django42_storages branch 6 times, most recently from dd1a5d3 to 15fa9d8 Compare May 28, 2024 08:24
@PetrDlouhy PetrDlouhy changed the title add support for Django 4.2 storages add support for Django 4.2 storages (Django 5.1 support) May 28, 2024
@PetrDlouhy
Copy link
Contributor Author

I have rewritten this and added Django 5.1 (pre-release) to the testing.
Now all tests are passing on my repo: https://github.com/PetrDlouhy/easy-thumbnails/actions/runs/9267240401

The testing update PR #632 is part of this PR, so preferably pull that one first.
@jrief @SmileyChris Would you please look at this?

@fsbraun
Copy link

fsbraun commented Jul 10, 2024

Yeah, this would be really helpful!

@jrief jrief merged commit 98f29e6 into SmileyChris:master Jul 25, 2024
@jrief
Copy link
Collaborator

jrief commented Jul 25, 2024

@PetrDlouhy any idea why test py39-django42-svgfails?

@PetrDlouhy
Copy link
Contributor Author

@jrief Looking into it. Seems something changed, probably in setuptools that broke easy_thumbnails/version_utils.py.

@PetrDlouhy
Copy link
Contributor Author

@jrief Looks like I was looking at the wrong error. The KeyError: 0 is caused by commit fe51af8 which changes number of version parameters.

Now I will look into the SuspiciousFileOperation errors.

@PetrDlouhy
Copy link
Contributor Author

@jrief The SuspiciousFileOperation is caused by security fix in Django 4.2.14 release: https://docs.djangoproject.com/en/5.0/releases/4.2.14/

I will try to figure out how to get around that, probably we would need to change the TemporaryStorage somehow.

@PetrDlouhy
Copy link
Contributor Author

I didn't find out how to resolve it. I will return to this, but do you have any idea, @jrief?

@PetrDlouhy
Copy link
Contributor Author

@jrief After I spent some time investigating this I realized, that there is already a fix for this issue: #634

I taught, that the change breaks only tests, but it breaks all easy-thumbnail usage with FileSystemStorage and newest Django versions.

@jrief
Copy link
Collaborator

jrief commented Jul 26, 2024

That unfortunately didn't solve the problem either.

Btw., for simplicity I prefer to not use tox. With Github actions it is very easy to setup a testing matrix and tox just adds another level of complexity. Therefore I removed tox from all of my projects. What do you think?

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented Jul 26, 2024

I don't think, that tox the problem here.

One problem is the commit fe51af8 changing number of parameters in the VERSION string, as I have already written.

The second problem is, that there are probably more places, where the CVE-2024-39330 fix is exposing problems in easy-thumbnails code, as I have written on #634. I will try to investigate further.

You are the maintainer, so if you want to remove tox, do it. For me the benefits are, that it is easier to run the tests locally.
In projects, where the tox configuration present, I think it is reasonable to use it also for GitHub testing, because it gives more coherent results as in local tests and also it ensures the tox.ini is always updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants