Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Lets remove usage of "blacklist" and "whitelist" in file_munge_filename() and its test \Drupal\KernelTests\Core\File\NameMungingTest
They are:
- An historic bad labelling of people
- Provide no context: "what is listed in them"?
Proposed resolution
TBD
Remaining tasks
User interface changes
None
API changes
@todo
Data model changes
@todo
Release notes snippet
@todo
Comment | File | Size | Author |
---|---|---|---|
#16 | 3151087-16.patch | 2.88 KB | rik-dev |
#13 | 3151087-13.patch | 2.81 KB | rik-dev |
#9 | 3151087-9.patch | 3.16 KB | rik-dev |
#6 | 3151087-6.patch | 1.13 KB | rik-dev |
#4 | 3151087-2.patch | 1.12 KB | rik-dev |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
rik-dev CreditAttribution: rik-dev at DevBranch commentedComment #5
alexpottThis is looking pretty good. I think we can make a further improvement:
Let's change this to describe what is in the list. I.e.
$allowed_extensions
Plus I checked and we can fix the related test too - \Drupal\KernelTests\Core\File\NameMungingTest
I've checked usages in core of file_munge_filename() and I can't see anything else to change.
Comment #6
rik-dev CreditAttribution: rik-dev at DevBranch commentedComment #7
MatroskeenPlease take a look into #5 regarding related test - \Drupal\KernelTests\Core\File\NameMungingTest
Comment #8
dwwHuge +1 to this effort. Long overdue!
As someone far too familiar with
file_munge_filename()
, I can safely say that part of the change is ready. But great catch in #7 for also fixing the test(s). I checked core/modules/file/tests/src/* and didn't find anything else that needs help. So yeah, I think it's just core/tests/Drupal/KernelTests/Core/File/NameMungingTest.phpThanks!
-Derek
Comment #9
rik-dev CreditAttribution: rik-dev at DevBranch commented@alexpott Thanks for review.
Comment #10
dww@rik-dev: Please get in the habit of uploading an interdiff with your patches. Thanks!
If we're changing it anyway, we should probably use:
"Tests that allowed extensions..."
since PHPDoc comments are supposed to start with a verb.
The 2nd line can now wrap more words into the previous line.
Also, maybe start with:
"Declare that our extension is allowed."
Comment #11
dwwWhile we're touching these lines, we could also completely remove the assertion message and all of
new FormattableMarkup()
here, too. If so, check if those are the lastFormattableMarkup
used in this test class, and if so, remove theuse
statement at the top of the file. But that's perhaps scope creep, so if you ignore this comment, we can clean that up later in some other issue instead.Thanks,
-Derek
Comment #12
rik-dev CreditAttribution: rik-dev at DevBranch commentedComment #13
rik-dev CreditAttribution: rik-dev at DevBranch commented@dww Thanks for review and thanks a lot of interdiff I didn’t know about this, so will use this in future.
Comment #14
dwwSorry if I wasn't clear, but you missed what I was asking for in #10.2. "Declare that our allowed extension." isn't a sentence. And "be" can wrap into the previous line and still be under 80 chars wide.
This comment should be:
Thanks,
-Derek
Comment #15
dwwComment #16
rik-dev CreditAttribution: rik-dev at DevBranch commentedComment #17
dwwThat looks great, thanks!
Would still be useful to see interdiffs for these, but this is small enough I can just re-review each time. But seriously, please make this part of your workflow whenever you upload a new patch to an issue that already has a patch (whether the previous patch was yours or not).
Thanks!
-Derek
p.s. Crediting all the contributors so far...
Comment #18
alexpottCommitted and pushed 9eb7a173a2 to 9.1.x and fdedb4e9f5 to 9.0.x and df3e7a5863 to 8.9.x. Thanks!
As there is no API change here I've backported this to 8.9.x to keep code and tests aligned in an important function from the POV of security.