Skip to content

Refs #2068: Do not reinstantiate staticfiles storage classes #2069

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

Closed
wants to merge 1 commit into from

Conversation

matthiask
Copy link
Member

@matthiask matthiask commented Feb 6, 2025

Description

This change might address an issue where reinstantiating the staticfiles class inside our staticfiles panel would lose some attributes of the storage instance.

Fixes #2068

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@tim-schilling
Copy link
Member

@matthiask would it be possible to wrap the url method on the storage class without having to create a new class?

@matthiask
Copy link
Member Author

@tim-schilling Yes, I think we could directly monkey-patch the url method on the static files storage instance. Do you think that would be preferrable to the Mixin injection? I can see how a straightforward monkeypatch might be easier to understand.

@tim-schilling
Copy link
Member

I think either case works well. I haven't manipulated __mro__ often, but if it works I'm good with it.

@matthiask
Copy link
Member Author

I have done too many hacks with __bases__, and monkey patching the method seems to be a better way to go.

If we want a quick bugfix release for #2068 I can finish this, but maybe this would also be a good fit for our Djangonauts? Especially if we can wait a few days or weeks for a fix.

@tim-schilling
Copy link
Member

I'm good with waiting.

@dr-rompecabezas
Copy link
Member

I assigned #2068 to myself, thinking all this PR needed was tests, but now I see you were considering a different approach (monkey patching). I'm trying to understand the proposal to see if I can help.

@matthiask
Copy link
Member Author

So, monkeypatching would mean something like this:

def patched_url(self, path):
    url = orig_url(path)
    ... our own customizations
    return url

orig_url = storage.staticfiles_storage.url
storage.staticfiles_storage.url = patched_url

It's a less tricky way to achieve basically the same thing my MRO hack did. I'm not immediately aware of any downsides either way. Also, runtime patching is frowned upon, but we have to do something ugly to add our instrumentation.

@tim-schilling
Copy link
Member

I suspect the MRO hack may be less intrusive to other devs. If we monkey patch things, we'd want to consider using functools.wraps or something similar to keep the method interface the same.

@dr-rompecabezas
Copy link
Member

Superseded by #2097

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.

Version 5.0 breaks collectstatic when using some storage backends
3 participants