Opened 5 years ago
Last modified 5 years ago
#31517 closed Cleanup/optimization
ManifestFilesMixin.file_hash() returning None get's included in hashed filename as 'None'. — at Version 2
Reported by: | Richard Campen | Owned by: | Richard Campen |
---|---|---|---|
Component: | contrib.staticfiles | Version: | dev |
Severity: | Normal | Keywords: | HashedFilesMixin, ManifestFilesMixin, file_hash, hashed_name |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
When returning a string from a custom ManifestFilesMixin.file_hash()
implementation, the resulting file name is <file_path>.<custom_hash>.<ext>
as expected, whereas returning None
results in <file_path>None.<ext>
.
Discussion on django-developers supports this behaviour being unintended.
Behavior appears to have been introduced with #17896 which split the file hashing into a separate method.
The following test, when included in the test_storage.TestCollectionManifestStorage
test class demonstrates the bug:
def test_hashed_name_unchanged_when_file_hash_is_None(self): with mock.patch('django.contrib.staticfiles.storage.ManifestStaticFilesStorage.file_hash', return_value=None): self.assertEqual(storage.staticfiles_storage.hashed_name('test/file.txt'), 'test/file.txt')
As suggested by the name of my test, my opinion is that the correct behaviour should be that if file_hash
returns None, then no hash is inserted into the filename and it therefore remains unchanged.
With that in mind, a possible solution is to change the following lines in the hashed_name()
method (~line 100 in staticfiles.storage
):
if file_hash is not None: file_hash = ".%s" % file_hash hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
to
if file_hash is None: file_hash = "" else: file_hash = ".%s" % file_hash hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
Change History (2)
comment:1 by , 5 years ago
Version: | 3.0 → master |
---|
comment:2 by , 5 years ago
Description: | modified (diff) |
---|---|
Needs tests: | set |
Owner: | changed from | to
Patch needs improvement: | set |
Status: | new → assigned |
Summary: | ManifestFilesMixin.file_hash returning None get's included in hashed filename as 'None' → ManifestFilesMixin.file_hash() returning None get's included in hashed filename as 'None'. |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
Thanks for this ticket.