[Django] #31517: ManifestFilesMixin.file_hash returning None get's included in hashed filename as 'None'
Django
filename as 'None'
-------------------------------------+-------------------------------------
Reporter: Richard | Owner: nobody
Campen |
Type: Bug | Status: new
Component: | Version: 3.0
contrib.staticfiles | Keywords: HashedFilesMixin,
Severity: Normal | ManifestFilesMixin, file_hash,
Triage Stage: | hashed_name
Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
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>`.
[https://groups.google.com/forum/#!searchin/django-
developers/file_hash%7Csort:date/django-
developers/GXNDGBb3tlM/VE61CAgoAgAJ 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))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31517>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
filename as 'None'
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: HashedFilesMixin, | Triage Stage:
ManifestFilesMixin, file_hash, | Unreviewed
hashed_name |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 3.0 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:1>
Django
filename as 'None'.
-------------------------------------+-------------------------------------
Reporter: Richard Campen | Owner: Richard
Type: | Campen
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
ManifestFilesMixin, file_hash, |
hashed_name |
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* owner: nobody => Richard Campen
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Old description:
New description:
to
--
Comment:
Thanks for this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:2>
Django
filename as 'None'.
-------------------------------------+-------------------------------------
Reporter: Richard Campen | Owner: Richard
Type: | Campen
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: HashedFilesMixin, | Triage Stage: Accepted
ManifestFilesMixin, file_hash, |
hashed_name |
-------------------------------------+-------------------------------------
Comment (by Richard Campen):
Thanks for the feedback. I've update the PR as per your recommendations.
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:3>
Django
filename as 'None'.
-------------------------------------+-------------------------------------
Reporter: Richard Campen | Owner: Richard
Type: | Campen
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
ManifestFilesMixin, file_hash, | checkin
hashed_name |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:4>
Django
filename as 'None'.
-------------------------------------+-------------------------------------
Reporter: Richard Campen | Owner: Richard
Type: | Campen
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution: fixed
Keywords: HashedFilesMixin, | Triage Stage: Ready for
ManifestFilesMixin, file_hash, | checkin
hashed_name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"67b334fbaf7cfb0936666ea788d406a55a09754c" 67b334f]:
{{{
#!CommitTicketReference repository=""
revision="67b334fbaf7cfb0936666ea788d406a55a09754c"
Fixed #31517 -- Fixed HashedFilesMixin.hashed_name() if hash of the file
is None.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:5>