Allowed file extensions Widget Input/Display Length Problem

I think this is probably related, I just ran into this issue myself. Once it's done for Drupal 8, it will hopefully be back-ported for Drupal 7.

Duplicate of #997900: Not possible to allow uploading files with any file extension

@dale42 -- Thanks for the clarification, that makes sense now, indeed they do sound like different issues.

Encountered the maxlength problem when trying to...well...add more than 128 characters. I don't see why this limitation would exist. Here's a patch to increase it to 255.

This patch works (#5)

patch in #5 works for me.

This same bug afflicts the file_entity module. I can confirm that this same fix works in both instances with no adverse effects. Marking RTBC.

Changing version, since this isn't fixed yet.

As far as I can see, this affects Drupal 8 too, so would need to be fixed there first.

Here's a forward port of #5 to D8.

Here's another patch that:

• is a forward port of #5 to D8 (raises the limit to 255);
• removes the commas the form builder adds to the stored settings;
• removes any and all signs of commas related to this setting: from the setting's default value, the description, the validation and the error message.

I don't know if a test is necessary; probably not, as there were no tests for the 128-character limit either. That limit of 128 characters is implicitly added by Form API: a textfield's default #maxlength is 128. OTOH, the 255-character limit is just a bit higher: it could be even 1024 or 65535 or 2,147,483,648 or just about any size a config entity setting can hold.

That limit of 128 characters is implicitly added by Form API: a textfield's default #maxlength is 128. OTOH, the 255-character limit is just a bit higher: it could be even 1024 or 65535 or 2,147,483,648 or just about any size a config entity setting can hold.

Seems if we're going to bother changing it, it should go up to at least 1024.

A test here, particularly one for the commas the form builder adds, that shows the current failure and illustrates the fix would be valuable I think.

Also, the current patch no longer applies.

Patch Rerolled!

Reviewing...

Patch #16 does not work. `form_error` is an undefined function in D8.

Also, when I add a new extenstion and save and come back to the form, my extension is missing.

I'm working on a patch.

Patch applies, removing reroll.

This patch builds on top of work done already. The problem with patches #13 and #16 is that they use D7's `form_error` function, which is not defined in D8. It appears that simple test is passing for those patches because the testing does not flow through that code. So, I've written additional simple tests.

Changed the assert in #20 to a better one.

Thanks for adding a test! Can you upload a separate patch file that only includes the test so it can illustrate the current failure?

This patch is only to demonstrate the current failure in the "allowed extensions" field.

@jhedstrom Do you want me to re-roll #21 with this modification (hitting "save" twice) for regression purposes?

@msound sure, but also perhaps update the code comment as to why the double save is needed?

It might be me, but why do we make it so hard for people to enter file extensions? Does it really matter how people enter the extensions?
It's quite easy to filter out comma's, spaces and semicolons.

Can't we simply remove the description and explode on several characters, something like:

<?php
$extensions = preg_split( "/ (,|;| |) /", str_replace('.', '', $input));
?>

Then we sure need to fix the issue with length of the textfield. Let's increase it to 255 and set a #max_length to 200?

Allowing different separators makes sense to me.

Then we sure need to fix the issue with length of the textfield. Let's increase it to 255 and set a #max_length to 200?

The length is already at 256--this patch bumps to 1024.

This is a re-roll of #23 with clear comments. This patch should FAIL thereby proving the bug exists in core.

I am working on re-rolling patch #21 to include the above simpletest in patch #30.

Patch attached. This patch ensures that only space is used as the delimiter. This way user can make full use of the allowed 1024 characters.

This patch also cleans user input, so even if they entered periods or commas, the input will be cleaned up and accepted.

I'm working on this at DrupalCon LA.

I have tested patch #32 and it works for me.

+1

I'm at DrupalCon and reviewing this patch

I'm not at DrupalCon LA (oh wait, it's over), but had a look at the patch in #32 and it looks good. I'm not marking as RTBC because I haven't tested it.

I looked over the code. I also compared it to the site without the patch. This is a simper way forward:

