- Patch #552436 by yched: validation of the number of values in field_default_validate().

10 files changed, 197 insertions, 95 deletions

diff --git a/modules/field/field.api.php b/modules/field/field.api.php
index af76ecf6c..653641a6d 100644
--- a/modules/field/field.api.php
+++ b/modules/field/field.api.php
@@ -347,10 +347,6 @@ function hook_field_sanitize($obj_type, $object, $field, $instance, $langcode, &
  *   The type of $object.
  * @param $object
  *   The object for the operation.
- *   Note that this might not be a full-fledged 'object'. When invoked through
- *   field_attach_query(), the $object will only include properties that the
- *   Field API knows about: bundle, id, revision id, and field values (no node
- *   title, user name...).
  * @param $field
  *   The field structure for the operation.
  * @param $instance
@@ -927,11 +923,6 @@ function hook_field_attach_form($obj_type, $object, &$form, &$form_state, $langc
  * indexed by object id. For performance reasons, information for all available
  * objects should be loaded in a single query where possible.
  *
- * Note that $objects might not be full-fledged 'objects'. When invoked through
- * field_attach_query(), each object only includes properties that the Field
- * API knows about: bundle, id, revision id, and field values (no node title,
- * user name...)
-
  * The changes made to the objects' field values get cached by the field cache
  * for subsequent loads.
  *
diff --git a/modules/field/field.attach.inc b/modules/field/field.attach.inc
index 1b55180cd..e0c745976 100644
--- a/modules/field/field.attach.inc
+++ b/modules/field/field.attach.inc
@@ -685,11 +685,16 @@ function field_attach_load_revision($obj_type, $objects, $options = array()) {
  *   The type of $object; e.g. 'node' or 'user'.
  * @param $object
  *   The object with fields to validate.
- * @return
- *   Throws a FieldValidationException if validation errors are found.
+ * @throws FieldValidationException
+ *   If validation errors are found, a FieldValidationException is thrown. The
+ *   'errors' property contains the array of errors, keyed by field name,
+ *   language and delta.
  */
 function field_attach_validate($obj_type, $object) {
   $errors = array();
+  // Check generic, field-type-agnostic errors first.
+  _field_invoke_default('validate', $obj_type, $object, $errors);
+  // Check field-type specific errors.
   _field_invoke('validate', $obj_type, $object, $errors);
 
   // Let other modules validate the object.
diff --git a/modules/field/field.default.inc b/modules/field/field.default.inc
index 7ab203d14..7305b2db9 100644
--- a/modules/field/field.default.inc
+++ b/modules/field/field.default.inc
@@ -21,6 +21,49 @@ function field_default_extract_form_values($obj_type, $object, $field, $instance
   }
 }
 
+/**
+ * Generic field validation handler.
+ *
+ * Possible error codes:
+ * - 'field_cardinality': The number of values exceeds the field cardinality.
+ *
+ * @see _hook_field_validate()
+ *
+ * @param $obj_type
+ *   The type of $object.
+ * @param $object
+ *   The object for the operation.
+ * @param $field
+ *   The field structure for the operation.
+ * @param $instance
+ *   The instance structure for $field on $object's bundle.
+ * @param $langcode
+ *   The language associated to $items.
+ * @param $items
+ *   $object->{$field['field_name']}[$langcode], or an empty array if unset.
+ * @param $errors
+ *   The array of errors, keyed by field name and by value delta, that have
+ *   already been reported for the object. The function should add its errors
+ *   to this array. Each error is an associative array, with the following
+ *   keys and values:
+ *   - 'error': an error code (should be a string, prefixed with the module name)
+ *   - 'message': the human readable message to be displayed.
+ */
+function field_default_validate($obj_type, $object, $field, $instance, $langcode, $items, &$errors) {
+  // Filter out empty values.
+  $items = _field_filter_items($field, $items);
+
+  // Check that the number of values doesn't exceed the field cardinality.
+  // For form submitted values, this can only happen with 'multiple value'
+  // widgets.
+  if ($field['cardinality'] != FIELD_CARDINALITY_UNLIMITED && count($items) > $field['cardinality']) {
+    $errors[$field['field_name']][$langcode][0][] = array(
+      'error' => 'field_cardinality',
+      'message' => t('%name: this field cannot hold more than @count values.', array('%name' => t($instance['label']), '@count' => $field['cardinality'])),
+    );
+  }
+}
+
 function field_default_submit($obj_type, $object, $field, $instance, $langcode, &$items, $form, &$form_state) {
   $field_name = $field['field_name'];
 
@@ -30,7 +73,7 @@ function field_default_submit($obj_type, $object, $field, $instance, $langcode,
   }
 
   // Filter out empty values.
-  $items = field_set_empty($field, $items);
+  $items = _field_filter_items($field, $items);
 }
 
 /**
diff --git a/modules/field/field.form.inc b/modules/field/field.form.inc
index f450dcae4..287604119 100644
--- a/modules/field/field.form.inc
+++ b/modules/field/field.form.inc
@@ -141,7 +141,7 @@ function field_multiple_value_form($field, $instance, $langcode, $items, &$form,
   // Determine the number of widgets to display.
   switch ($field['cardinality']) {
     case FIELD_CARDINALITY_UNLIMITED:
-      $filled_items = field_set_empty($field, $items);
+      $filled_items = _field_filter_items($field, $items);
       $current_item_count = isset($form_state['field_item_count'][$field_name])
                             ? $form_state['field_item_count'][$field_name]
                             : count($items);
diff --git a/modules/field/field.module b/modules/field/field.module
index c87cb0363..d89b76fa3 100644
--- a/modules/field/field.module
+++ b/modules/field/field.module
@@ -303,23 +303,22 @@ function field_get_default_value($obj_type, $object, $field, $instance, $langcod
 }
 
 /**
- * Helper function to filter out empty values.
+ * Helper function to filter out empty field values.
  *
- * On order to keep marker rows in the database, the function ensures
- * that the right number of 'all columns NULL' values is kept.
- *
- * @param array $field
- * @param array $items
- * @return array
- *   returns filtered and adjusted item array
+ * @param $field
+ *   The field definition.
+ * @param $items
+ *   The field values to filter.
  *
- * TODO D7: poorly named...
+ * @return
+ *   The array of items without empty field values. The function also renumbers
+ *   the array keys to ensure sequential deltas.
  */
-function field_set_empty($field, $items) {
+function _field_filter_items($field, $items) {
   $function = $field['module'] . '_field_is_empty';
-  // We ensure the function is loaded, but explicitly break if it is missing.
   function_exists($function);
   foreach ((array) $items as $delta => $item) {
+    // Explicitly break if the function is undefined.
     if ($function($item, $field)) {
       unset($items[$delta]);
     }
diff --git a/modules/field/modules/options/options.module b/modules/field/modules/options/options.module
index f3d945014..42f881088 100644
--- a/modules/field/modules/options/options.module
+++ b/modules/field/modules/options/options.module
@@ -128,20 +128,10 @@ function options_field_widget(&$form, &$form_state, $field, $instance, $langcode
  * Form element validation handler for options element.
  */
 function options_field_widget_validate($element, &$form_state) {
-  $field = $form_state['complete form']['#fields'][$element['#field_name']]['field'];
-  $instance = $form_state['complete form']['#fields'][$element['#field_name']]['instance'];
-
   // Transpose selections from field => delta to delta => field, turning
   // multiple selected options into multiple parent elements.
   $items = _options_form_to_storage($element);
   form_set_value($element, $items, $form_state);
-
-  // Check that we don't exceed the allowed number of values.
-  if ($field['cardinality'] >= 2 && $field['cardinality'] != FIELD_CARDINALITY_UNLIMITED) {
-    if (count($items) > $field['cardinality']) {
-      form_error($element, t('%name: this field cannot hold more than @count values.', array('%name' => t($instance['label']), '@count' => $field['cardinality'])));
-    }
-  }
 }
 
 /**
diff --git a/modules/field/modules/options/options.test b/modules/field/modules/options/options.test
index 46d29b782..0f9cb4afa 100644
--- a/modules/field/modules/options/options.test
+++ b/modules/field/modules/options/options.test
@@ -85,7 +85,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Select first option.
     $edit = array("card_1[$langcode]" => 0);
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_1', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'card_1', $langcode, array(0));
 
     // Check that the selected button is checked.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -96,7 +96,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Unselect option.
     $edit = array("card_1[$langcode]" => '_none');
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_1', $langcode, array());
+    $this->assertFieldValues($entity_init, 'card_1', $langcode, array());
 
     // Required radios with one option is auto-selected.
     $this->card_1['settings']['allowed_values'] = '99|Only allowed value';
@@ -145,7 +145,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
       "card_2[$langcode][2]" => TRUE,
     );
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array(0, 2));
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array(0, 2));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -160,7 +160,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
       "card_2[$langcode][2]" => FALSE,
     );
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array(0));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -185,7 +185,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     );
     $this->drupalPost(NULL, $edit, t('Save'));
     // Check that the value was saved.
-    $this->assertListValues($entity_init, 'card_2', $langcode, array());
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array());
 
     // Required checkbox with one option is auto-selected.
     $this->card_2['settings']['allowed_values'] = '99|Only allowed value';
@@ -227,7 +227,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: select first option.
     $edit = array("card_1[$langcode]" => 0);
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_1', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'card_1', $langcode, array(0));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -238,7 +238,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: Unselect the option.
     $edit = array("card_1[$langcode]" => '_none');
     $this->drupalPost('test-entity/' . $entity->ftid .'/edit', $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_1', $langcode, array());
+    $this->assertFieldValues($entity_init, 'card_1', $langcode, array());
 
     // A required select list does not have an empty key.
     $instance['required'] = TRUE;
@@ -281,7 +281,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: select first and third options.
     $edit = array("card_2[$langcode][]" => array(0 => 0, 2 => 2));
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array(0, 2));
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array(0, 2));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -292,7 +292,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: select only first option.
     $edit = array("card_2[$langcode][]" => array(0 => 0));
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array(0));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -308,7 +308,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: uncheck all options.
     $edit = array("card_2[$langcode][]" => array());
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array());
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array());
 
     // Test the 'None' option.
 
@@ -316,12 +316,12 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // as well.
     $edit = array("card_2[$langcode][]" => array('_none' => '_none', 0 => 0));
     $this->drupalPost('test-entity/' . $entity->ftid .'/edit', $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array(0));
 
     // Check that selecting the 'none' option empties the field.
     $edit = array("card_2[$langcode][]" => array('_none' => '_none'));
     $this->drupalPost('test-entity/' . $entity->ftid .'/edit', $edit, t('Save'));
-    $this->assertListValues($entity_init, 'card_2', $langcode, array());
+    $this->assertFieldValues($entity_init, 'card_2', $langcode, array());
 
     // A required select list does not have an empty key.
     $instance['required'] = TRUE;
@@ -362,7 +362,7 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: check the option.
     $edit = array("bool[$langcode]" => TRUE);
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'bool', $langcode, array(0));
+    $this->assertFieldValues($entity_init, 'bool', $langcode, array(0));
 
     // Display form: check that the right options are selected.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
@@ -371,33 +371,11 @@ class OptionsWidgetsTestCase extends DrupalWebTestCase {
     // Submit form: uncheck the option.
     $edit = array("bool[$langcode]" => FALSE);
     $this->drupalPost(NULL, $edit, t('Save'));
-    $this->assertListValues($entity_init, 'bool', $langcode, array(1));
+    $this->assertFieldValues($entity_init, 'bool', $langcode, array(1));
 
     // Display form: with 'off' value, option is unchecked.
     $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
     $this->assertNoFieldChecked("edit-bool-$langcode");
   }
-
-  /**
-   * Assert that a 'list' field has the expected values in an entity.
-   *
-   * @param $entity
-   *   The entity to test.
-   * @param $field_name
-   *   The name of the field to test
-   * @param $langcode
-   *   The language code for the values.
-   * @param $expected_values
-   *   The array of expected values.
-   */
-  function assertListValues($entity, $field_name, $langcode, $expected_values) {
-    $e = clone $entity;
-    field_attach_load('test_entity', array($e->ftid => $e));
-    $values = isset($e->{$field_name}[$langcode]) ? $e->{$field_name}[$langcode] : array();
-    $this->assertEqual(count($values), count($expected_values), t('Expected number of values were saved.'));
-    foreach ($expected_values as $key => $value) {
-      $this->assertEqual($values[$key]['value'], $value, t('Option @value was saved correctly.', array('@value' => $value)));
-    }
-  }
 }
 
diff --git a/modules/field/tests/field.test b/modules/field/tests/field.test
index 418017f68..1bbb36744 100644
--- a/modules/field/tests/field.test
+++ b/modules/field/tests/field.test
@@ -40,6 +40,32 @@ class FieldTestCase extends DrupalWebTestCase {
     }
     return $values;
   }
+
+  /**
+   * Assert that a field has the expected values in an entity.
+   *
+   * This function only checks a single column in the field values.
+   *
+   * @param $entity
+   *   The entity to test.
+   * @param $field_name
+   *   The name of the field to test
+   * @param $langcode
+   *   The language code for the values.
+   * @param $expected_values
+   *   The array of expected values.
+   * @param $column
+   *   (Optional) the name of the column to check.
+   */
+  function assertFieldValues($entity, $field_name, $langcode, $expected_values, $column = 'value') {
+    $e = clone $entity;
+    field_attach_load('test_entity', array($e->ftid => $e));
+    $values = isset($e->{$field_name}[$langcode]) ? $e->{$field_name}[$langcode] : array();
+    $this->assertEqual(count($values), count($expected_values), t('Expected number of values were saved.'));
+    foreach ($expected_values as $key => $value) {
+      $this->assertEqual($values[$key][$column], $value, t('Value @value was saved correctly.', array('@value' => $value)));
+    }
+  }
 }
 
 class FieldAttachTestCase extends FieldTestCase {
@@ -1036,7 +1062,6 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
     $values = array();
     for ($delta = 0; $delta < $this->field['cardinality']; $delta++) {
       $values[$delta]['value'] = -1;
-      $values[$delta]['_error_element'] = 'field_error_' . $delta;
     }
     // Arrange for item 1 not to generate an error
     $values[1]['value'] = 1;
@@ -1060,6 +1085,17 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
       }
     }
     $this->assertEqual(count($errors[$this->field_name][$langcode]), 0, 'No extraneous errors set');
+
+    // Check that cardinality is validated.
+    $entity->{$this->field_name}[$langcode] = $this->_generateTestFieldValues($this->field['cardinality'] + 1);
+    try {
+      field_attach_validate($entity_type, $entity);
+    }
+    catch (FieldValidationException $e) {
+      $errors = $e->errors;
+    }
+    $this->assertEqual($errors[$this->field_name][$langcode][0][0]['error'], 'field_cardinality', t('Cardinality validation failed.'));
+
   }
 
   /**
@@ -1538,11 +1574,6 @@ class FieldFormTestCase extends FieldTestCase {
     // Test with several multiple fields in a form
   }
 
-  // Check with a multiple widget (implement a textfield with comma separated values).
-
-  // Check inaccessible fields are preserved on update.
-  // Check inaccessible fields get default value on insert (not implemented yet).
-
   function testFieldFormJSAddMore() {
     $this->field = $this->field_unlimited;
     $this->field_name = $this->field['field_name'];
@@ -1598,6 +1629,46 @@ class FieldFormTestCase extends FieldTestCase {
   }
 
   /**
+   * Tests widgets handling multiple values.
+   */
+  function testFieldFormMultipleWidget() {
+    // Create a field with fixed cardinality and an instance using a multiple
+    // widget.
+    $this->field = $this->field_multiple;
+    $this->field_name = $this->field['field_name'];
+    $this->instance['field_name'] = $this->field_name;
+    $this->instance['widget']['type'] = 'test_field_widget_multiple';
+    field_create_field($this->field);
+    field_create_instance($this->instance);
+    $langcode = LANGUAGE_NONE;
+
+    // Display creation form.
+    $this->drupalGet('test-entity/add/test-bundle');
+    $this->assertFieldByName("{$this->field_name}[$langcode]", '', t('Widget is displayed.'));
+
+    // Create entity with three values.
+    $edit = array("{$this->field_name}[$langcode]" => '1, 2, 3');
+    $this->drupalPost(NULL, $edit, t('Save'));
+    preg_match('|test-entity/(\d+)/edit|', $this->url, $match);
+    $id = $match[1];
+
+    // Check that the values were saved.
+    $entity_init = field_test_create_stub_entity($id);
+    $this->assertFieldValues($entity_init, $this->field_name, $langcode, array(1, 2, 3));
+
+    // Display the form, check that the values are correctly filled in.
+    $this->drupalGet('test-entity/' . $id . '/edit');
+    $this->assertFieldByName("{$this->field_name}[$langcode]", '1, 2, 3', t('Widget is displayed.'));
+
+    // Submit the form with more values than the field accepts.
+    $edit = array("{$this->field_name}[$langcode]" => '1, 2, 3, 4, 5');
+    $this->drupalPost(NULL, $edit, t('Save'));
+    $this->assertRaw('this field cannot hold more than 4 values', t('Form validation failed.'));
+    // Check that the field values were not submitted.
+    $this->assertFieldValues($entity_init, $this->field_name, $langcode, array(1, 2, 3));
+  }
+
+  /**
    * Tests fields with no 'edit' access.
    */
   function testFieldFormAccess() {
diff --git a/modules/field/tests/field_test.field.inc b/modules/field/tests/field_test.field.inc
index 89def7e10..8db03401c 100644
--- a/modules/field/tests/field_test.field.inc
+++ b/modules/field/tests/field_test.field.inc
@@ -157,20 +157,56 @@ function field_test_field_widget_info() {
  * Implements hook_field_widget().
  */
 function field_test_field_widget(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
-  $element = array(
-    'value' => $element + array(
-      '#type' => 'textfield',
-      '#default_value' => isset($items[$delta]['value']) ? $items[$delta]['value'] : '',
-    ),
-  );
-  return $element;
+  switch ($instance['widget']['type']) {
+    case 'test_field_widget':
+      $element += array(
+        '#type' => 'textfield',
+        '#default_value' => isset($items[$delta]['value']) ? $items[$delta]['value'] : '',
+      );
+      return array('value' => $element);
+
+    case 'test_field_widget_multiple':
+      $values = array();
+      foreach ($items as $delta => $value) {
+        $values[] = $value['value'];
+      }
+      $element += array(
+        '#type' => 'textfield',
+        '#default_value' => implode(', ', $values),
+        '#element_validate' => array('field_test_widget_multiple_validate'),
+      );
+      return $element;
+  }
+}
+
+/**
+ * Form element validation handler for 'test_field_widget_multiple' widget.
+ */
+function field_test_widget_multiple_validate($element, &$form_state) {
+  $values = array_map('trim', explode(',', $element['#value']));
+  $items = array();
+  foreach($values as $value) {
+    $items[] = array('value' => $value);
+  }
+  form_set_value($element, $items, $form_state);
 }
 
 /**
  * Implements hook_field_widget_error().
  */
 function field_test_field_widget_error($element, $error) {
-  form_error($element['value'], $error['message']);
+  // @todo No easy way to differenciate widget types, we should receive it as a
+  // parameter.
+  if (isset($element['value'])) {
+    // Widget is test_field_widget.
+    $error_element = $element['value'];
+  }
+  else {
+    // Widget is test_field_widget_multiple.
+    $error_element = $element;
+  }
+
+  form_error($error_element, $error['message']);
 }
 
 /**
diff --git a/modules/taxonomy/taxonomy.module b/modules/taxonomy/taxonomy.module
index a15e7b4ba..e21728c9d 100644
--- a/modules/taxonomy/taxonomy.module
+++ b/modules/taxonomy/taxonomy.module
@@ -1046,17 +1046,6 @@ function taxonomy_field_validate($obj_type, $object, $field, $instance, $langcod
   $allowed_values = taxonomy_allowed_values($field);
   $widget = field_info_widget_types($instance['widget']['type']);
 
-  // Check we don't exceed the allowed number of values for widgets with custom
-  // behavior for multiple values (taxonomy_autocomplete widget).
-  if ($widget['behaviors']['multiple values'] == FIELD_BEHAVIOR_CUSTOM && $field['cardinality'] >= 2) {
-    if (count($items) > $field['cardinality']) {
-      $errors[$field['field_name']][$langcode][0][] = array(
-        'error' => 'taxonomy_term_illegal_value',
-        'message' => t('%name: this field cannot hold more that @count values.', array('%name' => t($instance['label']), '@count' => $field['cardinality'])),
-      );
-    }
-  }
-
   foreach ($items as $delta => $item) {
     if (!empty($item['tid'])) {
       if (!isset($allowed_values[$item['tid']])) {