Support multiple default values for fields with cardinality > 1

Yeah, I asked about the UI impact in lieu of the discussion at http://groups.drupal.org/node/210973.

Basically, for single-value fields, there would be no change.

For multi-value fields, if the field has cardinality N, N default value widgets would be displayed. if the field has unlimited cardinality, a single default value widget + add more button would be displayed.

This obviously would be a data structure change. However, it's on the admin side, and generally falls under the "Adding more of what we have" thing rather than violent adjustments. So I'd like to go ahead with the backport, assuming there are no dire concerns of doom from anyone.

I'd totally support this.

Internally, $instance['default_value'] is already handled as "an array of multiple values".
It's just Field UI that doesn't let you enter more than one, just because the 'add more' button only worked on entity forms back then.

So this is "only" a matter of making sure the "add more" button works (JS and non-JS) when included in the 'edit instance settings' form - which basically means handling multistep.
A little far off right now, but it's very possible that the refactors done in #942310: Field form cannot be attached more than once have done part of the job already.

The diffstat for this should show more removals than additions, as it effectively removes a hack. Thus, +1, totally makes sense.

I think this was partially fixed now with the move to plugins. Play around with the body field, that at least brings multiple widgets on the settings page, haven't tested on an actual form though. Doesn't work for images, but that's because of the definition of the widget iirc ?

Yeah, this has been fixed in D8 as a side effect of "widgets as plugins", the "default values" input now has the 'add more' button and allows entering multiple values.
Image files don't have that because they have their own handling of the notion of 'default image'.

Looks like this still needs discussion as to whether it's possible to add in Drupal 7?

Yes it might be possible for D7 as I have partially implemented the feature for cardinality >1 but not for 'unlimited' because of its complexity of using 'add more' button for later. Anyways right now on selecting the cardinality >1 it populates the Default value text box to that number with proper #description.(Check the screenshots for cardinality 2 and then for selecting 3). For 'unlimited' it replaces the default textbox (Check screenshot for unlimited). I have submitted the patch. Need suggestion!

Looks like a nice start! Not sure about the "Default value 1", "Default value 2" descriptions... Drupal 8 doesn't have those, and I don't think they're necessary here?

Right, plus big -1 on modifying field_default_form(), this is what generates widgets on all entity forms, so I'd say this is forbidden territory :-)

This would need to be done using the regular "several widgets in a d-n-d table" output that is found in regular entity forms.
You should check what happens if you just remove the 0 at the end of the field_default_form() call in field_ui_default_value_widget(), like so :

-  $element += field_default_form($instance['entity_type'], NULL, $field, $instance, LANGUAGE_NONE, $items, $element, $form_state, 0);
+  $element += field_default_form($instance['entity_type'], NULL, $field, $instance, LANGUAGE_NONE, $items, $element, $form_state);

So regarding #description, its just a additional put up to track those multiple default fields. I thought from this user will get an idea about which field he/she is going to provide default value for. I am going to bring those changes yched has mentioned but what you guys think about the 'unlimited' case ?

Thanks yched you were right about that change. It does work for me also improvises the look(check the screenshots) still didn't work for 'unlimited' like earlier. Submitted the patch for D7.14 with needed change.

Resubmitted the patch

#14: 1482386.patch queued for re-testing.

#20 is not working for me (with a multi-line text field) - I click "Add more" and the existing field disappears.

Yes you are right. It won't work in case of Unlimited cardinality result of which adds "Add more" for multi-line text field in the Default value section, as I have already mentioned it at #8 also asked for suggestion. Unfortunately I have to change my approach due to some implication in #ajax callback function and its wrapper as because of which is unable to render element via field_add_more_js() function.

Henceforth followed the approach of D8 and implemented the same for D7.x which now shifted the cardinality section to Field Setting form. Submitted the patch with screenshots(its working now for multi-line field).

Resubmitted patch with test and notice fix.

Resubmitted patch

Works for me. Thanks.

Works for me.