I also confirmed I could us a much longer string - "txt doc wpd rdf drd3 fed3 def2 pwd pwd3 d4 de df dag ded ed dad rad nad tad jss ddd rd 3n dl 88 non none rlan ln ee re nle dea e0 dn0 no dr3 nnn ll rr nn fish h dee dg dse drd lfl wed mom ewl eee fff ggg sss wfw qqw qeqw wwq ww bb lll ere3 vdd3 se si9 le9 bad cdad sad lal lad dal ni l nil u8 ede rkrk rk kr kkr rkk" with the patch than without.

I did notice that there was no error if you went above the character length without the patch (and assume there is none with the patch either). That isn't a blockage for this issue, just something I'm noticing.

Added the string via the content types admin/structure/types/manage/article/fields/node.article.field_file

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -162,16 +162,14 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   -    // Make the extension list a little more human-friendly by comma-separation.
   

   It is possible to enter field data which can be saved once, but not can be saved if the field is edited at a later time.

   This is a nice bug find. And yep the fix of being less human-friendly in one way but way more human friendly in other makes sense to me :)

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -162,16 +162,14 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   -      '#description' => t('Separate extensions with a space or comma and do not include the leading dot.'),
   ...
   +      '#description' => t('Separate extensions with a space and do not include the leading dot.'),
   
   @@ -227,11 +225,11 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +      $extensions = preg_replace('/([\., ]+)/', ' ', trim(strtolower($element['#value'])));
   ...
   -        $form_state->setError($element, t('The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space.'));
   +        $form_state->setError($element, t('The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a space.'));
   

   why are we replacing commas here now they are not used? If we do want to do this it should be tested. I think it might make since to do this but... then the description should probably not be changed.

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -162,16 +162,14 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   -      '#maxlength' => 256,
   +      '#maxlength' => 1024,
   

   Why have a max length? Afaik the config storage does not have a limit.

4. +++ b/core/modules/file/src/Tests/FileFieldValidateTest.php
   @@ -158,4 +158,50 @@ function testFileExtension() {
   +    // Test extensions with periods and commas. The field should be forgiving
   +    // of the user input and clean the input and accept it.
   +    $edit = array();
   +    $extensions = array();
   +    while (count($extensions) < 5) {
   +      $rnd_ext = $this->randomMachineName(3);
   +      if (!in_array($rnd_ext, $extensions)) {
   +        $extensions[] = '.' . $rnd_ext;
   +      }
   +    }
   +    $edit['settings[file_extensions]'] = implode(', ', $extensions);
   +    $this->drupalPostForm('admin/structure/types/manage/article/fields/node.article.' . $field_name, $edit, t('Save settings'));
   +    $this->assertText("Saved $field_name configuration", 'File field saved successfully even when user input contained periods and commas.');
   

   ONly has periods now - the field no longer ignores commas

Removing sprint weekend tag!!
As suggested by @YesCT

Sorry, these issues were actually worked on during the 2015 Global Sprint
Weekend https://groups.drupal.org/node/447258

Updated patch as per comments in #38.

Why have a max length? Afaik the config storage does not have a limit.

Removing the max length resulted in failing tests. So, had to keep it.

Needed a reroll, so started with that. But the logic of allowed extensions has become a bit more complex since this was last rolled due to things like #3166044: Allow file extensions containing underscores

I'm not sure if we should add the bit where we are lenient about the starting dot here. If we want that, we should probably add it in a separate issue since it is something of a feature vs the bug we are trying to fix here.

So for now I've taken that out and just removed the artificial padding with commas on display and added the test coverage. Sorry no interdiff because of the reroll.

Fix the other test.

Needs a different patch for 9.4/9.5 and not going to bother with that until this lands, so moving to D10

Manually tested successfully, including adding commas between extensions when configuring via the form.

Agree with #55 that we should handle the leading dot in a follow-up.

Sorry, I think my comment in #bugsmash was mis-understood.

+++ b/core/modules/file/tests/src/Functional/FileFieldValidateTest.php
@@ -162,6 +162,31 @@ public function testFileExtension() {
+      $rnd_ext = $this->randomMachineName(3);

The first time I ran the test a string of 'php' was created which will cause the test to fail. I think the final array needs to be stripped of any extensions that will cause a failure.