Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc)

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-01-27 Fixed . That issue will have a link to a recording of the meeting.

For the record, the attendees at this usability meeting were @BlackBamboo, @benjifisher, @ckrina, @iszabo, @shaal, and @simohell.

In the review we agreed, that

• - The setting for Transliterate should not set other options' visibility as there is no obvious hierarchy. Instead it is recommended to use an HTML details-element to contain all the settings.
• - The title would be better as a slightly shorter version "Sanitize filenames".
• - Removing duplicate dots etc. should state the precise action. We assume the actual action is "Replace duplicate dots (..), underscores (__) or dashes (--) with a single one."
• - Sticking to a single replacement character keeps the settings simple and therefore usable. The settings are easier to read if the replacement character is in the end.

- Updated the interface based on usability review. Made the details open by default. Screenshots updated to summary.
- Based on usability review I also updated the code and tests so its possible to only do specific sanitizations (eg. only lowercase).
- Removed the mentioned PHP bug workaround on FileEventSubscriber::sanitizeFilename, since its only on PHP 7.4, not 8.x

For a suggestion on #337 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed about using AsciiSlugger, I think it would be relevant if core would use it also on \Drupal\Component\Transliteration. So suggesting leaving it to follow-up issue.

(Fixing screenshot filename in issue summary.)

Fixing the CSpell issue.

Hi @sokru , Reviewed the patch #341 for drupal version 10.1.x. It was successfully applied .Adding screenshots for the reference. Need RTBC+1

I've tested the patch in #341 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed on a drupal 10.1.x-dev install as well. overall the feature looks great! the only nitpick i would have is when you are using a screenreader (voiceover on macos 12.6.1 in my case). in my current verbosity text setting (see the linked video) i have activated the default option some. if you switch to the option all the information becomes even more granular. but as you can see in the current video, with the option some set, underscore gets announced twice for the first sentence and in the second second sentence three times. for sighted users i consider it a good thing to exemplify and illustrate what dot, underscore and dashes stand for, but in the aural interface it is sort of redundant for some or all characters depending on the set verbosity level. so i wonder if it would make sense to wrap the parenthesis with dots, underscores, and dashes in spans and hide them from the screenreader with something like aria-hidden="true"?

1. +++ b/core/modules/file/file.module
   @@ -1331,3 +1331,80 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
   +    '#title' => t('Remove duplicate dots (..), underscores (__) or dashes (--) with a single one'),
   

   Text should be:

   "Replace duplicate dots (..), underscores (__) or dashes (--) with a single one."
   

   i.e., swap "Remove" with "Replace"

2. IMO, there is another feature that could be supported here:

   replace sequences of dots, underscores and dashes with a single one: so that stuff like: _-_ would be simplified to -.

Addressed the comment on #344
Attached patch against Drupal 10.1.x

Patch #345 does not apply.

I would also prefer not to change the quotes to double quotes (as there are single quotes used everywhere).

-    '#title' => t('Remove duplicate dots (..), underscores (__) or dashes (--) with a single one'),
+    '#title' => t("Replace duplicate dots (..), underscores (__) or dashes (--) with a single one."),

Addressed the comment on #346
Attached patch against Drupal 10.1.x

Addressed #344.
Needs review.

Removing the dot at end of title and addressing #343, will ask feedback from #ux about the change.

#344.2 "IMO, there is another feature that could be supported. "Replace sequences of dots, underscores and dashes with a single one". So that stuff like: _-_ would be simplified to -."

Not sure how common this replacement pattern is. IMHO I'd leave it to follow-up or to contrib (eg. "regexp filename transliteration") module.

@sokru this kind of filters better leave for contrib a-la https://www.drupal.org/project/smiley →

+++ b/core/modules/file/file.module
@@ -1372,12 +1372,12 @@
-    '#description' => t('Alphanumeric characters, dots (.), underscores (_) and dashes (-) are preserved.'),
+    '#description' => t('Alphanumeric characters, dots <span aria-hidden="true">(.)</span>, underscores <span aria-hidden="true">(_)</span> and dashes <span aria-hidden="true">(-)</span> are preserved.'),

not sure potx can properly parse it as it contains attributes

@andypost, but there's plenty of strings like

Permissions <span class="visually-hidden">for @module</span>

in core already. I tried exporting string with aria-hidden attribute with drupal/potx → module and it exported the string nicely.

Patch no longer applies cleanly - Will move to NW for that.

error: patch failed: core/modules/file/file.post_update.php:13
error: core/modules/file/file.post_update.php: patch does not apply

but fixed manually for testing (didn't upload as I didn't want to take myself out of the review role)