Just awesome, just what I needed. Works great, tested with an unlimited-value field collection field!

26: 1482386.patch queued for re-testing.

Patch #26 works.

26: 1482386.patch queued for re-testing.

Tests pass, so back to RTBC for now.

Sorry, I'm confused why this patch is moving the cardinality setting from the instance settings form to the field settings form?

That may or may not be a good idea, but it's a big change that certainly has lots of implications beyond the goal of this issue that would need to be discussed... I think there might even be another issue for it somewhere.

If the goal is just to support everything except unlimited fields for now (which I suppose sounds like it might be reasonable) do we really need anything besides a small part of the patch? (See attached.)

If you change the cardinality and want to change the default values accordingly you'll have to go back to the settings form a second time to edit them, but I think that's the way the default value form works for other things too - it is sometimes affected by various choices made on the instance settings form and you need to go back and edit a second time for those changes to be reflected.

@David_Rothstein: I guess we could live with that - although I'm not sure what happens if you set 3 default values and set back cardinality to < 3 on the same submit ?

Also - I thought "add more" button did work for "default values on fields with unlimited cardinality", but I might be confused.

I think the extra default values would just be removed. I suppose it's not great to be doing that without any warning, so maybe we need form validation to catch that case and prevent the form from being submitted?

I'm pretty sure when I tested this, I did not see any "add more" button in the case of unlimited cardinality... just a single default value field.

Hello? Any update?

RTBC for #61. It is reasonable to just support that for fixed value cardinality first.

Manually tested this and this seems to work fine, but it would be nice if we could add a check for #63.

And just a nit, the description says "The default value for this field, used when creating new content." -- shouldn't this be "default values" (plural)?

Form validation now checks for a mismatch between number of entered values and cardinality.
And that latest nit was taken into account.

Works fine... except for Image fields.
The code from #61 does not change the number of file upload controls available for the "Default image", there is only one.
Any suggestions?

Thanks!

Can we somehow make the error message a bit easier to understand for non-technical users? We'll also need to get rid of the anonymous function as PHP 5.2 doesn't support those :)

How about 'you have entered more default values (%count_non_empty_items) than the given cardinality setting allows (%cardinality).' ?

Tagging string freeze as a new string is likely added by this.

+ PHP 5.2 compatible (did away with the anonymous function callback and array_filter).
+ Changed the message to a slightly less technical =>

Some field: you have entered more default values (4) than allowed by the current cardinality (2).

