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

What's the status of this? Should #2504815: d6 to d8 migration throws integrity contraint warning with duplicate file paths expect a unique constraint to exist in the database target?

Is this going to make it for 8.0 or should this be moved to 8.1?

Yep, this is 8.1.x material at this point, so contrib could provide it for 8.0.x. Thanks!

Suppose it's time, but maybe this blocked on #1925684: Provide a "original filename" field for file entity.

I wouldn't consider this blocked on that one. They should be able to be worked on in parallel.

It's worth mentioning that this functionality is part of File (Field) Paths module, which has a D8 beta release.

It's worth mentioning that this functionality is part of File (Field) Paths module, which has a D8 beta release.

True but it seems like this functionality should be in core. This would also require the transliteration in core to support replacing spaces with a symbol which it, to my knowledge, currently does not. Also why the transliteration functionality in the File (Field) Paths module is limited compared to the old Transliteration module.

I'm not at all debating that it should be in core, but given @xjm's comment that it's not going to happen in 8.0.x, and that it needs to be dealt with in contrib, this is it being dealt with in contrib.

File (Field) Paths does also provide replacing of spaces with a symbol, as it has since inception (or not long afterwards) via the Pathauto integration.

I'm more than happy to remove the functionality from F(F)P as soon as it's no longer needed.

I have test with File (Field) Paths with Drupal8.0.1, it does not works at first. it works correctly at last.
I want to upload chinese pdf file using file field module.

That seems like a poorly worded statement. But I will be happy to look into it further in the appropriate issue queue.

Sorry for my English.

I have tried filefield paths to solve the problem of transliterating filenames. For me it dit not work. Some comments for filefield paths:

1. It seems to keep the original filename and create the new filename, which is some kind of alias. I think this is by design and in the sense of the module, but unneccessary for the simple goal of getting web-safe filenames.
2. One has do the setting "transliterate filename" in every single filefield, maybe even instance of every filefield. That is quite some work and can cause oversights.
3. There is a major problem with filefield paths: it cannot be uninstalled https://www.drupal.org/node/2685731 The module is not alone with this problem. The new uninstall system of Drupal 8 requires all database content of a module to be removed before you can uninstall it. If this content is not deletable by hand, there is a problem.

I don't think that the issue's topic is about File (Field) Paths and we should concentrate on implementing Transliteration for filenames manipulation in the Core.

Let's start with something.

Here's a patch using code similar to that from a transliteration module.

"Something" looks like a good start to me, at least it doesn't break any existing tests. Next step would be tests with file names that trigger this.

Also wondering about how to introduce this exactly, is there any reason to make this a setting, so it could be disabled? It will be a behavior change, although I suspect a very welcome one for most (all?) sites?

i'll be bold enough to say all sites should use.

The only other request I've ever had is about storing the original file name, that could be used for Title href. As people often name a file because they expect it would be saved. Same for Search API related issues. At this time, I haven't directly looked to see if the File API is yet doing that..

^I think there would be no need for an 'on/off' switch of transliteration if org. name is stored for about points.

$clean_filename = \Drupal::transliteration()->transliterate($file->getFilename(), 'en', '');
I should think 'en' shouldn't be hardcoded. sites will not all be english first.

opt-in

Makes sense to have that configurable via container param

I updated the patch #15.

1.

$clean_filename = \Drupal::transliteration()->transliterate($file->getFilename(), 'en', '');
I should think 'en' shouldn't be hardcoded. sites will not all be english first.

I replaced the hardcoded 'en' to $langcode with the following code:

      $langcode = \Drupal::languageManager()->getCurrentLanguage()->getId();

      // Transliterate and sanitize the destination filename.
      $filename = \Drupal::transliteration()->transliterate($file->getFilename(), $langcode, '');

2.

I changed the strtolower() to Drupal\Component\Utility\Unicode\Unicode::strtolower(). I consulted #1842718: Use new Transliteration functionality in core for machine names for this point. This may be unnecessary.

      // Force lowercase to prevent issues on case-insensitive file systems.
      $filename = Unicode::strtolower($filename);

3.

I introduced an option config to toggle the transliteration for the file names.

This probably should be opt-in (at least for existing sites), so as to not change behavior.

