Uploading a managed file on a custom form that allows multiple files also triggers the remove button which results in a duplicate temporary record and results in the file being deleted

We define the loss of stored data as critical. There are a handful of issues related to file deletion that I've related to this issue.

Does it really need to be a custom form ? It would explain a couple of issues in the queue already with sudden (sometimes random) deletion of files, very nice find!

Tagging so javascript maintainers will see this.

Can the reporters of this issue also look into #2869855: Race condition in file_save_upload causes data loss, which I suspect focused on the wrong thing to point to as the root cause? I think the root cause of that issue (and their steps to reproduce) is the same as the root cause reported here.

@charlotte.b Thanks for raising the issue. I tried to reproduce the issue with article (updating file field to accept unlimited images), but couldn't reproduce. Could you update steps to reproduce on issue summary please?

Had to reroll as current patch doesn't apply.

Thanks @charlotte.b. Updated issue summary with details and tagged for testing.

Back to needs work to add tests.

When it comes to a reproducible test-case, #2869855: Race condition in file_save_upload causes data loss may help here - it's consistently reproducible by introducing a delay in hook_file_validate().

Nevermind

I tested this using the SimpleForm from the examples module's fapi_example module. Adding a managed_file element along the lines of charlotte.b's example:

  public function buildForm(array $form, FormStateInterface $form_state) {

    $form['title'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Title'),
      '#description' => $this->t('Title must be at least 5 characters in length.'),
      '#required' => TRUE,
    ];

    $form['exam_document'] = [
      '#type' => 'managed_file',
      '#title' => $this->t('Exam Document'),
      '#required' => FALSE,
      '#upload_location' => 'public://',
      '#multiple' => TRUE,
      '#default_value' => '',
      '#upload_validators' => [
        'file_validate_extensions' => ['doc docx txt pdf'],
      ]
    ];

...puts the upload widget into the form at /examples/fapi-example/simple-form

I was then able to reproduce the issue of getting duplicate entries in the file_managed table when uploading a second file to the field.

mysql> SELECT fid, uuid, filename, uri, status, created FROM file_managed WHERE fid > 31;
+-----+--------------------------------------+------------------+---------------------------+--------+------------+
| fid | uuid                                 | filename         | uri                       | status | created    |
+-----+--------------------------------------+------------------+---------------------------+--------+------------+
|  32 | d3713acc-ad6b-430f-af43-42f25674f36a | test_upload.txt  | public://test_upload.txt  |      0 | 1508422451 |
|  33 | c9c25e7a-9c76-48a1-adee-3de19b855357 | another_test.pdf | public://another_test.pdf |      0 | 1508423711 |
|  34 | c539c80b-09c1-48e9-97e6-c4d74d6710d7 | another_test.pdf | public://another_test.pdf |      0 | 1508423711 |
+-----+--------------------------------------+------------------+---------------------------+--------+------------+

I assume you'd need a submit handler etc.. in order for one of the rows to be marked as FILE_STATUS_PERMANENT, but I think having two rows with the same uri (created at the same time) reproduces the problem anyway.

The duplicates were caused by the browser sending two POSTs more-or-less simultaneously:

[19/Oct/2017:15:35:11 +0100] "POST /examples/fapi-example/simple-form?element_parents=exam_document&ajax_form=1&_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax HTTP/1.1" 200 5886
[19/Oct/2017:15:35:11 +0100] "POST /examples/fapi-example/simple-form?element_parents=exam_document&ajax_form=1&_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax HTTP/1.1" 200 5886

Applying the patch from #13 (and rebuilding caches to ensure I was getting the updated JS) resolved the issue; no more duplicate POSTs and rows in the file_managed table when adding multiple files to the upload field.

So this fix looks good to me; agree tests would be good, but perhaps made slightly tricky by the fact that the faulty behaviour is client-side.

I am going to un-mark #2869855: Race condition in file_save_upload causes data loss as a duplicate of this; they are very similar in terms of the symptoms / outcomes, but I think the cause is quite different. I'd argue this issue should remain focused on the JS fix in the file module. I'll update the other issue with some findings around the race condition.

Is this the same issue as https://www.drupal.org/project/drupal/issues/2666746 ?

Nope, confirmed that it's not the same issue; I can still reproduce the problem as described in #23 with the latest changes committed over in #2666746: Fix simultaneous file uploads re-posting data.

I've also confirmed that the (attached, rerolled) patch still resolves the issue of double-uploads. (If anyone else is testing, ensure you rebuild caches so that you actually get the updated JS.)

This still needs tests though.

NR for the testbot

Back to Needs Work for tests to be added.

13:15:09 Build timed out (after 110 minutes). Marking the build as aborted.

So test only patch hangs somehow https://dispatcher.drupalci.org/job/drupal_patches/56961/console

Great! Next time please attach test only patch first to keep issue status NR.

+++ b/core/modules/file/file.js
@@ -92,7 +92,7 @@
+      $(event.target).closest('.js-form-managed-file').find('.managed-file-upload-button').trigger('mousedown');

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -243,7 +243,12 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
+          'managed-file-upload-button',

We could use a data attribute so it would be easier to remember that there's some JS attached to the element.

+++ b/core/modules/file/file.js
@@ -92,7 +92,7 @@
+      $(event.target).closest('.js-form-managed-file').find('.managed-file-upload-button').trigger('mousedown');

Please make your changes to the .es6.js file and then use the transpile process.

@laurii would a js- prefix work?

@drpal updated es6 file and transpiled

Rerolled.

I use on the site. Tested manually.

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -243,7 +243,10 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
+        'data-managed-file-js-upload' => '',

How about using the data-drupal-selector which we've kind of standardized on?

@lauriii Thank you for the suggestion.

We can use data-drupal-selector to distinguish the buttons. Additional 'data-' attributes are not needed.

In the upload button the data-drupal-selector is equal to edit-$form_id-upload-button.
In the remove button the data-drupal-selector is equal to edit-$form_id-remove-button.

+++ b/core/modules/file/file.js
@@ -92,7 +92,7 @@
       }
     },
     triggerUploadButton: function triggerUploadButton(event) {
-      $(event.target).closest('.js-form-managed-file').find('.js-form-submit').trigger('mousedown');
+      $(event.target).closest('.js-form-managed-file').find('.js-form-submit[data-drupal-selector$="upload-button"]').trigger('mousedown');
     },
     disableFields: function disableFields(event) {
       var $clickedButton = $(this);

+++ b/core/modules/file/tests/src/FunctionalJavascript/AjaxFileManagedMultipleTest.php
@@ -0,0 +1,60 @@
+    $file_system = $this->container->get('file_system');
+    $file_storage = $this->container->get('entity_type.manager')

I prefer \Drupal::service() or in the second case \Drupal::entityTypeManager() in tests, see #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests

Thanks. I don't know too much about JS, but this was RTBC before and I think the feedback from @laurii has been incorporated, so back to him it goes :)

Random/time related fail:

-'5 years 11 months'
+'6 years'

+++ b/core/modules/file/tests/src/FunctionalJavascript/AjaxFileManagedMultipleTest.php
@@ -0,0 +1,59 @@
+    // Save entire form.
+    $page->find('css', '[value="' . t('Save') . '"]')->click();

This can be a bit simpler - i.e. $page->pressButton('Save');

In fact looking at the test there's a few ways it can be made a little easier to understand - i.e. not having to work out what $path, $paths and $realpath all are...

  /**
   * Test if managed file form element works well with multiple files upload.
   */
  public function testMultipleFilesUpload() {
    $file_system = \Drupal::service('file_system');
    $file_storage = \Drupal::entityTypeManager()->getStorage('file');
    $page = $this->getSession()->getPage();

    $this->drupalGet(Url::fromRoute('file_module_test.managed_test', ['multiple' => TRUE]));

    $paths = [];
    foreach (array_slice($this->drupalGetTestFiles('image'), 0, 2) as $image) {
      $paths[] = $image->filename;
      $page->attachFileToField('files[nested_file][]', $file_system->realpath($image->uri));
      $this->assertSession()->assertWaitOnAjaxRequest();
    }

    // Save entire form.
    $page->pressButton('Save');

    /** @var \Drupal\file\FileInterface[] $files */
    $files = $file_storage->loadByProperties(['filename' => $paths]);
    $this->assertSession()->responseContains((string) t('The file ids are %fids.', ['%fids' => implode(',', array_keys($files))]));
    $this->assertCount(2, $files, 'Only two files expected to be saved.');
  }

1. Addressed #57.

2. $modules is now a protected property in unit tests
#2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase

--- a/core/modules/file/tests/src/FunctionalJavascript/AjaxFileManagedMultipleTest.php
+++ b/core/modules/file/tests/src/FunctionalJavascript/AjaxFileManagedMultipleTest.php
@@ -18,11 +18,9 @@ class AjaxFileManagedMultipleTest extends WebDriverTestBase {
   }
 
   /**
-   * Modules to enable.
-   *
-   * @var array
+   * {@inheritdoc}
    */
-  public static $modules = ['file_test', 'file', 'file_module_test'];
+  protected static $modules = ['file_test', 'file', 'file_module_test'];

I've confirmed the test fails without the fix. Patch attached makes the test a little bit simpler - less t() and easier to read assertions.

Committed and pushed 982773dea5 to 8.8.x and 20ebfa4348 to 8.7.x. Thanks!

Credited @lauriii and @Berdir for issue review.