From 94571bdbf79f43769aff2d11d47a4802dbaacd23 Mon Sep 17 00:00:00 2001 From: Sascha Grossenbacher Date: Sun, 10 Apr 2011 19:50:54 +0200 Subject: [PATCH] Issue #1059268 by Berdir: Fixed files are lost when adding multiple files to multiple file fields at the same time. --- modules/file/file.field.inc | 67 +++++++++++--------- modules/file/tests/file.test | 139 +++++++++++++++++++++++++----------------- 2 files changed, 122 insertions(+), 84 deletions(-) diff --git a/modules/file/file.field.inc b/modules/file/file.field.inc index 2af3cb6..1b21858 100644 --- a/modules/file/file.field.inc +++ b/modules/file/file.field.inc @@ -447,32 +447,12 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco 'description' => '', ); - // Retrieve any values set in $form_state, as will be the case during Ajax - // rebuilds of this form. - if (isset($form_state['values'])) { - $path = array_merge($element['#field_parents'], array($field['field_name'], $langcode)); - $path_exists = FALSE; - $values = drupal_array_get_nested_value($form_state['values'], $path, $path_exists); - if ($path_exists) { - $items = $values; - drupal_array_set_nested_value($form_state['values'], $path, NULL); - } - } - - foreach ($items as $delta => $item) { - $items[$delta] = array_merge($defaults, $items[$delta]); - // Remove any items from being displayed that are not needed. - if ($items[$delta]['fid'] == 0) { - unset($items[$delta]); - } + $field_state = field_form_get_state($element['#field_parents'], $field['field_name'], $langcode, $form_state); + if (isset($field_state['items_count']) && $field_state['items_count'] === 0) { + // Assume that field got deleted, force $items to an empty array. + $items = array(); } - // Re-index deltas after removing empty items. - $items = array_values($items); - - // Update order according to weight. - $items = _field_sort_items($field, $items); - // Essentially we use the managed_file type, extended with some enhancements. $element_info = element_info('managed_file'); $element += array( @@ -494,16 +474,18 @@ function file_field_widget_form(&$form, &$form_state, $field, $instance, $langco $elements = array($element); } else { + + // Determine the number of widgets to display. + $max = $field_state['items_count']; + // If there are multiple values, add an element for each existing one. - $delta = -1; - foreach ($items as $delta => $item) { + for ($delta = 0; $delta < $max; $delta++) { $elements[$delta] = $element; - $elements[$delta]['#default_value'] = $item; + $elements[$delta]['#default_value'] = isset($items[$delta]) ? $items[$delta] : $defaults; $elements[$delta]['#weight'] = $delta; } // And then add one more empty row for new uploads. - $delta++; - if ($field['cardinality'] == FIELD_CARDINALITY_UNLIMITED || $delta < $field['cardinality']) { + if (($field['cardinality'] == FIELD_CARDINALITY_UNLIMITED || $delta < $field['cardinality']) && empty($form_state['programmed'])) { $elements[$delta] = $element; $elements[$delta]['#default_value'] = $defaults; $elements[$delta]['#weight'] = $delta; @@ -757,6 +739,33 @@ function file_field_widget_submit($form, &$form_state) { // so nothing is lost in doing this. $parents = array_slice($form_state['triggering_element']['#parents'], 0, -2); drupal_array_set_nested_value($form_state['input'], $parents, NULL); + + $button = $form_state['triggering_element']; + + // Go one level up in the form, to the widgets container. + $element = drupal_array_get_nested_value($form, array_slice($button['#array_parents'], 0, -1)); + $field_name = $element['#field_name']; + $langcode = $element['#language']; + $parents = $element['#field_parents']; + + $submitted_values = drupal_array_get_nested_value($form_state['values'], array_slice($button['#array_parents'], 0, -2)); + foreach ($submitted_values as $delta => $submitted_value) { + if (!$submitted_value['fid']) { + unset($submitted_values[$delta]); + } + } + $count = count($submitted_values); + + // Re-index deltas after removing empty items. + $submitted_values = array_values($submitted_values); + + // Update form_state values. + drupal_array_set_nested_value($form_state['values'], array_slice($button['#array_parents'], 0, -2), $submitted_values); + + // Update items count. + $field_state = field_form_get_state($parents, $field_name, $langcode, $form_state); + $field_state['items_count'] = $count; + field_form_set_state($parents, $field_name, $langcode, $form_state, $field_state); } /** diff --git a/modules/file/tests/file.test b/modules/file/tests/file.test index ea8c5c6..fadb982 100644 --- a/modules/file/tests/file.test +++ b/modules/file/tests/file.test @@ -370,7 +370,7 @@ class FileFieldWidgetTestCase extends FileFieldTestCase { } /** - * Tests upload and remove buttons, with and without Ajax, for a multi-valued File field. + * Tests upload and remove buttons, with and without Ajax, for multiple multi-valued File field. */ function testMultiValuedWidget() { // Use 'page' instead of 'article', so that the 'article' image field does @@ -379,77 +379,106 @@ class FileFieldWidgetTestCase extends FileFieldTestCase { // using a custom node type. $type_name = 'page'; $field_name = strtolower($this->randomName()); + $field_name2 = strtolower($this->randomName()); $this->createFileField($field_name, $type_name, array('cardinality' => 3)); + $this->createFileField($field_name2, $type_name, array('cardinality' => 3)); + $field = field_info_field($field_name); $instance = field_info_instance('node', $field_name, $type_name); + $field2 = field_info_field($field_name2); + $instance2 = field_info_instance('node', $field_name2, $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. + // Visit the node creation form, and upload 3 files for each field. Since + // the field has cardinality of 3, ensure the "Upload" button is displayed + // until after the 3rd file, and after that, isn't displayed. Because + // SimpleTest triggers the last button with a given name, so upload to the + // second field first. // @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'); + foreach (array($field_name2, $field_name) as $each_field_name) { + for ($delta = 0; $delta < 3; $delta++) { + $edit = array('files[' . $each_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 for each field, the "Upload" button is no longer displayed.')); + + $num_expected_remove_buttons = 6; + + foreach (array($field_name, $field_name2) as $current_field_name) { + // How many uploaded files for the current field are remaining. + $remaining = 3; + // 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. + 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) { + $key = $i >= $remaining ? $i - $remaining : $i; + $check_field_name = $field_name2; + if ($current_field_name == $field_name && $i < $remaining) { + $check_field_name = $field_name; + } - // "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->assertIdentical((string) $button['name'], $check_field_name . '_' . LANGUAGE_NONE . '_' . $key. '_remove_button'); + } + + // "Click" the remove button (emulating either a nojs or js submission). + $button_name = $current_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; + $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--; + $remaining--; + + // Ensure an "Upload" button for the current field is displayed with the + // correct name. + $upload_button_name = $current_field_name . '_' . LANGUAGE_NONE . '_' . $remaining . '_upload_button'; + $buttons = $this->xpath('//input[@type="submit" and @value="Upload" and @name=:name]', array(':name' => $upload_button_name)); + $this->assertTrue(is_array($buttons) && count($buttons) == 1, t('The upload button is displayed with the correct name (JSMode=%type).', array('%type' => $type))); + + // Ensure only at most one button per field is displayed. + $buttons = $this->xpath('//input[@type="submit" and @value="Upload"]'); + $expected = $current_field_name == $field_name ? 1 : 2; + $this->assertTrue(is_array($buttons) && count($buttons) == $expected, t('After removing a file, only one "Upload" button for each possible field is displayed (JSMode=%type).', array('%type' => $type))); } - $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).')); } // 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))); + $this->assertNoFieldByXPath('//input[@type="submit"]', t('Remove'), t('After removing all files, there is no "Remove" button displayed (JSMode=%type).', array('%type' => $type))); // Save the node and ensure it does not have any files. $this->drupalPost(NULL, array('title' => $this->randomName()), t('Save')); -- 1.7.4.1