Use default values to #disabled form fields
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Under drupal 5, if we had a form field declared with '#disabled' => true the field would appear on the form displayed, but be grayed out (no editing by the user).. upon submission of the form, we could retrieve the value of the field just like any other ($form_values['Test1']). We generally used this for occasional places were we wanted to show a field on the form, but not let the user touch it. (As opposed to a hidden field, that we neither need them to see OR touch)...
However, since upgrading to Drupal6 (Confirmed in 6.1 as of this morning when I upgraded it), all #disabled fields are no longer passing back a value in the new $form_state['values'] array... well, there is a place for it in the array, but the values are always blank.
For instance, we snuck a drupal_set_message("".print_r($form_state['values'],1)."") into the top of our submit function to see what was going on)
Array
(
[step] => 2
[payref] => 1234567890
[aid] => 75
[account] =>
[status] => 5
[invoice] => 802230006
[paymentmethod] => 2
[amount] => 29.95
[oldduedate] =>
[nextduedate] => 2008-04-01
[op] => Post Payment
[submit] => Post Payment
[form_build_id] => form-eefd37749f53ca496abfe746d657beac
[form_token] => d405b813e23dabd54c08dcc1f197b570
[form_id] => abc_accounting_apply_payment_form
)In this instance, account and oldduedate were the two fields we were displaying but marking "#disabled" on the form itself. In the form displayed to the user, they contained data... in the values returned to submit, they are blank.
Not sure if this is a planned design change from D5 to D6 (if it was, I couldn't find tell of it in the upgrade docs currently), but figured since I couldn't find docs mentioning it, and since the forms reference still just says "Disables (greys out) a form input element." I better get it out there in case this issue driving anyone else crazy...
This is all on a standard LAMP box that returned said values when run under Drupal5.
#1
I can confirm the bug.
It seems the problem is here:
if (!$form['#programmed'] || isset($edit)) {// Call #type_value to set the form value;
if (function_exists($function)) {
$form['#value'] = $function($form, $edit);
}
$function($form,$edit), which is used to translate the form input, is called even when there's no input. "No input" happens on two occasions: on #disbabled fields and on unchecked checkboxes. And since$function($form,$edit)returns something, FAPI doesn't continue to consult #default_value --which is how things should go for #disabled fields.#2
I should mention that the browser, in accordance with section 17.13.2 of the HTML spec, doesn't send disabled fields to the server.
(And this means that wherever we have disabled checkboxes we should be concerned about bugs.)
#3
i'll agree that this is a bug from a developer perspective.
#4
The meaning of this bug:
Modules that use disabled textfields are likely to obliterate this data when the form is submitted: It's as if the user typed empty strings.
I say 'textfields' because textareas, non-#multiple selects, and radios aren't affected by this bug.
(As for checkboxes... they are tough to tackle because there's no way to differentiate between unchecked checkboxes and disabled checked checkboxes. I already linked to one bug which was caused because of this, and, since there's a usability move to disable irrelevant fields, we're likely to see more such bugs.)
#5
Actually, it's as if the field was not on the page. For checkboxes, there is no difference, as an unchecked checkbox will not be appended to the request data anyway. For text fields, an empty string will still be in the request data as
fieldname=, while a disabled text field will not be.That means
isset($_POST['fieldname'])will be false at the Form API level. Once the Form API sanitizes the input against the original form, it probably puts empty strings into the form state - this would be an ideal time at which to put in the default value instead...#6
Code in question is here. See _form_builder_handle_input_element()
<?phpif (($form['#programmed']) || ((!isset($form['#access']) || $form['#access']) && isset($form['#post']) && (isset($form['#post']['form_id']) && $form['#post']['form_id'] == $form_id))) {
$edit = $form['#post'];
foreach ($form['#parents'] as $parent) {
$edit = isset($edit[$parent]) ? $edit[$parent] : NULL;
}
if (!$form['#programmed'] || isset($edit)) {
// Call #type_value to set the form value;
if (function_exists($function)) {
$form['#value'] = $function($form, $edit);
}
if (!isset($form['#value']) && isset($edit)) {
$form['#value'] = $edit;
}
}
?>
The code should check for the #disabled flag and copy #default_value into #value if the field is disabled.
#7
(When one says "actually," he means to fix some error said previously --but you're just repeating what I said ;-)
It shouldn't, because fields may get disabled via Javascript. However, checking for the #disabled flag is the only means to detect checkboxes that are disabled (not via Javascript), so I included it in the patch.
The patch contains an explanation.
A word about style: the body of my 'if' is empty. I felt it would be clearer this way.
#8
This is a different version of the patch, which I'm uploading just for brainstorming. It's how my patch looked in the beginning. I don't expect it to get commited as it looks too 'radical'. So don't bother to test it.
The "what to do if no input?" logic was placed here in the form_type_XYZ_value() hooks. This leaves the main FAPI code to be blessfully ignorant of the field type (that is, you don't see 'if $is_checkbox ...' here). So this solution is more 'pure', more object oriented.
But since only checkbox(es) require special treatment I folded it into the much shorter version you see in #7.
#9
Am I? I've added the bold parts to clarify...
#10
I have the same problem with Drupal 5.x.
Problem is with: checkboxes, select and multi-select using node reference and custom select cck widget.
Anybody knows similar bug in 5.x?
#11
I am able to confirm that the patch shown in #7 was workable for me, and seems to have resolved the issues I'd had with #disabled form fields. (Both in my own inhouse modules, as well as problems using pathauto with D6 and the "Automatic alias" checkbox it provides to the node edit form).
Not enough of a FAPI expert to feel confident marking this as "reviewed" myself, but definitely a +1 for it fixing the problem as I've encountered it. :)
#12
Looks good to me. Is there some easy way to test this? We probably need a patch for HEAD as well.
The HTML spec is pretty annoying for these disabled elements and for checkboxes. Gosh I wish we could just change it.
#13
The patch wasn't applying cleanly to the latest D6 dev version, so I re-rolled it. I also confirmed that it fixed the issue I ran into where I was unable to submit a form with a field that I had designated as both disabled and required.
#14
Hi.
Added a patch for Drupal 6.3 which bases on mooffie's patch from #7.
This solves a problem I had with 'wikitools' Click!. Hopefully no side effects will appear.
#15
+ if (!empty($form['#attributes']['disabled']) || (!isset($edit) && (!$is_checkbox || $form['#programmed']))) {+ // We pick the default value in three circumstances:
+ // 1. The field is reported to be disabled; or
+ // 2. No input is available and this isn't a checkbox; or
+ // 2. No input is available, this is a checkbox, and the form is programmed.
+ }
+ else {
// Call #type_value to set the form value;
This still looks questionable to me. I don't see that the confusing flow control is justified here...
Does it really lose that much clarity if you simply reverse the condition and put the comment outside the if-block?
<?php// We pick the default value in three circumstances:
// 1. The field is reported to be disabled; or
// 2. No input is available and this isn't a checkbox; or
// 3. No input is available, this is a checkbox, and the form is programmed.
if (empty($form['#attributes']['disabled']) && (isset($edit) || ($is_checkbox && !$form['#programmed']))) {
?>
#16
When you say "looks questionable" you're expressing your doubt about the correctness of the code (and without providing explanations...). But then you go on to better --in your opinion-- some style issue. That's a mishmash: correctness and style are different things.
And when you use the word "still" (in "still looks questionable") you insinuate that you had some doubt about this patch before. Where? I don't see it.
Arancaytar, you should speak loud and clear. Only loud and clear.
First,
Yes, IMHO you lose clarity. You reversed the condition, breaking its parallelism with the English comment. And you might give the false impression that it's the body of the 'if' that deals with "picking the default value". I fail to see how your revised conditional is clearer than my "confusing flow control".
Second,
"Really lose that much" is a namby-pamby phrase. Almost anybody would be glad to to hold hands in a circle and agree "sure, we don't really lose that much".
Having said that,
I don't at all feel strongly about this issue.
#17
Hum. Ok.
First, because this bug is also on the development version (7.x), it needs to be fixed there, and then (and only then) back-ported.
Second, the Form API has a
#value_callbackfor a reason: not having to deal with specifics of each field type in generic functions like_form_builder_handle_input_element(). We need to deal with the generic case of #disabled fields in that function, but handle the special case of checkboxes inform_type_checkboxe_value()andform_type_checkboxes_value()(which is sort of the idea mooffie had in #8, but without any need to touch other _value functions).Third, @mooffie: please keep changes as minimal as possible. Also please accept reviews, that people may be thinking out loud, and that some may have a different opinion. The logic in your patch in #7 may be good, but you totally inverted the condition, making your change hard to review.
#18
My words seemed belligerent. I apologize for this, espcecially to Arancaytar; that would be a most unfortunate reading of my words. I wish I could change the past, those words. Alas.
#19
And I apologize for my poor choice of words too. I tend to place great weight on the code style, because I do have a lot of difficulty understanding other people's code. ;)
#20
this is a valuable patch. hope someone can find time for it. ... also, would be great to get a review from moofie, damien and others at #284917: Add disabled attribute to select options
#21
Hi.
I ran into a problem with the patch. The Drupal 6 feature of "Split summary at cursor" creates a summary "sub page". But upon saving (and preview) it's content disappears! With the original form.inc it works. Tested with Drupal 6.3 and the normal Story node type.
Can anybody confirm this?
#22
@perforator: technically there is no patch to test here, yet. But thanks for your report.
#23
I played around with it a bit, without any actual valuable knowledge of the FAPI to solve my problem stated in #21.
<?phpif( empty($form['#attributes']['disabled']) && (isset($edit) || ($is_checkbox && !$form['#programmed'])))
?>
to
<?phpif( empty($form['#attributes']['disabled']) || (isset($edit) || ($is_checkbox && !$form['#programmed'])))
?>
seems to work. Yet no sideeffects for me, but I will test on. Again: I don't have a clue about this, so can anybody please take a look at the issue and my "shoot in the dark"?
Best wishes
#24
Subscribing to this so I can keep an eye on it (seems to be the problem in one of the modules I installed)...
#25
Comment #23 from perforator.
woW! it seems to work great... still did not notice any problems so far..
i'm using it for wikitools as well, good job.
#26
subscribing.
#27
subscribing. The webforms module would definitely benefit from a fix to this.
#28
@perforator - regarding #23
where should that change apply? form.inc?
the patch from #14 has the following similar line of code but it differs from the what you have in #23:
<?php if (!empty($form['#attributes']['disabled']) || (!isset($edit) && (!$is_checkbox || $form['#programmed']))) ?>#29
Subscribing
#30
Suscribing
#31
In D6 form_type_checkbox_value deals with disabled for the 'checkbox' element. If that's broken, tell me, but I'd be surprised. For the rest (like textfields) we could create a patch.
#32
This most certainly is a bug.
Imagine the scenario where a field is required (or has some other validation), when editing the field has #disabled = TRUE (is greyed out), now the form doesn't pass validation as the value is not being passed back for processing, so the edit can't be submitted - ever.
Just my two pennith worth.