Probably it depends on the language that people want to make this setting opt-in or opt-out (of course, it shouldn't be changed for existing sites with update). In my small personal experience, the current transliteration function for Japanese is not very good and I'd like it to be off by default...

Tests are not yet added.

Patch in #21 tested and works. Thanks.

+++ b/core/modules/file/file.module
@@ -842,7 +842,28 @@ function file_save_upload($form_field_name, $validators = array(), $destination
+    if (\Drupal::config('system.file')->get('filename_transliteration')) {
+      $langcode = \Drupal::languageManager()->getCurrentLanguage()->getId();
+
+      // Transliterate and sanitize the destination filename.
+      $filename = \Drupal::transliteration()->transliterate($file->getFilename(), $langcode, '');
+
+      // Replace whitespace.
+      $filename = str_replace(' ', '_', $filename);
+      // Remove remaining unsafe characters.
+      $filename = preg_replace('![^0-9A-Za-z_.-]!', '', $filename);
+      // Remove multiple consecutive non-alphabetical characters.
+      $filename = preg_replace('/(_)_+|(\.)\.+|(-)-+/', '\\1\\2\\3', $filename);
+      // Force lowercase to prevent issues on case-insensitive file systems.
+      $filename = Unicode::strtolower($filename);
+    }

I'm wondering why this is part of file_save_upload(), which is part of an API which is related to the form, not some actual underlying API level. IMHO you would like to have the same for potential files uploaded via RESt one day.

Maybe it would be enough for now to move all that logic into its own API function and call it from here?

Thanks for your reviews. As dawehner told, surely it's better to make this logic an independent API function.

Maybe it would be enough for now to move all that logic into its own API function and call it from here?

I tried creating a new method getTransliteratedFilename() in \Drupal\file\Entity\File class and moved this new transliteration logic into it. I'm not sure where we should put this API... I'd like this patch to be fully reviewed. Thank you.

The patch #26 failed the auto test because I didn't change the code properly when I extracted the logic to a method. I'll try it again.

Tested patch in #28 with a file named with spaces and German umlaut -> works

If this is added to core it would be very helpful to offer an update function to transliterate the already uploaded files, wouldn't it?

Nope, that doesn't work. you don't know how and where those files are used, could be hardcoded in content. We can only offer it for new files.

In my eyes this answer makes no sense as this patch is about using transliteration on images uploaded by a form. Whenever an image uploaded for a field for example is used somewhere hardcoded the same problem as described in #31 would appear when simply a new image is uploaded. Whenever an image path/ name is used hardcoded although the image can be changed by a form it is a wrong programming, isn't it?

It would be good to have transliteration of file names in Drupal 8 - and really great to get it into Drupal 8.3, if possible.

One of the features why I have not yet completely switched to D8 for all my projects.
Would be great to have this in the upcoming 8.3 release.

In response to #30 and #31: perhaps add a drush command to transliterate existing files, if a site admin needs to.

+++ b/core/modules/file/file.module
@@ -842,7 +842,10 @@ function file_save_upload($form_field_name, $validators = array(), $destination
+    $filename = $file->getTransliteratedFilename();
+
+    $file->destination = file_destination($destination . $filename, $replace);

Earlier code in this function does a setFilename() if the filename is changed (see code that handles insecure file extensions at line 818). I'd recommend doing that here, too.

+++ b/core/modules/file/src/Entity/File.php
@@ -196,6 +197,38 @@ public function preSave(EntityStorageInterface $storage) {
+      // Remove multiple consecutive non-alphabetical characters.
+      $filename = preg_replace('/(_)_+|(\.)\.+|(-)-+/', '\\1\\2\\3', $filename);

This seems extraneous. Remove it?

Updated the patch #28 to work with Drupal 8.3.0.
Changes: new PHP array syntax.

I tried this patch and it works, thanks!

I think the failure in #37 is unrelated, but this does need it's own tests. Since we're adding a new public function for the file name transliteration, all we'd need to do is call that function with different file names in the test, I think?

We can test it directly in a kernel test with diferent variations. I think we also need at least one integration test, upload an image with some spaces or so to make sure the function is actually called.

This patch adds the kernel tests. Not sure if the test-class is the correct one.

Ah, those migrate tests are related, I don't see how those should be fixed though.

I believe the failed tests for #42 can be fixed just by providing the added configuration into $expectedConfig['system.file'] in the test classes.

       // temporary_maximum_age is not handled by the migration.
       'temporary_maximum_age' => 21600,
+      // filename_transliteration is not handled by migration.
+      'filename_transliteration' => FALSE,
     ],
     'system.image.gd' => [
       'jpeg_quality' => 75,

The test passed in my environment with this change and I'd like to see how the testbot reacts.

Oops, I'm sorry, the extension of the interdiff is wrong.

The tests passed. (The failure was just caused by the wrong extension of the interdiff.)

@@ -274,4 +275,37 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
     return $fields;
   }
 
+  /**
+   * Gets the transliterated filename.
+   *
+   * @return string
+   *   The transliterated filename. If the filename transliteration options is
+   *   disabled, the raw filename of the file.
+   */

Just found a typo:

If the filename transliteration options is disabled, the raw filename of the file.

"options" should be "option"

The patch works for me. Thank you.

Should not this transliteration be applied to the name displayed to the user?

Like this:

     $file->destination = file_destination($destination . $filename, $replace);

+    $file->filename = $filename;

Without transliteration in the name displayed to the user:

With transliteration in the name displayed to the user:

@J-Lee, thank you for the review. I fixed the typo.

@PascoDiogo, in my opinion, this transliteration should not necessarily be applied to displayed names. I believe Transliteration module's project page would help to understand transliteration.

I'm not sure with #50. The original filename is replaced by the transliterated name. Why should a not existent filename displayed to the user? Isn't it confusing if you got a filename shown but the real filename is different?

+++ b/core/modules/file/src/Entity/File.php
@@ -274,4 +275,37 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      $langcode = $this->languageManager()->getCurrentLanguage()->getId();

Shouldn't this use the file language instead?

Isn't it confusing if you got a filename shown but the real filename is different?

An argument could be made either way:

It is confusing to upload a file and see a different filename after uploading.
It is confusing to see the name of a file in the UI and then look for that file only to find out that it doesn't exist.

The first option is fixable by letting the user know that the filename will be changed upon upload for reasons. The second option would require adding a bit of UI to let the user know that the file name on the disk will be different than the one shown.

I lean toward the first option. Let the user know that the file name will be changed for reasons and then just display the actual file name.

#53/#55

+1 option1.
the file description should include (like it does for file size restriction) a li child that outlines that filename will be cleanup.

This is similar to the fact that drupal adds '_0' ' _1' (file counts) if use keeps re-attaching the same file name.

i know i've had clients want to have file name as they have make them (ie: '2017 Q1 earning.xls'). but then would have a bit of overhead on tracking related file name changes are 'repairing' them for file downloads etc. same issue when a User re-attached a new version of that same file name (_0 _1 _2....)

Knowing the name of the actual file on the server is also necessary for complying with DMCA takedown notices and the like.

i haven't looked over this whole issue in a while.

#4 andypost is correct.

When it will be integrated in CORE?

+1 for adding this to core. Not sure on postponing on #1925684: Provide a "original filename" field for file entity.. The feature is optional and the same problem (changing filenames) already occurs for duplicate filenames as mentioned in #56.

We could create a followup for the interface change mentioned in #56 "the file description should include (like it does for file size restriction) a li child that outlines that filename will be cleanup". It seems this particular part could turn into a huge bikeshed and I'm personally not convinced this is a big issue for most users. Hopefully, we don't have to block on that.

New patch to fix #54.

Thanks for the patch!

I've started using it on a project, but once I've enabled the configuration and exported it, the site installations started failing due to import of configuration issues. After some checks it turned out the config has stored 1 in the YML file, where the default value was false, so logically it should have stored true in there. Also hinted by the bool schema type.

I've changed the config manually in the YML file to true and installs passed.

I guess the fix should be to just type-cast the value, before setting it to storage from the form-submit handler.
This is also the fix implemented directly on the patch from #60. Diff between the two patch versions is provided.

I do not know should this regression needs any tests, so any help will be appreciated.

As I've done changed on the code still needs review :)

Patch from #64 seems to work for me (although spaces are converted to underscores (_) which I think is not ideal).

Work for me too always. :D

@yan, what isn't ideal about underscores?

@frob

There is a difference from a SEO perspective. See #1 in the article linked below:

https://searchengineland.com/9-seo-quirks-you-should-be-aware-of-146465

That article is from 2013 and the point you refer to is linked from 2005. Also, the files it is referring to are .html/.htm or in other words, that article is talking about the link to the content and not the link to the static asset.

SEO is constantly evolving and changing. Do you have a more current reference? I would argue that this should be configurable to make it long term SEO friendly, but I would also argue that should be contrib or another issue. I just don't want this issue to get side-tracked when it is so close.

From #61 Looks like it still needs work:

site installations started failing due to import of configuration issues. After some checks it turned out the config has stored 1 in the YML file, where the default value was false, so logically it should have stored true in there. Also hinted by the bool schema type.

I've changed the config manually in the YML file to true and installs passed.

I guess the fix should be to just type-cast the value, before setting it to storage from the form-submit handler.
This is also the fix implemented directly on the patch from #60. Diff between the two patch versions is provided.

Current article from Google states:

"We recommend that you use hyphens (-) instead of underscores (_) in your URLs."

https://support.google.com/webmasters/answer/76329?hl=en

This is consistent with the earlier article I shared, and I don't see any distinction in either between html URLs and image URLs.

Another reason to use hyphens here is that the Transliteration module in D7 used hyphens for file names, so IMHO it is desirable to remain consistent with that unless the core transliteration functionality is also using underscores (although my argument in favour of underscores hyphens would remain).

It appears that in this case, the use of an underscore is defined in one line of code in patch #61 that could be changed easily without affecting the patch overall.

Providing the option to choose the separator may be useful for some use cases, but agree that this would be better left for a separate issue for the core module rather than this patch.

@millionleaves this is an interesting discussion as I've always preferred underscores in filenames while keeping hyphens in the rest of the URL. It turns out this method could be problematic due to underscores being a special character in computing.

Google's examples all use hyphens: https://support.google.com/webmasters/answer/114016?hl=en

This could be used to our advantage in certain cases. For example, "big_red_socks.jpg" you might want people to find only if they're looking for "big_red_socks" and not "big", "red" or "socks". Maybe not very often, but it's good to have that level of control.

Yeah, it looks like it should be changed to a "-" character. The patch has always had this from #15

$clean_filename = str_replace(' ', '_', $clean_filename);

So that line needs to be changed and the issue from #61

Make the SEO change by rerolling the patch from 61, that leaves the issue mentioned in #61, can anyone review that?

This will also need fixes in the kernel-tests written for this issue, see testFileNameTransliteration in modules/file/tests/src/Kernel/SaveTest.php. Specifically the data-provider (provideFilenames) will need to be updated.

Fixed provideFilenames().

Patch in #72 should resolve all remaining issues. All that is left is manual review.

Could you provide an interdiff for #72?

I applied the patch#51 to my application. It only transliterating the filename in database but the file name still appears to be the original on UI for the uploaded file . any idea how do i fix the filename that appears on UI too.

See comments #50, #51, #53, #55, #56 and #60.

This is a point of discussion but could/should a follow up issue.

anyone tested patch #72?

Confirming that the patch in #72 is working.

The Patch #72 applies cleanly with Drupal 8.4.0.
The filenames for the uploaded files itself are correctly transliterated. It adds dashes for spaces.
The displayed filename are an issue for a followup, as I understand, so no surprise they are kept from the original filename.

I think the test failures look unrelated to this patch - back to NR to try the tests again.

Noting also that this was mentioned as a step toward being able to remove the workaround for the file uri uniqueness constraint added in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols), but it probably won't help much if it's configurable and opt-in. (The idea being that the uri field in the file_managed table could be set to ascii if that's all it ever contains, and as such there wouldn't be a problem with the key length when imposing a uniqueness constraint in the db schema).

Setting it back to @rootwork's RTBC. As @mcdruid noted, "test failures look unrelated to this patch".

1. +++ b/core/modules/file/src/Entity/File.php
   @@ -277,4 +278,36 @@
   +  public function getTransliteratedFilename() {
   +    // If the transliteration option is enabled, transliterate the filename.
   +    if (\Drupal::config('system.file')->get('filename_transliteration')) {
   

   This is a bit funny because calling getTransliteratedFilename() might not return a transliterated filename. Maybe it should be the caller's responsibility to check the config.

2. +++ b/core/modules/system/config/install/system.file.yml
   @@ -3,3 +3,4 @@
   +filename_transliteration: false
   

   Adding a configuration key like this means we need an upgrade path for existing sites. Just an update in system.install to add the missing key with the default value.

3. We still need a change record
4. The issue summary could do with an update to explain the solution. I.e. why is it configurable. Why the default for new sites is false.

Patch from #72 worked for me for Drupal 8.5.1

I would say that the update should set to FALSE for existing sites through the proposed update path in #86.

For new ones it should be TRUE by default, as file names transliteration is a generally a good thing to do on the web.

I am trying to upload image with special characters in filename, Drupal does not budge (drupal 8)
any news?
Drupal 6 managed to upload it with no problem
I am talking about special slavic characters like this

srečna družina.jpg

The patch in #72 together with Drupal 8.6.x works for me. I applied the patch using the new command line tool to quickly install Drupal:

1. curl -sS https://ftp.drupal.org/files/projects/drupal-8.6.x-dev.zip --output drupal-8.6.x-dev.zip
2. unzip drupal-8.6.x-dev.zip && rm drupal-8.6.x-dev.zip
3. cd drupal-8.6.x-dev
4. wget -q -O - https://www.drupal.org/files/issues/use_new_transliteration-2492171-72.patch | git apply -
5. php core/scripts/drupal quick-start demo_umami

After enabling filename transliteration under admin/config/media/file-system, it seems to work perfectly:

Original image name
01-a (SMALL!!!)wé↑s e3RQ#er24!"#øæå.jpg

Resulting filename, after upload
01-a-smallwes-e3rqer24oaea.jpg

I also tried uploading a file named srečna družina.jpg, and it was transliterated to srecna-druzina.jpg, as expected.

I agree with @ndobromirov:

For new ones it should be TRUE by default, as file names transliteration is a generally a good thing to do on the web.

So, I

• updated the IS
• created a change record (https://www.drupal.org/node/2972665)
• adjusted the code based on @alexpott's review in #86
• removed the deprecated code since #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill landed
• provided an interdiff for #72

+++ b/core/modules/file/file.module
@@ -989,7 +989,16 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
-    $file->destination = file_destination($destination . $file->getFilename(), $replace);
+
+    // If the transliteration option is enabled, transliterate the filename.
+    if (\Drupal::config('system.file')->get('filename_transliteration')) {
+      $filename = $file->getTransliteratedFilename();
+    }
+    else {
+      $filename = $this->getFilename();
+    }
+    $file->destination = file_destination($destination . $filename, $replace);

I would expect this replacement to be done at a lower level, for example directly in file_create_filename() because that would allow unmanaged files to take advantage of transliteration as well.

Is there any reason for not doing that?

Sorry for interference. If the discussion for the default value of filename_transliteration is not enough, I'd like it to be decided carefully.

Transliteration for filenames will be disabled by default, because it's not useful for some languages like Japanese.

Surely I myself (= a Japanese user) am happy if the transliteration is disabled by default. And I guessed there would be several other languages like Japanese but I started to doubt there're many languages which have a problem with Drupal's transliteration rule like Japanese since I haven't found any opinion similar to mine in this page.

If the out-of-box experience of Drupal gets much better for the majority in case the file transliteration is enabled, it's acceptable for minority users whose language have trouble with the transliteration, even if they need to disable the function every time after installation, I think :)

Thanks for the consideration.

For new ones it should be TRUE by default, as file names transliteration is a generally a good thing to do on the web.

Transliteration for filenames will be disabled by default, because it's not useful for some languages like Japanese.

see #18

This probably should be opt-in (at least for existing sites), so as to not change behavior

update existing site and how that could effect expectations by those product owners should be included, regardless of language first install base.

Fixing the tests

I think @amateescu might be correct in #94 that this should be done on a lower level. That way transliteration can apply to unmanaged file saves too. file_create_filename() is not suitable because it is only called when FILE_EXISTS_RENAME is set. In fact there's stuff in that method that definitely should not be there. So I've added it to file_destination().

I also think having File::getFilename() and File::getTransliteratedFilename() is super confusing so I think we should avoid patches that do that.

I agree with the concerns raised about the filename on disk and the filename on the file entity not matching. I know it's an existing issue with the _1 etc added to duplicates but this makes it even more confusing in terms of File::getFilename() not matching the actual filename. I think we should consider either fixing that first or making it part of this issue.

This sets the filename correctly in file_save_upload(). Needs tests.

+1 . This seems to work perfectly for me. Applied to 8.5.4

Patch in #100 works fine but causes issues when css/js aggregation is enabled and server cares about filename case sensitivity.

Modified that patch from @alexpott to come over the issue. Did not though think thoroughly if just .css, .js and .gz are fine to skip from transliteration.

Oops, bad patch file in #103, sorry about that. Here's a patch that applies with the change that js/css filenames won't be transliterated.

Fixing the test. I think this shows that the fix is on the right lines because the current expected message is Raw "Field <em class="placeholder">test_file_field_1</em> can only hold 3 values but there were 6 uploaded. The following files have been omitted as a result: <em class="placeholder">text-0.txt, text-0.txt, text-0.txt</em>." found

I'm not sure about the fix added by #104 - I think we need a test for this situation and also to understand the problem more. This patch includes it and the interdiff is back to #100 so everyone can see what the fix is.

+++ b/core/includes/file.inc
@@ -595,11 +595,20 @@
+  // Do not transliterate if the file to process is css, js or gz.
+  $pieces = explode('.', $destination);
+  if (in_array(array_pop($pieces), ['js', 'css', 'gz'])) {
+    $transliterate = FALSE;
+  }
+  else {
+    $transliterate = TRUE;
+  }
+

@kirkkala this is the change you added to fix CSS aggregation can you give some steps-to-reproduce the problem you are seeing?

I've tried to reproduce the issue detailed by @kirkkala by:

1. Using a case sensitive filesystem
2. Installing standard
3. Ensuring the CSS and JS aggregration is enabled
4. Having the patch attached applied - it doesn't have @kirkkala's changes
5. Running the following snippet against the site: \Drupal::configFactory()->getEditable('system.file')->set('filename_transliteration', TRUE)->save();
6. Clearing the cache
7. Refreshing a page view which will rebuild the aggregates
8. Nothing is broken

I've even set debug break points in file_destination() and as far as I can see this method is not called during CSS/JS aggregation.

I had been on #2492171-100: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) and experienced #2991606: Filename Transliteration enforces lowercase filenames which causes 404 errors with CSS/JS Aggregation turned on mentioned by @kirkkala in #104. I have written a test for that which I believe applies to #106

My solution was to change the AssetDumper service to ensure it creates a lowercase filename when filename_transliteration is enabled, so I have that in the wings as well. I am updating this issue with the test for that, which should check if #104 is a suitable solution as well.

See attached patch and interdiff.

@arosboro awesome! No idea why my debugging didn't work but your test totally proves the problem.

+++ b/core/includes/file.inc
@@ -583,12 +583,47 @@ function file_build_uri($path) {
+    // Force lowercase to prevent issues on case-insensitive file systems.
+    $basename = mb_strtolower($basename);

This should be discussed in a followup issue and not part of this patch.

Patch attached removes this and adds the test from #109.

+++ b/core/includes/file.inc
@@ -583,12 +583,36 @@ function file_build_uri($path) {
+    $basename = \Drupal::transliteration()->transliterate($basename, $langcode, '');
+    // Replace whitespace.
+    $basename = str_replace(' ', '-', $basename);
+    // Remove remaining unsafe characters.
+    $basename = preg_replace('![^0-9A-Za-z_.-]!', '', $basename);
...
+    $basename = preg_replace('/(_)_+|(\.)\.+|(-)-+/', '\\1\\2\\3', $basename);

So here's a question can these rules futz with a name produced by \Drupal\Component\Utility\Crypt::hashBase64() - I think it is possible because I think it is possible to generate a base64 encoded string with several consecutive non-alphabetical characters.

I think we need to change how we're deciding to do transliteration and this should only occur on files uploaded by users and not generated files like CSS/JS aggregates.

So I've moved the transliterating of filenames to file_save_upload() as this is the method through which all files uploaded to the site are vaildated and processed. I don't think that filename transliteration should be part of the File entity as it is something that is done only when uploading the file to the server.

I used patch and its latest version worked nice! Thanks to the creator! Cheers!

Tested, worked

It works great, but I still don't understand why it is not enabled by default on a fresh installation ... Just to emphasize, I don't think it should be enabled for existing installations.

I'll be reviewing this at DrupalEurope

The patch applied cleanly to a new 8.7.x and a 8.6.1. However, the test fails for 8.7.x. The file label cannot be found, because the waitForLink function returns Null.

1. +++ b/core/modules/file/file.module
   @@ -989,7 +990,24 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +      // Remove remaining unsafe characters.
   +      $filename = preg_replace('![^0-9A-Za-z_.-]!', '', $filename);
   +      // Remove multiple consecutive non-alphabetical characters.
   +      $filename = preg_replace('/(_)_+|(\.)\.+|(-)-+/', '\\1\\2\\3', $filename);
   

   Unexpected characters and sequences are properly being stripped. Languages like Japanese and Russian are being converted to a Latin alphabet.

2. +++ b/core/modules/system/config/install/system.file.yml
   @@ -3,3 +3,4 @@ default_scheme: 'public'
   +filename_transliteration: false
   

   Default is set to false, as discussed above. I think this is the logical decision, due to the issue with some languages and not everyone needing to translate filenames.

3. +++ b/core/modules/system/system.install
   @@ -2173,3 +2173,12 @@ function system_update_8501() {
   +
   +/**
   + * Set filename_transliteration config to the default value.
   + */
   +function system_update_8701() {
   +  \Drupal::configFactory()->getEditable('system.file')
   +    ->set('filename_transliteration', FALSE)
   +    ->save();
   +}
   

   Files are being defaulted to false as discussed above.

After discussion with @LaravZ I think personally think that we need to change \Drupal\file\Plugin\rest\resource\FileUploadResource as well, given that you expect transliteration as well.
Maye we could extract all the transliteration logic into a helper function and call it from the upload resource.

@dawehner maybe rest can have this optional/configurable per request option? makes sense to file separate issue

Thanks! Patch is worked.

I bet it needs usability review

this is default in new drupal? But I cant find the settings....
I went to content type story...than edit fields...and edit image field and I cant find the setting

sopranos, use patch from #113

#113 can't be applied to newest drupal8.7.x and drupal8.6.x any more, reroll it for drupal8.7.x(just file.module, and nothing functional change)

Note that using this patch with the update function is a very bad idea because this adds a 87xx update function, which tells Drupal to ignore all future 86xx update functions.

It's not required anyway since FALSE/NULL is the implicit fallback, so I would suggest you provide an alternative patch without that update function that people can use if they are using the patch.

rerolled without update hook 8701

I think this additional feature should be in a module like D7 does in https://www.drupal.org/project/transliteration, because only few sites need special transliteration. While core only provide basic transliteration only.

I disagree with #131. Content creators name their files all kinds of weird and unfortunate things. Even if everything is in English, it's still great to convert spaces to dashes, remove weird punctuation, special characters, etc. Plus, many users find themselves in a mix of case-sensitive and case-insensitive filesystems, so it's great to have Drupal automatically convert everything to lowercase. I constantly build custom functionality into my sites to cleanup filenames on upload so as to prevent the editors from making a mess of things. It'd be much better if core did that for me automatically.

In short: even if you don't think you want this, you probably do. That's a good reason for core to do it, instead of forcing site builders to look for a contrib solution to a problem they don't necessarily realize they have. :)

Cheers,
-Derek

I think the problem is that transliteration only really makes sense for languages with latin characters.. if you try to use it with files that have chinese, arabic etc. characters than you just receive garbage.

That said, it is important to be configurable (maybe it would even need to be configurable by language, but that likely doesn't make much sense, as people might upload chinese filenames when adding an english translation or something) but I don't think it needs to be a separate module. It could maybe be a global setting provided by system.module that different things could check.. if you don't want to transliterate filenames, you probably also don't want to do it for pathauto aliases. But again, who knows, maybe someone does ;)

I think we could say any site wants rules to cleanup file names. This would be removing undesirable stuff like whitespace, control characters, invalid bytes or extreme length from file names, and case and unicode normalization to avoid cross-file system issues. In Drupal this all got agglomerated into "transliteration", which is more clearly optional. There's nothing wrong with 🐈.txt or any other non-ASCII as a file name. The URL would be http://example.com/%f0%9f%90%88.txt which isn't super pretty but works fine.

According to the IS this has a technical limitation as well.

@catch mentioned in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) we may want to transliterate filenames in core, so that we can have a database-level unique constraint on the URI field in the database.

Look back 130 comments for the discussion as to why this should be in core. I am not trying to say we should have the discussion, rather I am saying that we already had the discussion.

We need a re-roll so tests pass again.

#131 - i can't image a project not needing it. If you've never had a client/editor upload a file with a space in the name and then wonder why they can't embed a link in a WYSIWYG -- then you're luck to have such a tight scope on your projects.

🐈.txt

The basic point of clean names is ensure names are possible to type on a regular keyboard. Certainly MySQL storage of characters needs to match ability.

This base issue does outline having a setting. I'm not directly aware of where the patch is at this point -- hopefully it's not lost.

Also we had previous conversation here about tracking original names #4 has a sub issue.

EDIT
===
check for get('filename_transliteration') config exists.

#132 I do not have much experience with chinese, arabic etc. characters, but for cyrillic characters, I very much rely on transliteration.

Reroll for 8.6.7

UPDATE: This one messes up CSS/JS asset filenames when aggregation (preprocess) in enabled though. ¯\_(ツ)_/¯

Re #139

This one messes up CSS/JS asset filenames when aggregation (preprocess) in enabled though. ¯\_(ツ)_/¯

Yep this is why we can only do this for user uploaded files and not all files generated by Drupal.

Here's a reroll of #113 for 8.7.x with the update function re-instated and at least one of the tests fixed.

Updated the issue summary.

+++ b/core/modules/file/file.module
@@ -1022,7 +1023,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+  $filename = $file->getFilename();
+  if ($system_file_config->get('filename_transliteration')) {
+    // Transliterate and sanitize the destination filename.
+    $filename = \Drupal::transliteration()
+      ->transliterate($filename, $file->language()->getId(), '');
+    // Replace whitespace.
+    $filename = str_replace(' ', '-', $filename);
+    // Remove remaining unsafe characters.
+    $filename = preg_replace('![^0-9A-Za-z_.-]!', '', $filename);
+    // Remove multiple consecutive non-alphabetical characters.
+    $filename = preg_replace('/(_)_+|(\.)\.+|(-)-+/', '\\1\\2\\3', $filename);
+    // Force lowercase to prevent issues on case-insensitive file systems.
+    $filename = mb_strtolower($filename);
+  }

One thing I wonder is if this should be:

  $filename = $file->getFilename();
  if ($system_file_config->get('filename_transliteration')) {
    // Transliterate and sanitize the destination filename.
    $filename = \Drupal::transliteration()->transliterate($filename, $file->language()->getId(), '');
  }
  // Force lowercase to prevent issues on case-insensitive file systems.
  $filename = mb_strtolower($filename);

I wonder:

• Is whitespace replacing necessary and solving problems for anyone?
• Is the "Remove remaining unsafe characters." really correct after transliteration?
• The replace multiple consecutive non-alphabetical would annoy me as a user. If named a file I---like---a---hyphen.txt and the system renamed it I-like-a-hyphen.txt I'd be annoyed

If we did the above suggestion then system.file:filename_transliteration would really only mean transliteration which seems a good thing. And the case-insensitive vs case-sensitive filesystems are a fact of like and so it seems worthwhile always doing. We problem should add a message a tell the user why this has occurred if it has though.

For me lowercasing everything would be a much larger intrusion and annoyance than replacing spaces or consecutive hyphens (which both seems legitimate to do). Plus not many Drupal setups will be on a mixture of Windows and Linux machines.
Even then: be smart, not intrusive. Instead of enforcing lowercase, enforce case insensitivity.

Also, spaces are usually replaced by underscores. Multiple hyphens are rather hard to read, especially if some of them are actual hyphens, while others are whitespace replacements.

„Force lowercase“ (why not force uppercase?) and the whitespace replacement character at the very least need to be configurable. But then it might be something for contrib.

We might possibly want to only do a minimalistic Drupal::transliteration()->transliterate() and leave the whole rest to contrib.

I changed the description a bit, since Thunder is not actually shipping this patch, but implemented a custom solution similar to this.
The reason for introducing a similar mechanism in the Thunder distribution were faulty http clients. E.G. the OkHttp client which is used in several mobile apps has a bug, where it double encodes urlencoded image names.
So technically only important for us is:

• Transliterate the filename.
• Replace whitespace with -
• Remove remaining characters other than 0-9A-Za-z or - or . or _

While reducing multiple consecutive characters or lowercasing is not required on production systems. But for developers, it might be interesting to also have the lowercasing, since Windows, as well as OSX, have case insensitive filesystems.

Remaining tasks
Agree exactly what should occur when system.file:filename_transliteration is TRUE.

Apart from transliterating what else should we do? Currently we:

Replace whitespace with -
Remove remaining characters other than 0-9A-Za-z or - or . or _
Remove multiple consecutive non-alphabetical characters ie --- or ... or ___ with be replaced with - or . or _
Force lowercase to prevent issues on case-insensitive file systems.

What about if we made configurable theses options, once the filename transliteration option is checked ? In this way, many use cases could be satisfied ?
At least :

- Replace whitespace with - or _ => select list, default is _
- Remove multiple consecutive non-alphabetical characters ie --- or ... or ___ with be replaced with - or . or _ => checkbox TRUE/FALSE, default is FALSE
- Force lowercase => checkbox TRUE/FALSE, default is FALSE

Agreed with #147, except there should be a "don't replace" option (or something) in the select list for "Replace whitespace". Otherwise, yes, I'd love it if core provided these options, instead of punting this to contrib. We're so close here to something that will satisfy almost everyone's use-case.

Thanks,
-Derek

p.s. The top-level checkbox could be "Transform filename", and if selected, reveals all the other settings:

[X] Transliterate
[ ] Force lowercase
...

The config setting IDs could be of the form:

filename_transform
filename_transform_transliterate
filename_transform_lowercase
filename_transform_convert_whitespace
filename_transform_only_alphanumeric //..._and_separators (?) ugh.
filename_transform_strip_multiple_punctuation // or ..._strip_multiple_separators (?)
...

What about if we made configurable theses options, once the filename transliteration option is checked ?

++ :)

Agreed with #147

++ :)

This would be awesome and definitely better than doing just a minimum and leaving the rest to contrib. I was just anxious we would force an overly strict ruleset on users who would in many cases feel disenfranchised. Now I know (and agree) we don't unnecessarily add clutter to the Core UI. However, I believe that this feature warrants some configurability.

Even if configurable, I still think that instead of lowercasing filenames, we should enforce case insensitivity in our files table. However that clearly needs to be investigated further.

So let's go forward as proposed by @flocondetoile and Derek. I hope noone's angry if for now I'm setting the issue to "needs work."

Here's a start.

Reviews of the UI text and config key names would be great.

We need more test coverage of how the sanitisations play together.

@alexpott re: #151:

Looks great, thanks! This is getting close. Agreed it needs more tests. Haven't manually tested, but here's what I see looking at the patch:

1. A few unrelated 80 char hunks at the front make it less readable as a patch.

2. We're inconsistent about "whitespace" vs. "spaces" in the config/variable names, UI text and how it actually works. Is this just for spaces or is it all whitespace (tabs, newlines, etc)? Most of it points towards all whitespace. I think we should standardize on that, expand the str_replace() to actually work that way, fix the UI text to match, etc.

3. Do we want to dispatch an event to let contrib easily continue to tweak the filename before we use it, or is that too much scope creep? How else do we expect contrib to extend this?

4. I don't see the "-- disabled --" pattern anywhere else in core. "- None -" seems much more common. If we keep "disabled" it should be "- Disabled -". Maybe "- Do not replace -"?

5.

+      '#title' => $this->t('Remove non-alphanumeric characters'),
+      '#default_value' => $config->get('filename_sanitization.remove_non_alpha_characters'),
+      '#description' => $this->t('Remove non-alphanumeric characters exception dots, underscores or dashes.'),

Description repeats title and contains a typo. Maybe this:

      '#title' => $this->t('Remove non-alphanumeric characters'),
      '#default_value' => $config->get('filename_sanitization.remove_non_alpha_characters'),
      '#description' => $this->t('Alphanumeric characters, dots, underscores and dashes are preserved.'),

? "...are allowed."?

6. I know I just suggested 'transliterate' should be an option, but this is still in the summary:

@catch mentioned in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) we may want to transliterate filenames in core, so that we can have a database-level unique constraint on the URI field in the database. (However, this would presumably mean that transliteration of the field could not be optional).

Do we want to force transliteration to help resolve @catch's concern, and make everything else optional? Or do we keep this a config knob and if people run into that problem, they have to turn this on? Seems non-ideal. Should we at least default to on? A bulk rename of already-uploaded files is way beyond scope for this. :/ Should we open a follow-up about that? Does it already exist?

Thanks!
-Derek

Some more comments:

+    filename_sanitization:
+      type: mapping
+      label: 'Uploaded filename sanitization options'
+      mapping:

"sanitize_filenames" would be easier to grasp. We have "temporary_maximum_age", "default_scheme" and "path", but also "allow_insecure_uploads." On the other hand, this is a container, not a boolean. So yeah, maybe keep 'filename_sanitization.'

+        transliterate:
+          type: boolean
+          label: 'Transliterate'
+        replace_whitespace:
+          type: string
+          label: 'Replace whitespace'
+        remove_non_alpha_characters:
+          type: boolean
+          label: 'Remove non alphanumeric characters except dot, underscore and dash'

Maybe "strip_non-alphanumeric"?

+        dedupe_consecutive_separators:
+          type: boolean
+          label: 'Deduplicate consecutive dots, underscores and dashes'

"dedupe_separators" is enough. If they're not consecutive, they're no duplicates.

+        force_lowercase:
+          type: boolean
+          label: 'Force lowercase'

+    $form['filename_sanitization'] = [
+      '#type' => 'fieldset',
+      '#title' => $this->t('Filename sanitization options'),
+      '#tree' => TRUE,
+    ];

Maybe just "Filename sanitization" without "options"?

+    $form['filename_sanitization']['transliterate'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Enable filename transliteration'),
+      '#default_value' => $config->get('filename_sanitization.transliterate'),
+      '#description' => $this->t('Transliteration ensures that filenames do not contain unicode characters.'),
+    ];

How about "Transliterate filenames"?
We're replacing not only characters that are exclusively Unicode, but all non-ASCII characters. So how about "Transliteration ensures filenames contain only ASCII characters. It is recommended to keep transliteration enabled."

+    $form['filename_sanitization']['replace_whitespace'] = [
+      '#type' => 'select',
+      '#title' => $this->t('Replace spaces with the selected character'),
+      '#default_value' => $config->get('filename_sanitization.replace_whitespace'),
+      '#options' => [
+        ' ' => $this->t('-- disabled --'),
+        '-' => '-',
+        '_' => '_',
+        '.' => '.',
+      ],
+    ];
+
+    $form['filename_sanitization']['remove_non_alpha_characters'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Remove non-alphanumeric characters'),
+      '#default_value' => $config->get('filename_sanitization.remove_non_alpha_characters'),
+      '#description' => $this->t('Remove non-alphanumeric characters exception dots, underscores or dashes.'),
+    ];

"Strip non-alphanumeric characters except for dots (.), underscores (_) or dashes (-).'

+    $form['filename_sanitization']['dedupe_consecutive_separators'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Remove duplicate consecutive dots, underscores or dashes'),
+      '#default_value' => $config->get('filename_sanitization.remove_non_alpha_characters'),
+    ];

"Remove duplicate dots (..), underscores (__) or dashes (--)"

+    $form['filename_sanitization']['force_lowercase'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t('Convert all filenames to lowercase'),
+      '#default_value' => $config->get('filename_sanitization.force_lowercase'),
+    ];

"Convert filenames to all lowercase."

I think we need a #state on each option to hide them on the config form if the feature is not enable. This way, all the options will not be visible until the checkbox transliterate is checked.

+1 to all the notes in #153.

-1 to using #states from #154. None of these features depend on using "transliterate". That's simply one kind of sanitation a site might want. But you could still want to strip whitespace even if you're not transliterating. As @mfb pointed out at #134:

In Drupal this all got agglomerated into "transliteration", which is more clearly optional.

.

If we wanted a global "turn on any of this stuff" option, it should be called "[X] Sanitize filename", not "Transliterate". But, I think that complication is unneeded. It's a small fieldset of distinct options, so let's leave them all visible by default.

Thanks,
-Derek

No reason for states, meantime makes sense to give a room for contrib to extend

At lease token and https://www.drupal.org/project/field_tokens

Re-title, move component (since this is about more than transliteration now), and summary update to reflect the current state.

A few more edits. Also fleshed out the Remaining tasks. Currently:

1. Decide if all whitespace or only spaces are converted. Update code and UI text to match.
2. Design and dispatch the appropriate event(s) after (? and/or before?) core's sanitation is done.
3. Decide if we give the user enough feedback about renaming their file upon upload. If not, implement the appropriate feedback.
4. Finish cleaning up the UI text, settings names, etc.
5. Final code cleanup and fixes.
6. Finish writing test cases for (all?) the combinations of settings.
7. Upload screenshot of final settings UI and add to the summary (@see @todo). ;)
8. Final string/UI review + signoff. If we get this in by 8.7.0, tag for "String change in 8.7.0".
9. Write change record.
10. Final reviews + RTBC.
11. Commit.

Getting close. ;)

Cheers,
-Derek

#152.1: Fixed.
#152.2: Fixed in favor of all whitespace, not just spaces. Crossed that off from the summary remaining TODO.
#152.3: Added @todo for now. ;)
#152.4: Fixed. Using '- None -'. While I was at it, I made the labels for the other choices readable. Very hard to tell - from _ in the previous version.
#152.5: Fixed (mostly via the text proposed in #153). Now:

      '#title' => $this->t('Strip non-alphanumeric characters'),
      '#description' => $this->t('Alphanumeric characters, dots (.), underscores (_) and dashes (-) are preserved.'),

Fixed all of #153, except I further simplified these:

      '#title' => $this->t('Transliterate'),
...
      '#title' => $this->t('Convert to lowercase'),

Still needs work (for tests and other things), but back to NR for the test bot. I'm done for tonight, if anyone else wants to pick up the torch. ;)

Thanks!
-Derek

Whoops, I forgot to update the settings in the test. ;) Sorry about that.

Ooof, I realized I partly renamed "remove_non_alphanumeric" to "strip_non_alphanumeric" so that the setting name matched the UI text, but I was inconsistent. This should actually work. ;) Sorry for the noise (and bot churn). My local testing world stopped working suddenly, and I don't have time to get that happy again right now.

