Currently no matter which cardinality a field has, only a single field widget is displayed to enter the field default value. Instead it would be nice to have the possibility to submit more than one default field item. Moreover, as noted in http://drupal.org/node/1224710, this was possible in CCK. AAMOF there is a @todo about this in the relevant code.
Obviously this would be useful in D7 too, not sure this is actually possible. Tentatively tagging for backport after talking with @webchick.
Comments
Comment #1
webchickYeah, 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.
Comment #2
yched CreditAttribution: yched commentedI'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.
Comment #3
sunThe diffstat for this should show more removals than additions, as it effectively removes a hack. Thus, +1, totally makes sense.
Comment #4
swentel CreditAttribution: swentel commentedI 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 ?
Comment #5
yched CreditAttribution: yched commentedYeah, 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'.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like this still needs discussion as to whether it's possible to add in Drupal 7?
Comment #8
monish_deb CreditAttribution: monish_deb commentedYes 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!
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedLooks 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?
Comment #10
yched CreditAttribution: yched commentedRight, 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 :
Comment #11
monish_deb CreditAttribution: monish_deb commentedSo 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 ?
Comment #12
monish_deb CreditAttribution: monish_deb commentedThanks 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.
Comment #14
monish_deb CreditAttribution: monish_deb commentedResubmitted the patch
Comment #16
monish_deb CreditAttribution: monish_deb commented#14: 1482386.patch queued for re-testing.
Comment #18
monish_deb CreditAttribution: monish_deb commentedComment #20
monish_deb CreditAttribution: monish_deb commentedComment #21
Owen Barton CreditAttribution: Owen Barton commented#20 is not working for me (with a multi-line text field) - I click "Add more" and the existing field disappears.
Comment #22
monish_deb CreditAttribution: monish_deb commentedYes 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).
Comment #24
monish_deb CreditAttribution: monish_deb commentedResubmitted patch with test and notice fix.
Comment #26
monish_deb CreditAttribution: monish_deb commentedResubmitted patch
Comment #27
Aleksey Zubko CreditAttribution: Aleksey Zubko commentedWorks for me. Thanks.
Comment #28
Aleksey Zubko CreditAttribution: Aleksey Zubko commentedComment #29
mastervissie CreditAttribution: mastervissie commentedWorks for me.
Comment #30
lunk rat CreditAttribution: lunk rat commentedJust awesome, just what I needed. Works great, tested with an unlimited-value field collection field!
Comment #31
mgifford26: 1482386.patch queued for re-testing.
Comment #32
mastervissie CreditAttribution: mastervissie commentedComment #33
Yuri CreditAttribution: Yuri commentedPatch #26 works.
Comment #35
mgifford26: 1482386.patch queued for re-testing.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedTests pass, so back to RTBC for now.
Comment #39
dcam CreditAttribution: dcam commentedComment #42
dcam CreditAttribution: dcam commentedComment #45
dcam CreditAttribution: dcam commentedComment #48
dcam CreditAttribution: dcam commentedComment #51
dcam CreditAttribution: dcam commentedComment #54
mgiffordComment #57
dcam CreditAttribution: dcam commentedComment #60
dcam CreditAttribution: dcam commentedComment #61
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, 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.
Comment #62
yched CreditAttribution: yched commented@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.
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #64
kopeboy CreditAttribution: kopeboy commentedHello? Any update?
Comment #65
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC for #61. It is reasonable to just support that for fixed value cardinality first.
Comment #67
stefan.r CreditAttribution: stefan.r commentedManually 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)?
Comment #68
stefanos.petrakis@gmail.comForm 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?
Comment #70
stefan.r CreditAttribution: stefan.r commentedThanks!
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 :)
Comment #71
stefanos.petrakis@gmail.comHow about 'you have entered more default values (%count_non_empty_items) than the given cardinality setting allows (%cardinality).' ?
Comment #72
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedTagging string freeze as a new string is likely added by this.
Comment #73
stefanos.petrakis@gmail.com+ PHP 5.2 compatible (did away with the anonymous function callback and array_filter).
+ Changed the message to a slightly less technical =>
Comment #76
stefan.r CreditAttribution: stefan.r commentedLooks 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?
Comment #77
stefan.r CreditAttribution: stefan.r commentedComment #78
stefanos.petrakis@gmail.comThanks 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
Comment #79
stefan.r CreditAttribution: stefan.r commentedAssigning 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()
Comment #80
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedLooks pretty good but I think there are a couple issues:
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.
(minor) Should be "Check" rather than "Checking".
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()).
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.
Comment #81
stefanos.petrakis@gmail.comTagging for Dublin and will be working on this from the sprint rooms.
Comment #82
stefanos.petrakis@gmail.comStill getting around to providing some feedback to the latest review from D.Rothstein.
Comment #83
stefan.r CreditAttribution: stefan.r commentedFor the tests, maybe look at Drupal\field_ui\Tests\ManageFieldsTest::cardinalitySettings() and/or Drupal\field_ui\Tests\ManageFieldsTest::testDefaultValue()
Comment #84
stefan.r CreditAttribution: stefan.r commentedIt 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?
Comment #85
stefanos.petrakis@gmail.comTested this on Drupal 8, it is not there. Here are the steps for testing:
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
Comment #86
stefanos.petrakis@gmail.comCould 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.
Comment #87
Jyodsh CreditAttribution: Jyodsh commentedDoes this will be shipped along with the next core release ?