Allow file extensions containing underscores

Thanks, this is a feature request in my opinion.
In terms of testing, there should be an existing test case you can add to

#7 The classification for the category is explained in the issue handbook, see https://www.drupal.org/community/contributor-guide/reference-information...

Attached patch adds a test assertion a file extension containing an underscore can be configured through the user interface.

After applying the patch it is allowing to save the "_" (extension) , but while uploading the same it is giving an issue . This is giving an issue while uploading the file with "_" extension. This is not full filling the expectation , please look at the screen shot after applying the patch.

#9 LGTM and works fine for me for FIle field
Testing steps
1) Added x_t extension in article content type and.FIle field saved succesfully.
2) creating new node and uploaded "x_t" file into that file field and attached succesfully.
3) node saved successfully and again checked on edit page it remains attached file.

Just in checked patch that it has only functional test cases, not sure but might we should have more test cases on that.

@janmejaig I think you are trying to check _ (underscore) named extension in image field. but as per Issue summary this patch is allowing _ in file field.

I have recheck the above issue again and found that it is working fine for now . Please find the screen shot for the same .

Set back to RTBC because the failed test looks a random fail.

I run the test locally and did not find ant problem. See image attached.

I think a CR is required, so tagging "Needs change record"

I'll provide it.

I just create a CR. It is simple one but I think it is enough to understand. Please have a look.

Thanks, @paulocs.

BTW, Made a few little changes to it:

• Selected Module developers under Impacts
• Fixed a typo: exemple.x_t -> example.x_t
• Changed the word File in title to use lowercase

Changing the word File in title to use lowercase as well

An irrelevant random fail

