Problem

When i allow whatever extension on file upload, with multiple upload in that location is a bug with validation extension.
Validation of the get-go file work corretly.
For the second file and other $extensions apply default value = 'jpg jpeg gif png txt doctor xls pdf ppt pps odt ods odp';

Solution

For correct operation, we need to move the lawmaking with the $extensions above "foreach ($uploaded_files "...
I'm attaching a patch.

Back up from Acquia helps fund testing for Drupal Acquia logo

Comments

  • Log in or register to post comments

kkonuhov's picture

  • Log in or annals to postal service comments

dawehner's picture

Comment #3

dawehner

Primary language German

Credit Attribution: dawehner every bit a volunteer commented

Condition: Active » Needs review
  • Log in or register to post comments

Anonymous's picture

Comment #4

Anonymous (non verified) Credit Attribution: Bearding commented

  • Log in or register to postal service comments

runeasgar's picture

Comment #5

runeasgar Credit Attribution: runeasgar every bit a volunteer commented

Status: Needs review » Needs work
  1. Reproducing. I have two files created via touch: dummy.txt and dummy.ftw.
  2. Adding a file field to the article content blazon, allowing unlimited uploads and.. hmm. The just extension listed is txt. The default list mentioned in the consequence summary isn't in that location. Oh well, continuing the test equally best I can. Adding "ftw" to the listing of immune extensions.
  3. Adding an commodity and attempting to add together dummy.txt and dummy.ftw, in that order: succeeded.

Unable to reproduce. Delight provide step-by-step instructions for reproducing this result.

  • Log in or register to postal service comments

Anonymous's picture

Annotate #6

Anonymous (not verified) Credit Attribution: Bearding commented

  • Log in or annals to postal service comments

Anonymous's picture

Comment #vii

Anonymous (not verified) Credit Attribution: Anonymous commented

  • Log in or annals to post comments

jlscott's picture

Comment #ix

jlscott Credit Attribution: jlscott equally a volunteer commented

Condition: Needs review » Reviewed & tested by the community

I accept tested and confirmed the patch from #7.

I am using the contrib module allow_all_file_extensions to enable the UI to specify a file field that allows files with any extension to be uploaded. There is no official D8 release yet, but a tarball containing working D8 code is available from https://www.drupal.org/project/allow_all_file_extensions/issues/2974935

My proffer to improve the patch would be to put ALL of the processing of the $validators['file_validate_extensions'] value to a higher place the "foreach file" loop rather than just the portion that checks whether the listing is present but empty. The logic of the block can then exist left unchanged from the original.

Cheers

  • Log in or register to mail comments

mallezie's picture

Test and fix looks good. Already tester this in the other event as well.

One nitpick

                +  // If 'file_validate_extensions' is set and the list is empty and then the caller +  // wants to allow any extension. In this case we have to remove the validator +  // or else it will reject all extensions. +  $is_not_validate_extensions = isset($validators['file_validate_extensions']) && !isset($validators['file_validate_extensions'][0]); +    foreach ($uploaded_files as $i => $file_info

Why not test for empty($validators['file_validate_extensions']) instead of commencement element.

Does what the comment says then, and then a bit more readable imo.

  • Log in or annals to mail comments

alexpott's picture

Comment #12

alexpott

Primary language English

Location 🇪🇺🌍

Credit Attribution: alexpott commented

I remember we should refactor to prevent these type of errors. The problem is the loop. Allow's refactor everything in the loop into a role. This means this type of event is far less likely to occur.