Looks better! This will also need to account for unlimited cardinality (-1). And maybe we can use "Maximum number of values" instead of cardinality (as that's what we call it elsewhere on the form)?

We also missed #title for the plural. And is "singular(s)" a pattern we use elsewhere in Drupal core or would format_plural() be better here?

Thanks for the feedback, here is another go:

+ Using format_plural for the default value field's label and description.
+ Taking the -1 (cardinality => unlimited) into consideration in the validation logic
+ Re-phrasing the validation error message to

Body: you have entered more default values (3) than allowed by the Number of values setting.

Assigning to David so he can give this a final look.

Nit to be fixed on commit: 'Number of values' will need to be wrapped in t()

Looks pretty good but I think there are a couple issues:

1. -    '#title' => t('Default value'),
   +    '#title' => format_plural($current_cardinality, 'Default value', 'Default values'),
        '#collapsible' => FALSE,
        '#tree' => TRUE,
   -    '#description' => t('The default value for this field, used when creating new content.'),
   +    '#description' => format_plural($current_cardinality, 'The default value for this field, used when creating new content.', 'The default values for this field, used when creating new content.'),
   

   I don't think this is necessary, and actually it could be confusing in some cases (it might make you think you need to add multiple values, when in fact you can still just add one if you want).

   Doesn't the widget itself make it visually clear that multiple values are allowed?

   That combined with the fact that this would be adding a new translatable string.... I'd be inclined to just leave this change out.

2. +    // Checking for a difference between the existing default values 
   +    // and the cardinality setting.
   

   (minor) Should be "Check" rather than "Checking".

3. +    $non_empty_items_count = 0;
   +    foreach ($items as $item) {
   +      if (!empty($item['value'])) {
   +        $non_empty_items_count++;
   +      }
   +    }
   

   I'm not sure this is a robust way to check for empty field values. I think you might need to do something more like what field_default_validate() does (in particular, call _field_filter_items()).

4. +        'message' => t('%name: you have entered more default values (@count_non_empty_items) than allowed by the %cardinality_setting_label setting.', array('%name' => $instance['label'], '@count_non_empty_items' => $non_empty_items_count, '@cardinality' => $current_cardinality, '%cardinality_setting_label' => 'Number of values')),
   

   This looks mostly OK, but using the word "you" there is a little odd.

   Could we just use the same exact text that field_default_validate() uses instead? It's a fair amount shorter, and also would mean that the patch doesn't have to add any new translatable strings.

5. Do we want to add any tests here? (Does Drupal 8 have any tests for this that could be backported? - if not, maybe don't worry about it)

Tagging for Dublin and will be working on this from the sprint rooms.

Still getting around to providing some feedback to the latest review from D.Rothstein.

For the tests, maybe look at Drupal\field_ui\Tests\ManageFieldsTest::cardinalitySettings() and/or Drupal\field_ui\Tests\ManageFieldsTest::testDefaultValue()

+++ b/modules/field_ui/field_ui.admin.inc
@@ -2015,6 +2016,7 @@ function field_ui_field_edit_form_validate($form, &$form_state) {
+  $current_cardinality = isset($form_state['values']['field']['cardinality']) ? $form_state['values']['field']['cardinality'] : 1;

@@ -2032,6 +2034,20 @@ function field_ui_field_edit_form_validate($form, &$form_state) {
+    // Checking for a difference between the existing default values ¶
+    // and the cardinality setting.
+    $non_empty_items_count = 0;
+    foreach ($items as $item) {
+      if (!empty($item['value'])) {
+        $non_empty_items_count++;
+      }
+    }
+    if ($current_cardinality > 0 && $non_empty_items_count > $current_cardinality) {
+      $errors[$field['field_name']][LANGUAGE_NONE][0][] = array(
+        'error' => 'cardinality_smaller_than_existing_default_values',
+        'message' => t('%name: you have entered more default values (@count_non_empty_items) than allowed by the %cardinality_setting_label setting.', array('%name' => $instance['label'], '@count_non_empty_items' => $non_empty_items_count, '@cardinality' => $current_cardinality, '%cardinality_setting_label' => 'Number of values')),
+      );
+    }

It seems this functionality is not in Drupal 8.

Can we test if it is, and if not, leave this out of the current patch and create a Drupal 8 issue for this first?

Tested this on Drupal 8, it is not there. Here are the steps for testing:

1. Create a field, e.g. "Text (plain, long)"
2. Set the cardinality to 3 items and save your field's config
3. Enter the default values for the 3 text items and save your field's config
4. Set the cardinality to 2 items and save your field's config
5. The third of the previously entered default values is missing.

This seems like a missing feature from the configuration validation in the Field UI.
In one sentence: when the user reduces the cardinality of a field that has existing default values, a validation error should be thrown. This will safeguard against losing the user's work on field configuration.

This is the related D8 issue, tagged for backporting => https://www.drupal.org/node/2809779

Could somebody verify that the following points are valid?

a) The changes in the field_ui_field_edit_form_validate() function (see #84) need to be ported to D8 and are handled in a separate issue #2809779: Cardinality validation for fields with existing default values.
b) The tests in D8 do not cover the case of multiple default values, any new tests that do should probably be implemented in a separate issue as well as backported to D7.
c) If we don't modify the title and description to use plural forms, as mentioned in the first point in #80, we end up with the exact same patch as @David_Rothstein submitted in #61

So, all in all, it could very well be that we just ended back at #61.

If that is the case, we could probably close this one quite easily.

Does this will be shipped along with the next core release ?