This was tagged for issue rescope but that was 4 years ago, is that still needed? - Should be removed or update before moving to RTBC

Taking a look at the remaining tasks.

Final string/UI review

Has this been completed by UX team?

Manually testing
post_update hook ran without issue
Enabled all settings in File system.
On Drupal 10.1 with a Standard install
Edited Article content type image field to be unlimited
Uploaded these files and were transformed to
Test File!.png => test-file.png
Test File.png => test-file_0.png
Test ..--__.png => test-.-_.png
Test .. File.png => test-.-file.png

Switched to underscores vs dashes
Test File!.png => test_file.png
Test File.png => test_file_0.png
Test ..--__.png =>test_.-_.png May be a non issue but there was a dash left over
Test .. File.png => test_._file.png

Added reroll of patch #350.

re #353 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed the results of the ux meeting were in #338 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed and the recommended changes were applied in #339 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed

the only detail left is #343 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed . i was unable to attend the ux meeting on that day but have taken a look at the issue afterwards and just noticed the redundancy i've provided a video and a suggestion for a potential fix for.

@sokru already asked over in the accessibility channel for maintainers feedback in that regard to make sure there aren't any downsides: https://drupal.slack.com/archives/C2ANFUGGG/p1677133509757059

Can the issue summary be updated then please