The interdiff attached is non easy to read.

                +++ b/core/modules/system/tests/themes/test_theme_settings/config/schema/test_theme_settings.schema.yml @@ -x,3 +10,9 @@ test_theme_settings.settings:          label: 'fids' +    multi_file: +      type: sequence +      label: 'Multiple file field with all file extensions' +      sequence: +        type: integer +        characterization: 'fids'  +++ b/cadre/modules/system/tests/themes/test_theme_settings/theme-settings.php @@ -24,half-dozen +24,17 @@ function test_theme_settings_form_system_theme_settings_alter(&$course, FormStateI +  $form['multi_file'] = [ +    '#type' => 'managed_file', +    '#title' => t('Multiple file field with all file extensions'), +    '#multiple' => TRUE, +    '#default_value' => theme_get_setting('multi_file'), +    '#upload_location' => 'public://test', +    '#upload_validators'  => [ +      'file_validate_extensions' => [], +    ], +  ];

I'thousand not certain that this is the right place for this exam code. Can we not practise something but in the file module?

  • Log in or register to post comments

alexpott's picture

Comment #13

alexpott

Primary language English language

Location 🇪🇺🌍

Credit Attribution: alexpott commented

Here's a test that is simply office of the file module and its existing test infra.

  • Log in or annals to post comments

jlscott's picture

Annotate #xiv

jlscott Credit Attribution: jlscott as a volunteer commented

@alexpott. I like the refactoring yous have done, I converted your patch to allow it to apply to viii.5.4 and have tested it. I can confirm that it works correctly with my surroundings (D8.5.4 and contrib module "allow_all_file_extensions"). Converted patch fastened. leaving on "Needs review" equally I accept not tested your patch in an 8.6 environs.

  • Log in or register to post comments
  • Log in or annals to mail comments

jlscott's picture

Comment #17

jlscott Credit Attribution: jlscott as a volunteer commented

Patch for D8.5.4 rerolled and marked as do-non-exam.

  • Log in or register to post comments

alexpott's picture

@jlscott thanks for backporting to eight.v.x - the patch in #17 doesn't take the new exam. I'g uploading the 8.6.10 patch as nosotros take to get that done first and information technology helps the committers and reviewers when the about recent patch is the i to review / commit.

@jlscott perhaps yous tin review the patch with a view to rtbc?

  • Log in or annals to mail service comments

alexpott's picture

Crediting @mallezie for work on the indistinguishable issue. Crediting @kkonuhov for creating the issue.

  • Log in or annals to post comments

jlscott's picture

Comment #xx

jlscott Credit Attribution: jlscott as a volunteer commented

Status: Needs review » Reviewed & tested by the community

I can confirm that the patch in #18 works correctly on my system (D8.6.0-dev plus allow_all_file_extensions).

  • Log in or annals to post comments

mallezie's picture

Just took a look at the new patch. I really similar the dissever office, makes things more than readable. Only constitute some very pocket-sized nitpicks on the comments. 

                +++ b/core/modules/file/file.module @@ -887,182 +886,208 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL + *   (optional) A string containing the URI that the file should be copied to.

the Uri where the file should be copied to.

                + *   The created file entity or FALSE if the uploaded file not saved.

If the uploaded file was not saved.

                +  // See http://php.net/manual/features.file-upload.errors.php.

See lowercase

                +  // If file_destination() returns Simulated then $replace === FILE_EXISTS_ERROR and +  // in that location's an existing file so we need to bail.

Not sure this annotate makes much sense.

  • Log in or register to mail service comments

alexpott's picture

@mallezie all of this documentation is copied from the existing documentation is copied apart from The created file entity or FALSE if the uploaded file non saved. and I call up you lot could have was, is or fifty-fifty zip here are it is all still correct :)

  • Log in or register to postal service comments

mallezie's picture

Then only an extra RTBC from my side. Did non want to change status on those nitpicks, and so thank you a lot for explaining them.

  • Log in or register to post comments
Condition: Reviewed & tested by the community » Needs work
  • Log in or register to postal service comments

alexpott's picture

Status: Needs review » Reviewed & tested past the community

The failed test run had zilch to do with this outcome.

  • Log in or register to post comments
Status: Reviewed & tested by the customs » Needs piece of work
  • Log in or register to post comments

mallezie's picture

Status: Needs piece of work » Reviewed & tested by the community
  • Log in or register to post comments

larowlan's picture

