Use default values to #disabled form fields

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.

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.)

i'll agree that this is a bug from a developer perspective.

i'll agree that this is a bug from a developer perspective.

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.)

It's as if the user typed empty strings.

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...

Code in question is here. See _form_builder_handle_input_element()

    if (($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.

the browser [...] doesn't send disabled fields to the server
[...]
Modules that use disabled textfields [...] It's as if the user typed empty strings.

Actually, it's as if the field was not on the page [...]
For text fields, an empty string will still be in the request data

(When one says "actually," he means to fix some error said previously --but you're just repeating what I said ;-)

code should check for the #disabled flag

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.

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.

Actually, it's as if the field was not on the page [for fields that are disabled] [...]
For text fields [that are not disabled, but left empty by the user], an empty string will still be in the request data

(When one says "actually," he means to fix some error said previously --but you're just repeating what I said ;-)

Am I? I've added the bold parts to clarify...

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?

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.

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.

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.

+      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?

// 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']))) {

This still looks questionable to me. I don't see that the confusing flow control is justified here...

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.

Does it really lose that much clarity if you simply reverse the condition and put the comment outside the if-block?

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.

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_callback for 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 in form_type_checkboxe_value() and form_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.

@mooffie [...] please accept reviews

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.

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. ;)

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: Allow FAPI select, radios, and checkboxes to specify some options as disabled

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?

@perforator: technically there is no patch to test here, yet. But thanks for your report.

I played around with it a bit, without any actual valuable knowledge of the FAPI to solve my problem stated in #21.

if( empty($form['#attributes']['disabled']) && (isset($edit) || ($is_checkbox && !$form['#programmed'])))

to

if( 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

Subscribing to this so I can keep an eye on it (seems to be the problem in one of the modules I installed)...

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.

subscribing.

subscribing. The webforms module would definitely benefit from a fix to this.

@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']))) ?>

Subscribing

Suscribing

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.

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.

Subscribing

this is not a bug, disabled elements should not send back values to server.
i hope drupal core developer not change it.

Thanks, everyone, for sharing thoughts and ideas on this. Yes, this is a bug. There's two separate pieces:

1) If no input is submitted for a disabled element (as one would expect from a browser), then $form_state['values'] should contain the element's default value (that's what it means to be a default value).

2) If input is submitted for a disabled element (let's say, because javascript enabled the element, or someone wrote an application to perform an HTTP submission that emulates a form submission), then FAPI should ignore this input, and populate $form_state['values'] with the element's default value.

Both of these is already the case for checkboxes and needs to be made that way for all elements. The implementation details have been worked out on #426056: Server-side enforcement of #disabled is inconsistent, and consensus was reached there, and that issue is just awaiting a separate fix to the test framework, so that the patch can be re-rolled with sufficient tests to ensure that these things stay fixed.

So, marking this a duplicate. Feel free to subscribe/comment on the other issue.

'