you mean adding a task to the list or remaining tasks? something like 13. Avoid the announcement of redundant text by a screenreader (ref #343)? or would you recommend (also) another step?

Yup should be added to remaining tasks exactly how you said it, that's perfect.

Think the quickest fix would be to remove the examples of dots, underscores, and dashes completely but not sure if that's the desired fix.

Moving to NW as this fix needs to be done before review.

thanks i am always a bit hesitant updating issue summaries.

and removing those examples would have a downside for sighted users. by using those examples you are able to skim those punctuation character in a glimps of an eye instead of reading and processing their names. and it also provides a certain level of convenience for none native speakers who are using the admin interface in english. having examples for those punctuation characters is way more clear. if possible the micro copy for sighted users should be left as is.

and i've added the point to the issue summary.

woops somehow switched to needs review by accident. setting back to needs work. sorry

I wonder if this whole problem or accessibility, strings, and duplicate character handling could be simplified by following what filename_transliteration contrib module does:

Reduce duplicate separators - Sequences of underscores, dashes, and periods are simplified to a single character.

No need for confusing explanations and examples like (—), (..), etc. And handles more cases ultimately resulting in easier-to-read file names.

Think the solution is either that or what rkroller suggested and wrap the parenthesis in aria hidden tag.

Using approach from #368, Please check the interdiff; a bit of code repetition, but makes the results less messy.

The last submitted patch, 370: 2492171-370.patch, failed testing. View results →

re #368 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed ahhhh now i understand. thanks for the clarification! you definitely have a point. having an example for each punctuation example for the first checkbox label is the correct approach as you stated. but having the same pattern for the second checkbox which i basically about sequences of punctuation characters that is definitely suboptimal. i agree.

1. with the latest patch the label got changed to Simplify sequences of dots, underscores and dashes (eg _-_, .., --) with the replacement character.. i have to admit the label text posed a certain cognitive challenge to me as well. first dots, underscores and dashes got named in the label then within the parenthesis i get an eg and afterwards punctuations separated by commas. mentally my brain automatically tried to spot a pattern and draw the connection to the aforementioned dots, underscores and dashes.
i wonder wouldn't it be way more clear to shorten the label to Simplify sequences of dots, underscores and dashes with the replacement character. and add a description with one of the examples you've provided in #368 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed . imho that might be way more straight forward and clear? something like For example "Test_-_file.png" changes to "test_file.png" instead of "test_-_file.png"

2. and i think the aria-hidden should still be minded and kept in place for the second checkbox, label at least with the changes the latest patch introduced (in that case hide the parenthesis and it's content from screenreaders). the announcement for (eg _-_, .., --) is confusing (see and hear in the attached video).

Still seems slightly clumsy. How about:

"Reduce sequences of dots, underscores and dashes to a single replacement character."

Then provide an example in the description as mentioned above.

It might also be helpful to clarify that this occurs after an initial replacement of spaces.

This just fixes the tests from #370.

Looks good, with extensive test coverage.

One nitpick:

+++ b/core/modules/file/src/EventSubscriber/FileEventSubscriber.php
@@ -0,0 +1,110 @@
+    protected ConfigFactoryInterface $config,

Should this be named $configFactory?

• Removing 'Needs issue rescope' tag I previously added when this was blocked on other things. 😅
• Fixed #375.

We still need to decide if we're going to change the behavior and UI of the "Remove duplicates..." setting. I think I like the simplification, but it's going to take a bit of work to get it happy:

1. deduplicate_separators is no longer a very self-documenting config key for this setting. And its description needs help. 😉
2. The UI text for the settings form.
3. The actual implementation of this setting in FileEventSubscriber::sanitizeFilename()
4. The Unit test coverage.
5. New screenshots in the summary for the new UI
6. Update the text of the proposed resolution + UI sections in the summary.
7. Update the change record

Moving back to NR to get more eyes on this, but probably NW for all the above if we agree on changing this setting and how it works.

p.s. Starting to cleanup credit in here, removing from a few pointless/unproductive rerolls that the author then abandoned.

@rkoller found a small bug in Slack on the "Remove duplicates.." logic, this should address it with test coverage.

about #367.2 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed and in reply to #373 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed

I agree, my initial suggestion was sort of clumsy. But comparing your suggestion with the other labels the patch introduces i wonder if it would make sense to stick as close as possible to the pattern the previous checkbox label uses (Replace non-alphanumeric characters with the replacement character)?

Replace sequences of dots, underscores and/or dashes with the replacement character.
For example "One_test- ._--_. -file.png" changes to "One_test-file.png"

• As already mentioned I've tried to match the pattern of the previous sentence (Replace none-alphanumeric characters with the replacement character) as close as possible. Additionally I've just added an and/or to point out that it also applies to combinations of those characters. I've reflected that also in the updated example within the description.
• I've slightly changed the description. I've removed the "instead of"-part at the end and added "One_" at the beginning to demonstrate that single characters like _ remain untouched by the filter (as fixed in #377 ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed )

From the slack channel seems there is still work to be done around the "eg" part.

+++ b/core/modules/file/config/schema/file.schema.yml
@@ -24,6 +24,28 @@ file.settings:
+          label: 'replace sequences of dots, underscores and/or dashes with the replacement character'

Oops, "replace" should be uppercased.

1. Emojis are everywhere, and I'd like to have the option to transliterate them, as I commented in #337, but with the FileUploadSanitizeNameEvent added, it's easy to do in custom code.

Event subscribers can modify and sanitize the filename (but not extension) before the file is saved.

2. Meanwhile, I saw extension is not available for altering. Is there a good reason for that? I have not found it with a quick look.

Sorry, wrong changes in #380.2

+++ b/core/modules/file/file.module
@@ -1332,3 +1332,81 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
+ * These settings are enforced during upload by the FileEventSubscriber that
+ * listens to the FileUploadEvent::UPLOAD event.

The FileUploadEvent class does not exist. Updating the comment.

+++ b/core/modules/file/file.module
@@ -1377,8 +1377,9 @@ function file_form_system_file_system_settings_alter(array &$form, FormStateInte
+    '#description' => t('For example, "a _ file ._--_. name.txt" changes to "a _ file - name.txt".'),

After reading the discussion in slack, which @dww pointed me to (thanks!), I am removing the #description. And let's do it in the follow-up with the javascript approach to make the example dynamic depending on the selected replacement character.

One suggestion from me is to move the Replacement character form element to the top before deduplicate_separators , because in the label of deduplicate_separators, which mentioned replacement character, if the end users see the term Replacement character first which would make more sense.

Tagging "Needs followup"

@sokru: Personally I’d prefer option-1.png (first one), since “Transliterate” also uses replacement character.

Adjust the position of the Replacement character form element.

Update the description of Transliterate to mention the use of the replacement character.

So when I try to test the post_update hook I get

 Unable to decode output into JSON: Syntax error                                                                                                                                                                                      
                                                                                                                                                                                                                                       
  TypeError: ArrayObject::__construct(): Argument #1 ($array) must be of type array, bool given in ArrayObject->__construct() (line 15 of /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.ph  
  p).                                                                                                                                                                                                                                  
               

Anyone else getting that?

Disregard #387 got around that

post_update hook ran without issue

On Drupal 10.1 with a Standard install
Enabled all settings in File system.
Edited Article content type image field to be unlimited

Uploaded these files and were transformed to
new Test ..--__.png => new-test.png
new Test File!.png => new-test-file.png
new Test File.png => new-test-file_0.png
new_Test .. File.png => new_test-file.png (underscore not replaced)

Also when I upload the same file if it appends _0 should it not replace that underscore also?

new_Test .. File.png => new_test-file.png (underscore not replaced)

This is according the spec, the label says "Replace sequences of dots, underscores and/or dashes with the replacement character".

Also when I upload the same file if it appends _0 should it not replace that underscore also?

If this is desired, I'd leave this as a follow-up, since its handled on multiple places on core eg. in _file_save_upload_from_form().

Ah makes sense. Then my questions there are answered.

Though I did want to ask. I was only able to replicate once but when I uploaded a file that had been removed from the sites folder. I kept getting an error that the file that was being renamed already existed. Technically it did in the database but not file system. Didn’t flag that because I think it’s user error but wanted to ask before I mark

Technically it did in the database but not file system.

Drupal does not update the database when a file is deleted in the file system.

Thus, AFAIK, this is a user error.

Patch no longer applies.

Rerolled the patch, although the patch from #386 still applies to 11.x, as this is categorized as a "feature request" I don't think this will be backported to 9.x or 10.x branch.

The last submitted patch, 394: 2492171-394.patch, failed testing. View results →

Random test failure on #394, now it will pass.

Applied #394

Verified post update hook ran without issue
Verified UI appears as mentioned
Did the same file test as #388

Uploaded these files and were transformed to
new Test ..--__.png => new-test.png
new Test File!.png => new-test-file.png
new Test File.png => new-test-file_0.png
new_Test .. File.png => new_test-file.png (underscore not replaced)

Going to mark. Though shouldn't be merged until follow up from #384 is opened.

I can't believe this is finally RTBC! 🎉 Thanks to everyone who picked up the torch after I was too burned out trying to keep running with it. 😅

Here's that follow-up: ✨ Add JS to system_file_system_settings_form to dynamically update examples based on current settings Active

The last submitted patch, 394: 2492171-394.patch, failed testing. View results →

Random test failure.

The last submitted patch, 394: 2492171-394.patch, failed testing. View results →

The Drupal\Tests\user\FunctionalJavascript\UserPermissionsTest::testPermissionCheckboxes test failure seems unrelated here.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Just a reroll from 394, no interdiff.

@apaderno why was this put back to needs review?

I wonder if we should ship configuration to enable this in the standard profile, or even on new sites in general with the update setting things to off. However the reasons given for defaulting the config to off by default for existing sites (and possibly new sites) are good ones.

Some of the config options like remove duplicates I'm not sure about - does that really need to be an option? However I'm conscious we're at over 400 comments and eight years already, these would be small changes to make in a minor release if we wanted to change our minds later.

Tagging for 10.2.0 release highlights.

Committed 56e1b48 and pushed to 11.x. Thanks!

Thank you!

Change record also needs to be published!

Amazing! Thank you to everyone who worked on this issue.

The change is currently only commited to 11.x branch, but tagged for 10.2 release highlights - shouldn't it be cherry-picked to 10.x too?

10.2.x will be branched off 11.x, it is confusing but it saves retargeting MRs every six months, see https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →

oh thanks for this important info :) and hell yeah, it's great that this long awaited feature finally found its way into core - thanks!

oh thanks for this important info :) and hell yeah, it's great that this long awaited feature finally found its way into core - thanks!

Amazing work, @dww! 🤯👏

If you find your way here and you are still on Drupal 8, 9, you can use https://www.drupal.org/project/transliterate_filenames →

It also works on Drupal 10

Created a follow-up: 📌 Simplify & enable transliteration by default? Active , feel free to update it.

Automatically closed - issue fixed for 2 weeks with no activity.

Great work everybody, thanks! It's so nice to see improvements such as this one, and all the other ones getting included in Drupal core, making it incrementally better and better. ✨ Incremental filter for permissions page Fixed and ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed (this) are my two highlights in Drupal 10.2 → .

Shout-out to @smustgrave for help keeping Drupal core issues moving with his Needs Review Queue Initiative → . It's having a big impact now, keeping Drupal core issues moving and getting committed. We are all reaping the fruits now.

I have even had @smustgrave appear in one of my own Drupal core issues, reviewing and helping get it committed. Maybe you have also had the pleasure of having him help out in one of your issues?

I think @smustgrave is having such a huge impact because he bridges the often separated roles of a Scrum Master and a coder, skillfully wearing both hats simultaneously, acting sort of like a "Scrum Coder" :)

Imagine the velocity we could reach, if even more people helped out in the Needs Review Queue Initiative → ... Just spending ten minutes every other day could make a huge difference, and help keep the momentum in otherwise stalled issues.

See also Talking Drupal #426 - Needs Review Queue Initiative.

At first a big thanks to this new feature. Made parts of our custom implementation obsolete :)

Only one thing bothers me: why is the replacement character only trimmed at the right side of the filename and not on the left one, as well?

https://github.com/drupal/core/blob/10.2.x/modules/file/src/EventSubscri...

I think filenames like -some-filename.png are not so nice. What has been the reason to implement it that way? Couldn't find info in this issue log.

Please open a new issue for any additional refinements needed.