Status: Reviewed & tested by the community » Needs work 
  1.                       +++ b/core/modules/file/file.module @@ -887,182 +886,208 @@ office file_save_upload($form_field_name, $validators = [], $destination = FAL + * @param \SplFileInfo $file_info

    nit: this needs a param comment 

  2.                       +++ b/core/modules/file/file.module @@ -887,182 +886,208 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL +part _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $validators = [], $destination = Simulated, $supersede = FILE_EXISTS_RENAME) {

    are we sure we want to add a new procedural role? 

  3.                       +++ b/core/modules/file/tests/src/Functional/MultipleFileUploadTest.php @@ -0,0 +1,59 @@ + * @group system

    should this be file?

  • Log in or annals to mail comments

alexpott's picture

Thanks for the review @larowlan
1. Fixed
2. Yes because it makes these blazon of errors incommunicable to have in this function. It's much easier to understand the responsibilities when _file_save_upload_single() is in a different scope than file_save_upload()
iii. Stock-still.

  • Log in or register to post comments

diqidoq's picture 

                +++ b/core/modules/file/file.module @@ -887,182 +886,208 @@ part file_save_upload() ...  +  $file->setFileUri($file->destination);   +  if (!drupal_move_uploaded_file($file_info->getRealPath(), $file->getFileUri())) {   +    \Drupal::messenger()->addError(t('File upload error. Could not movement uploaded file.'));   +    \Drupal::logger('file')->notice('Upload error. Could not motion uploaded file %file to destination %destination.', ['%file' => $file->getFilename(), '%destination' => $file->getFileUri()]);   +    return False;   +  }

One niggling Nitpicking: When commenting the setting of permission we should comment consistently that nosotros ready FileUri too earlier that, isn't?

Autonomously from that RTBC from me, since simpletest.me examination with patch applied run successfully and multiple files upload test with different file types worked every bit expected without flaws. Additionally tested with experimental module media library: https://dmb66.ply.st/node/1

Do nosotros need a backport for 8.6.x ?

  • Log in or register to post comments

alexpott's picture

@diqidoq this code is not introduced on changed by this patch. This patch moves everything from inside a foreach into it'due south own function. As well the patch applies cleanly to viii.vi.x so it'southward upto the committer equally to whether it should be reddish-picked. There's no need for a separate backport patch.

  • Log in or annals to mail comments

diqidoq's picture

Comment #34

diqidoq Credit Attribution: diqidoq every bit a volunteer and commented

@alexpott: Thanks for taking the time to analyze. Dandy to come across progress on it. Thanks to all who have worked on it. Maybe nosotros become some more than testers for RTBC?

  • Log in or annals to post comments

lpeabody's picture

Status: Needs review » Reviewed & tested by the community

Quite right @diqidoq. Where are my manners. I tested this patch on Friday and it worked beautifully. Attached multiple files, no validation errors!

  • Log in or annals to post comments
Condition: Reviewed & tested by the community » Needs work
  • Log in or annals to postal service comments

alexpott's picture

  • Log in or register to post comments

catch's picture

Version: eight.7.x-dev » 8.6.x-dev
Condition: Reviewed & tested past the community » Fixed
  • Log in or register to mail comments
  • catch committed 2b94ed3 on 8.7.ten 
    Result #2894193 past alexpott, jlscott, kkonuhov, mallezie, larowlan:...
  • Log in or annals to mail service comments
  • grab committed e297dac on eight.6.x 
    Issue #2894193 past alexpott, jlscott, kkonuhov, mallezie, larowlan:...
  • Log in or annals to mail service comments

diqidoq's picture

Comment #41

diqidoq Credit Attribution: diqidoq as a volunteer and commented

1+ @take hold of & @alexpott! Awesome!

  • Log in or register to post comments

Lendude's picture

Annotate #42

Lendude

Primary language Dutch

Location Amsterdam

Credit Attribution: Lendude commented 

                +++ b/cadre/modules/file/tests/src/Functional/MultipleFileUploadTest.php @@ -0,0 +1,threescore @@ +    $assert->pageTextContains('Only files with the following extensions are allowed'); +    $assert->pageTextNotContains('The file ids are ane,2.'); +    $assert->pageTextContains('The specified file file1.wtf could non be uploaded.'); +    $assert->pageTextContains('The specified file file2.wtf could non be uploaded.');

Shame these assertions that @alexpott added in #thirteen got taken out in #14, now nosotros have no positive assertions for the error messages, only a negative assertion for the summit level error.

  • Log in or register to post comments
Status: Stock-still » Closed (fixed)

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

  • Log in or register to post comments

wturrell's picture

Comment #44

wturrell Credit Attribution: wturrell as a volunteer commented

Howdy. I'm comparing this and the patch in #2869855: Race condition in file_save_upload causes data loss - practise the changes to validation here brand the latter unnecessary now? (and if not, might someone exist able to help with re-rolling it?)

What I noticed was the chunk of identical lawmaking in file.module:1039 (building the render assortment for the error message, which is the primary part of the race condition patch).

  • Log in or register to mail comments