There's at least one other file type with _ in the apache mime type file - application/vnd.denovo.fcselayout-link fe_launch.

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -227,7 +227,7 @@ public static function validateExtensions($element, FormStateInterface $form_sta
-      if (!preg_match('/^([a-z0-9]+([.][a-z0-9])* ?)+$/', $extensions)) {
+      if (!preg_match('/^([a-z0-9_]+([.][a-z0-9_])* ?)+$/', $extensions)) {

I think the regex should be ^([a-z0-9]+([._][a-z0-9])* ?)+$

I.e. I don't think we should allow _ and the beginning or the end and like a dot I think we should only allow one of them.

Updated the regex as mentioned in #28

Tested with following extensions:
x_t, x.t, xt, x_y_t - accepted
x_.t, x._t, xt_, x__t, _xt - not accepted

The last patch works fine .

+++ b/core/modules/file/tests/src/Functional/FileFieldValidateTest.php
@@ -160,6 +160,14 @@ public function testFileExtension() {
+    $this->drupalPostForm("admin/structure/types/manage/article/fields/node.article.$field_name", $edit, 'Save settings');

drupalPostForm is deprectated.

Replacing the deprecated drupalPostForm method with the combination of drupalGet and submitForm methods.

Fixed the failing test.

Tested with following extensions:
x_t, x.t, xt, x_y_t - accepted
x_.t, x._t, xt_, x__t, _xt - not accepted

I think the above should be tested in the test(s).

I was thinking the same thing. Back to NW for #36

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -166,7 +166,7 @@
+      '#description' => t('Separate extensions with a space or comma and do not include the leading dot or underscores.'),

@@ -228,7 +228,7 @@
+        $form_state->setError($element, t('The list of allowed extensions is not valid, be sure to exclude leading dots or underscores and to separate extensions with a comma or space.'));

Since we are changing these lines can we also change t( to $this->t

Also, I think the messages are confusing and don't give me any understanding of what is allowed and not allow, particularly regarding underscores. In the first one, what does 'or underscores' mean. Does that mean that more than one underscore after the dot is not allowed? Is one underscore OK? Is '__a' accepted? Then the second message mentions 'leading dots' where as the first message is 'leading dot'. Can there be multiple dots? On yet another read, it is the addition of 'or underscores' that is causing problems for me.

I would say just revert these changes but then this is allowing underscores but only in certain positions in the extension and not two consecutive underscores. How to tell that to the user?

@quietone Thanks for reviewing.
I knew this would come up. I look quite complex to explain all the possible values which can be added. Maybe we can show accepted and not accepted cases in the description or If you can provide eny other input.

@anmolgoyal74, Before I can think about the interface test I want to understand why there are limitations on the use of the underscore. Looking back I see it is from alexpott in #27.

I.e. I don't think we should allow _ and the beginning or the end and like a dot I think we should only allow one of them.

Why? For example, what is the problem with this filename?

$ ls -la f*
-rw-r--r-- 1 root root 4 Jan 15 21:07 foo.___

@quietone
I have followed the suggestions from @alexpott in #27.
In my opinion, there is no problem with foo.___, but that is not a known extension. you can create a file with any extension like that.

$ ls -la fo*
-rw-r--r--  1 root       staff     4 Jan 18 22:43 foo._d:d@#$%6_3

I have the following document, I think we are good with the latest patch.
Check here https://en.wikipedia.org/wiki/List_of_file_formats.

@anmolgoyal74, thank you for the explanation. So, this just needs to have agreement on the user interface help text and to use $this->t.

So, the underscore is treated differently than [a-zA-z] and the dot. The file extensions can include underscores but not as the first or last character, multiple underscores are allowed in the extension but an underscore can not be followed by an underscore.

I am not good at user interface text and think this needs input from the UX folks. I considered setting to RTBC and having a follow up for the text but the text as it is now is confusing, at least to me, so I rejected that idea.

I have updated the IS and added some screenshots showing the changed text.

Change t() to this->t().
And also tried to update the description.

Updated tests.

I talked with benjifisher who suggested that this gets tagged, 'Needs usability review', and to post in #ux or in the agenda for the next Usability meeting. So adding tag and I'll do the others too.

Updated the IS with the latest text and screen shots, which required a patch reroll. :-(

I got a message from jhodgson on #ux that the best way to get this reviewed at the ux meeting is for someone to show up and explain what is needed. The next meeting is Friday 15:00 UTC. I really don't want to attend because that is Sat 0400 for me. Can anyone here attend and get the interface text discussed?

Looking at the patch from #49, the existing help text is

Separate extensions with a space or comma and do not include the leading dot.

and the proposed help text is

Separate extensions with a space or comma and should start & end with an alphanumeric character and can contain a dot or an underscore but not together.

If the validation fails, the original error message is

The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space.

and the proposed error message is

The list of allowed extensions is not valid, be sure to exclude leading or consecutive dots & underscores and to separate extensions with a comma or space.

I think having the text like this, as text, makes it easier to review than having it in a screenshot. That is especially true for people who are visually impaired.

Usability review

We discussed this issue at #3194390: Drupal Usability Meeting 2021-01-29.

General rules for interface text include

• Help text should be kept short.
• Keep sentences short.
• Avoid contractions and abbreviations.

Some of this is specified on Interface text. It does not mention "&" vs. "and", but I think it should.

Following these guidelines, we suggest the following for the help text:

Separate extensions with a comma or space. Each extension can contain alphanumeric characters, '.', and '_', and should start and end with an alphanumeric character.

In order to keep it short, we leave out the rule that '.' and '_' cannot appear next to each other. Although it is annoying to be surprised by such a rule after submitting the form, we want to keep the text short and it seems unlikely to come up.

The error message can be a little longer than the help text, and it should be more explicit. We suggest

The list of allowed extensions is not valid. Allowed characters are a-z, 0-9, '.', and '_'. The first and last characters cannot be '.' or '_', and these two characters cannot appear next to each other. Separate extensions with a comma or space.

I am setting the status to NW for the text changes and an update to the issue summary. Please add the text changes to the "Proposed resolution" section and update the screenshots in the "User interface changes" section.

Updated the text as per the proposed solution.

@benjifisher, thank you for the UX review. I hope to remember to show the strings in the future instead of screenshots.

#52. Added the string changes to the Proposed Resolution in the IS. The screen shots were updated in #55

I applied the patch and tested with valid and invalid field extensions, 'a.b', 'a..', 'a__'. With invalid options I found that the error message gave enough information so I could fix the problem. And the help text was clear and concise.

I then confirmed that the text changes exactly matched the suggested changes. (Probably should have done this first)

Both invalid and valid extensions are tested so I reckon this is ready.

@anmolgoyal74, thanks for your commitment to getting this in.

This all looks really good. A recent fix has added testing of very similar functionality...

+++ b/core/modules/file/tests/src/Functional/FileFieldValidateTest.php
@@ -160,6 +160,27 @@ public function testFileExtension() {
+    // Check that a file extension with an underscore can be configured.
+    $edit = [
+      'settings[file_extensions]' => 'x_t x.t xt x_y_t',
+    ];
+    $this->drupalGet("admin/structure/types/manage/article/fields/node.article.$field_name");
+    $this->submitForm($edit, 'Save settings');
+    $field = FieldConfig::loadByName('node', $type_name, $field_name);
+    $this->assertEquals('x_t x.t xt x_y_t', $field->getSetting('file_extensions'));
+
+    // Check that a file field with an invalid value in allowed extensions
+    // property throws an error message.
+    $invalid_extensions = ['x_.t', 'x._t', 'xt_', 'x__t', '_xt'];
+    foreach ($invalid_extensions as $value) {
+      $edit = [
+        'settings[file_extensions]' => $value,
+      ];
+      $this->drupalGet("admin/structure/types/manage/article/fields/node.article.$field_name");
+      $this->submitForm($edit, 'Save settings');
+      $this->assertSession()->pageTextContains("The list of allowed extensions is not valid. Allowed characters are a-z, 0-9, '.', and '_'. The first and last characters cannot be '.' or '_', and these two characters cannot appear next to each other. Separate extensions with a comma or space.");
+    }

Let's part this part of \Drupal\Tests\file\Functional\FileFieldWidgetTest::testFileExtensionsSetting() as this is about configuring the file field and it test very similar functionality.

Move the test from +++ b/core/modules/file/tests/src/Functional/FileFieldValidateTest.php to +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php.

Yes, that makes the change requested in #59.

Committed 663c57b and pushed to 9.2.x. Thanks!

BTW, updated CR and published it.