Summary updates:
- Fixed typo in remaining tasks generating an empty todo.
- Add link to #159 about whitespace.
- Add settings label to each config key in the UI section.

Not sure exactly what the bot is confused by, but this should work.

Thanks for all the work on this. @dww you mentioned:

@catch mentioned in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) we may want to transliterate filenames in core, so that we can have a database-level unique constraint on the URI field in the database. (However, this would presumably mean that transliteration of the field could not be optional).

It would be good to remind ourselves what would need to happen to filenames (actually uris) in core in order for us to restore the constraint in the db. (Especially as the code-level constraint is not yet properly enforced - see #2869855: Race condition in file_save_upload causes data loss).

Perhaps that aspect of this change needs to split off into a separate issue? Ideally we wouldn't make that part of the filename processing optional, as otherwise we're stuck without the database-level constraint.

I'll try to have a closer look at this ASAP but wanted to bring this up so that it's not overlooked.

Sorry, my regexp for replacing all whitespace had a typo (/s vs \s). It also had a thinko, since I was telling it to replace 1 or more (via +) (since that's what my custom code usually does), but really we want individual replacement (since dedupe is now a separate setting). Got my local test world happy again, and at least Drupal\Tests\file\Functional\SaveUploadTest now passes locally, so I think the bot should be happy with this.

For fun, I'm also attaching an interdiff relative to #151 to easily review everything I've done since @alexpott's last patch.

Re: #167: Yeah, sounds good. Thanks for the link and the info. Agreed that needs more thought, and perhaps should be handled separately.

Thanks,
-Derek