- Patch #926016 by effulgentsia, chrisshattuck: several bugs when trying to remove files from a multivalued file/image field.

3 files changed, 151 insertions, 37 deletions

diff --git a/modules/file/file.field.inc b/modules/file/file.field.inc
index faa661ea0..3b75f5bf9 100644
--- a/modules/file/file.field.inc
+++ b/modules/file/file.field.inc
@@ -649,7 +649,8 @@ function file_field_widget_process($element, &$form_state, $form) {
   // file, the entire group of file fields is updated together.
   if ($field['cardinality'] != 1) {
     $new_path = preg_replace('/\/\d+\//', '/', $element['remove_button']['#ajax']['path'], 1);
-    $new_wrapper = preg_replace('/-\d+-/', '-', $element['remove_button']['#ajax']['wrapper'], 1);
+    $field_element = drupal_array_get_nested_value($form, array_slice($element['#array_parents'], 0, -1));
+    $new_wrapper = $field_element['#id'] . '-ajax-wrapper';
     foreach (element_children($element) as $key) {
       if (isset($element[$key]['#ajax'])) {
         $element[$key]['#ajax']['path'] = $new_path;
@@ -659,6 +660,15 @@ function file_field_widget_process($element, &$form_state, $form) {
     unset($element['#prefix'], $element['#suffix']);
   }
 
+  // Add another submit handler to the upload and remove buttons, to implement
+  // functionality needed by the field widget. This submit handler, along with
+  // the rebuild logic in file_field_widget_form() requires the entire field,
+  // not just the individual item, to be valid.
+  foreach (array('upload_button', 'remove_button') as $key) {
+    $element[$key]['#submit'][] = 'file_field_widget_submit';
+    $element[$key]['#limit_validation_errors'] = array(array_slice($element['#parents'], 0, -1));
+  }
+
   return $element;
 }
 
@@ -723,6 +733,25 @@ function _file_field_get_description_from_element($element) {
 }
 
 /**
+ * Submit handler for upload and remove buttons of file_generic fields.
+ *
+ * This runs in addition to and after file_managed_file_submit().
+ *
+ * @see file_managed_file_submit()
+ * @see file_field_widget_form()
+ * @see file_field_widget_process()
+ */
+function file_field_widget_submit($form, &$form_state) {
+  // During the form rebuild, file_field_widget_form() will create field item
+  // widget elements using re-indexed deltas, so clear out $form_state['input']
+  // to avoid a mismatch between old and new deltas. The rebuilt elements will
+  // have #default_value set appropriately for the current state of the field,
+  // so nothing is lost in doing this.
+  list($field_name, $langcode) = $form_state['triggering_element']['#parents'];
+  unset($form_state['input'][$field_name][$langcode]);
+}
+
+/**
  * Returns HTML for an individual file upload widget.
  *
  * @param $variables
diff --git a/modules/file/file.module b/modules/file/file.module
index 2249b0b81..33868bf4f 100644
--- a/modules/file/file.module
+++ b/modules/file/file.module
@@ -376,6 +376,7 @@ function file_managed_file_process($element, &$form_state, $form) {
     '#value' => t('Upload'),
     '#validate' => array(),
     '#submit' => array('file_managed_file_submit'),
+    '#limit_validation_errors' => array($element['#parents']),
     '#ajax' => $ajax_settings,
     '#weight' => -5,
   );
@@ -389,16 +390,11 @@ function file_managed_file_process($element, &$form_state, $form) {
     '#value' => t('Remove'),
     '#validate' => array(),
     '#submit' => array('file_managed_file_submit'),
+    '#limit_validation_errors' => array($element['#parents']),
     '#ajax' => $ajax_settings,
     '#weight' => -5,
   );
 
-  // Limit validation errors to the file field only. The entire field is needed later
-  // by file_field_widget_form(), so the last element is sliced off the #parents array
-  // to avoid removing too much from $form_state['values'].
-  $element['upload_button']['#limit_validation_errors'] = array(array_slice($element['#parents'], 0, -1));
-  $element['remove_button']['#limit_validation_errors'] = array(array_slice($element['#parents'], 0, -1));
-
   $element['fid'] = array(
     '#type' => 'hidden',
     '#value' => $fid,
diff --git a/modules/file/tests/file.test b/modules/file/tests/file.test
index ff20a3abc..22c6ca4f8 100644
--- a/modules/file/tests/file.test
+++ b/modules/file/tests/file.test
@@ -203,29 +203,21 @@ class FileFieldTestCase extends DrupalWebTestCase {
 
 
 /**
- * Test class to test file field upload and remove buttons, with and without AJAX.
+ * Test class to test file field widget, single and multi-valued, with and without AJAX, with public and private files.
  */
 class FileFieldWidgetTestCase extends FileFieldTestCase {
   public static function getInfo() {
     return array(
       'name' => 'File field widget test',
-      'description' => 'Test upload and remove buttons, with and without AJAX.',
+      'description' => 'Tests the file field widget, single and multi-valued, with and without AJAX, with public and private files.',
       'group' => 'File',
     );
   }
 
   /**
-   * Tests upload and remove buttons, with and without AJAX.
-   *
-   * @todo This function currently only tests the "remove" button of a single-
-   *   valued field. Tests should be added for the "upload" button and for each
-   *   button of a multi-valued field. Tests involving multiple AJAX steps on
-   *   the same page will become easier after http://drupal.org/node/789186
-   *   lands. Testing the "upload" button in AJAX context requires more
-   *   investigation into how jQuery uploads files, so that drupalPostAJAX() can
-   *   emulate that correctly.
+   * Tests upload and remove buttons, with and without AJAX, for a single-valued File field.
    */
-  function testWidget() {
+  function testSingleValuedWidget() {
     // Use 'page' instead of 'article', so that the 'article' image field does
     // not conflict with this test. If in the future the 'page' type gets its
     // own default file or image field, this test can be made more robust by
@@ -241,11 +233,14 @@ class FileFieldWidgetTestCase extends FileFieldTestCase {
     foreach (array('nojs', 'js') as $type) {
       // Create a new node with the uploaded file and ensure it got uploaded
       // successfully.
+      // @todo This only tests a 'nojs' submission, because drupalPostAJAX()
+      //   does not yet support file uploads.
       $nid = $this->uploadNodeFile($test_file, $field_name, $type_name);
       $node = node_load($nid, NULL, TRUE);
       $node_file = (object) $node->{$field_name}[LANGUAGE_NONE][0];
       $this->assertFileExists($node_file, t('New file saved to disk on node creation.'));
-      // Test file download.
+
+      // Ensure the file can be downloaded.
       $this->drupalGet(file_create_url($node_file->uri));
       $this->assertResponse(200, t('Confirmed that the generated URL is correct by downloading the shipped file.'));
 
@@ -260,13 +255,8 @@ class FileFieldWidgetTestCase extends FileFieldTestCase {
           $this->drupalPost(NULL, array(), t('Remove'));
           break;
         case 'js':
-          // @todo This can be simplified after http://drupal.org/node/789186
-          //   lands.
-          preg_match('/jQuery\.extend\(Drupal\.settings, (.*?)\);/', $this->content, $matches);
-          $settings = drupal_json_decode($matches[1]);
           $button = $this->xpath('//input[@type="submit" and @value="' . t('Remove') . '"]');
-          $button_id = (string) $button[0]['id'];
-          $this->drupalPostAJAX(NULL, array(), array((string) $button[0]['name'] => (string) $button[0]['value']), $settings['ajax'][$button_id]['url'], array(), array(), NULL, $settings['ajax'][$button_id]);
+          $this->drupalPostAJAX(NULL, array(), array((string) $button[0]['name'] => (string) $button[0]['value']));
           break;
       }
 
@@ -279,34 +269,133 @@ class FileFieldWidgetTestCase extends FileFieldTestCase {
       $node = node_load($nid, NULL, TRUE);
       $this->assertTrue(empty($node->{$field_name}[LANGUAGE_NONE][0]['fid']), t('File was successfully removed from the node.'));
     }
+  }
 
-    // Test partial form submissions using the Upload button on a multivalue field.
-    field_delete_field($field_name);
+  /**
+   * Tests upload and remove buttons, with and without AJAX, for a multi-valued File field.
+   */
+  function testMultiValuedWidget() {
+    // Use 'page' instead of 'article', so that the 'article' image field does
+    // not conflict with this test. If in the future the 'page' type gets its
+    // own default file or image field, this test can be made more robust by
+    // using a custom node type.
+    $type_name = 'page';
+    $field_name = strtolower($this->randomName());
     $this->createFileField($field_name, $type_name, array('cardinality' => 3));
+    $field = field_info_field($field_name);
+    $instance = field_info_instance('node', $field_name, $type_name);
+
+    $test_file = $this->getTestFile('text');
+
+    foreach (array('nojs', 'js') as $type) {
+      // Visit the node creation form, and upload 3 files. Since the field has
+      // cardinality of 3, ensure the "Upload" button is displayed until after
+      // the 3rd file, and after that, isn't displayed.
+      // @todo This is only testing a non-AJAX upload, because drupalPostAJAX()
+      //   does not yet emulate jQuery's file upload.
+      $this->drupalGet("node/add/$type_name");
+      for ($delta = 0; $delta < 3; $delta++) {
+        $edit = array('files[' . $field_name . '_' . LANGUAGE_NONE . '_' . $delta . ']' => drupal_realpath($test_file->uri));
+        // If the Upload button doesn't exist, drupalPost() will automatically
+        // fail with an assertion message.
+        $this->drupalPost(NULL, $edit, t('Upload'));
+      }
+      $this->assertNoFieldByXpath('//input[@type="submit"]', t('Upload'), t('After uploading 3 files, the "Upload" button is no longer displayed.'));
+
+      // Test clicking each "Remove" button. For extra robustness, test them out
+      // of sequential order. They are 0-indexed, and get renumbered after each
+      // iteration, so array(1, 1, 0) means:
+      // - First remove the 2nd file.
+      // - Then remove what is then the 2nd file (was originally the 3rd file).
+      // - Then remove the first file.
+      $num_expected_remove_buttons = 3;
+      foreach (array(1, 1, 0) as $delta) {
+        // Ensure we have the expected number of Remove buttons, and that they
+        // are numbered sequentially.
+        $buttons = $this->xpath('//input[@type="submit" and @value="Remove"]');
+        $this->assertTrue(is_array($buttons) && count($buttons) === $num_expected_remove_buttons, t('There are %n "Remove" buttons displayed (JSMode=%type).', array('%n' => $num_expected_remove_buttons, '%type' => $type)));
+        foreach ($buttons as $i => $button) {
+          $this->assertIdentical((string) $button['name'], $field_name . '_' . LANGUAGE_NONE . '_' . $i . '_remove_button');
+        }
+
+        // "Click" the remove button (emulating either a nojs or js submission).
+        $button_name = $field_name . '_' . LANGUAGE_NONE . '_' . $delta . '_remove_button';
+        switch ($type) {
+          case 'nojs':
+            // drupalPost() takes a $submit parameter that is the value of the
+            // button whose click we want to emulate. Since we have multiple
+            // buttons with the value "Remove", and want to control which one we
+            // use, we change the value of the other ones to something else.
+            // Since non-clicked buttons aren't included in the submitted POST
+            // data, and since drupalPost() will result in $this being updated
+            // with a newly rebuilt form, this doesn't cause problems.
+            foreach ($buttons as $button) {
+              if ($button['name'] != $button_name) {
+                $button['value'] = 'DUMMY';
+              }
+            }
+            $this->drupalPost(NULL, array(), t('Remove'));
+            break;
+          case 'js':
+            // drupalPostAJAX() lets us target the button precisely, so we don't
+            // require the workaround used above for nojs.
+            $this->drupalPostAJAX(NULL, array(), array($button_name => t('Remove')));
+            break;
+        }
+        $num_expected_remove_buttons--;
+
+        // Ensure we have a single Upload button, and that it is numbered
+        // sequentially after the Remove buttons.
+        $buttons = $this->xpath('//input[@type="submit" and @value="Upload"]');
+        $this->assertTrue(is_array($buttons) && count($buttons) == 1 && ((string) $buttons[0]['name'] === ($field_name . '_' . LANGUAGE_NONE . '_' . $num_expected_remove_buttons . '_upload_button')), t('After removing a file, an "Upload" button is displayed (JSMode=%type).'));
+      }
 
-    $this->drupalGet("node/add/$type_name");
-    for ($delta = 0; $delta < 3; $delta++) {
-      $edit = array('files[' . $field_name . '_' . LANGUAGE_NONE . '_' . $delta . ']' => drupal_realpath($test_file->uri));
-      $this->drupalPost(NULL, $edit, t('Upload'));
+      // Ensure the page now has no Remove buttons.
+      $this->assertNoFieldByXPath('//input[@type="submit"]', t('Remove'), t('After removing all files, there is no "Remove" button displayed.', array('%n' => $num_expected_remove_buttons, '%type' => $type)));
+
+      // Save the node and ensure it does not have any files.
+      $this->drupalPost(NULL, array('title' => $this->randomName()), t('Save'));
+      $matches = array();
+      preg_match('/node\/([0-9]+)/', $this->getUrl(), $matches);
+      $nid = $matches[1];
+      $node = node_load($nid, NULL, TRUE);
+      $this->assertTrue(empty($node->{$field_name}[LANGUAGE_NONE][0]['fid']), t('Node was successfully saved without any files.'));
     }
-    $this->assertNoFieldByXpath('//input[@type="submit"]', t('Upload'), t('After uploading 3 files, the "Upload" button is no longer displayed.'));
+  }
+
+  /**
+   * Tests a file field with a "Private files" upload destination setting.
+   */
+  function testPrivateFileSetting() {
+    // Use 'page' instead of 'article', so that the 'article' image field does
+    // not conflict with this test. If in the future the 'page' type gets its
+    // own default file or image field, this test can be made more robust by
+    // using a custom node type.
+    $type_name = 'page';
+    $field_name = strtolower($this->randomName());
+    $this->createFileField($field_name, $type_name);
+    $field = field_info_field($field_name);
+    $instance = field_info_instance('node', $field_name, $type_name);
 
-    // Test private download method.
+    $test_file = $this->getTestFile('text');
+
+    // Change the field setting to make its files private, and upload a file.
     $edit = array('field[settings][uri_scheme]' => 'private');
     $this->drupalPost("admin/structure/types/manage/$type_name/fields/$field_name", $edit, t('Save settings'));
-    // Create a new node with the uploaded file and ensure it got uploaded
-    // successfully.
     $nid = $this->uploadNodeFile($test_file, $field_name, $type_name);
     $node = node_load($nid, NULL, TRUE);
     $node_file = (object) $node->{$field_name}[LANGUAGE_NONE][0];
     $this->assertFileExists($node_file, t('New file saved to disk on node creation.'));
-    // Test file download.
+
+    // Ensure the private file is available to the user who uploaded it.
     $this->drupalGet(file_create_url($node_file->uri));
     $this->assertResponse(200, t('Confirmed that the generated URL is correct by downloading the shipped file.'));
+
     // Ensure we can't change 'uri_scheme' field settings while there are some
     // entities with uploaded files.
     $this->drupalGet("admin/structure/types/manage/$type_name/fields/$field_name");
     $this->assertFieldByXpath('//input[@id="edit-field-settings-uri-scheme-public" and @disabled="disabled"]', 'public', t('Upload destination setting disabled.'));
+
     // Delete node and confirm that setting could be changed.
     node_delete($nid);
     $this->drupalGet("admin/structure/types/manage/$type_name/fields/$field_name");