#26297 closed Bug (fixed)
collectstatic --clear throws NotImplementedError, "This backend doesn't support absolute paths."
Reported by: | Topher | Owned by: | Berker Peksag |
---|---|---|---|
Component: | contrib.staticfiles | Version: | 1.9 |
Severity: | Normal | Keywords: | clear notimplementederror path |
Cc: | berker.peksag@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
storage.py docstrings state of storage.path()
Storage systems that can't be accessed using open() should *not* implement this method.
collectstatic.py does not catch this to use the implemented storage.delete() method on line 221
full_path = self.storage.path(fpath) if not os.path.exists(full_path) and os.path.lexists(full_path): # Delete broken symlinks os.unlink(full_path) else: self.storage.delete(fpath)
Patch?:
try: full_path = self.storage.path(fpath) if not os.path.exists(full_path) and os.path.lexists(full_path): # Delete broken symlinks os.unlink(full_path) else: self.storage.delete(fpath) except NotImplementedError: self.storage.delete(fpath)
Change History (7)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|---|
Easy pickings: | unset |
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 9 years ago
Easy pickings: | set |
---|---|
Needs tests: | unset |
Triage Stage: | Accepted → Unreviewed |
I may be wrong, I don't think a test would be feasible as testing would require simulating multiple storage backends that aren't currently tested?
Off topic, I'm starting to wonder why collectstatic has any symlink logic at all. Shouldn't staticfiles offer a symlink storage backend to be used, if the functionality was desired, instead?
comment:3 by , 9 years ago
Easy pickings: | unset |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
I can't think of a reason why creating a subclass of a storage backend that raises NotImplementedError
to test this wouldn't be feasible, but I didn't look into the details.
Maybe the symlink logic added in 0922bbf18d3ae8f37e1823df2dbc270d33334548 isn't ideal. Feel free to submit a patch if you feel it could be done better.
comment:4 by , 9 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Owner: | changed from | to
Status: | new → assigned |
Pull request: https://github.com/django/django/pull/6288
comment:5 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I think something like this would make it more obvious where the exception is expected:
A test is also needed.