PHP notice when submitting form with disabled checkbox

Subscribing. I'd like chx's thoughts on this.

#3: checkbox-disabled-4.patch queued for re-testing.

A glance from chx would be nice if he were available, but no need to hold this up any longer. It's a nice bug-fix, makes sense, I can't think of unwanted side-effects, it comes with tests (or rather, strengthening of an existing test), and now that bot is back and retested it successfully, it seems good to go.

That's OK. I am so happy checkboxes get attention and sorry for not reviewing this sooner!

Committed to CVS HEAD. Thanks!

last commit says it's been rolled back.

This was breaking the module page, and hence the test bot. I asked Dries to roll it back.

Hmm, this patch does two things:

1) it wants to fix the checkbox specific tests regarding the value for unchecked checkboxes.
2) It tries to fix the PHP notice, but actually breaks checkboxes.
Before the patch an error could occur and the empty string would become the element #value for disabled checkboxes with no #default_value. After the patch integer 0 can become the element value for a disabled but checked checkbox... Auch.

#426056: Server-side enforcement of #disabled is inconsistent also gives love to checkboxes, but there it was decided to remove the special behaviour for checkboxes. With that patch at the moment for a checkbox the value will be the empty string if no #default_value is available, which can be problematic too...

The reason #4 caused an error for the modules page is because it made the requirement for #default_value to be equal to #return_value too strict, and therefore, a #return_value of 1 with a #default_value of TRUE (as is the case for checkboxes on the modules page) resulted in the checkbox being treated as unchecked after submission. I'm not sure yet how bot didn't catch that (missing tests for the modules page?). This patch fixes the problem, and includes the necessary FAPI tests for it, but it doesn't do anything about the missing test coverage of the modules page.

I'd like this issue resolved first, and then I'll do the necessary re-roll for #426056: Server-side enforcement of #disabled is inconsistent.

This doesn't really qualify as something trivial any more, so removing tag. An RTBC or feedback would be much appreciated though.

Different issue, but might be of interest to the people subscribed to this one: #712990: Test framework does not correctly emulate browser submission of disabled elements

The strict comparison is still there and causes some issues. Consider these scenarios:

#return_value: '5'; #default_value: 5 => with this patch we will get element value 0 and an unchecked box, while now we get value 5 and a checked box. I think we should keep the current behaviour while fixing the notice.
EDIT: meaning: keep it checked. We should still return #return_value rather than #default_value, to be consistent with the enabled case.

#return_value: 0; #default_value: '0' => with this patch we will get element value 0 and an unchecked box, while now we get value '0' and a checked box. I'm sure some people will argue that the current behaviour is better because in Drupal PHP it should be possible to declare an integer for your return value, always.

So here are some requirements while fixing the notice:

1) The #default_value FALSE value must not map to #return_value values '0' and 0. (FALSE renders to checkbox value '' and should therefore map to #return_value ''.)
2) The #default_value integer 0 is a special case but apart from that we want #default_value numeric string values to map to the corresponding #return_value integers and we want #return_value numeric strings to map to the corresponding #default_value integers.
3) A disabled checkbox should lead to the exact same element value as a browser submitted value from an enabled checkbox.

So how about this fragment?

+      return isset($element['#default_value']) && $element['#default_value'] !== 0 && (string) $element['#default_value'] == (string) $element['#return_value'] ? (string) $element['#return_value'] : 0;

Erhm, when there is input from an enabled checkbox element the unmanipulated FAPI #return_value is set as element value, so we should not cast to string for disabled elements.
So this is the consistent one:

+      return isset($element['#default_value']) && $element['#default_value'] !== 0 && (string) $element['#default_value'] == (string) $element['#return_value'] ? $element['#return_value'] : 0;

double post...

Auch, I think we have been missing something big. All patches here are focussing on the case where $input !== FALSE and !empty($element['#disabled']), which is the case where our accessible (as in #access) but disabled element somehow has a defined $input variable. This the the special case where checkbox decides to ignore input. It should return the default value if it exists or 0. Nothing else is needed.

Now for the case where $input === FALSE, meaning we should return a default. Perhaps #access was FALSE, i.e. no rendering and no submitting values for this checkbox. Again, we should return either #default_value if it exists or 0. Unfortunately we are returning nothing for this case, which means the element value will be set to #default_value if it exists or to '' if it does not. Oops.

In short. We should keep it simple and we should handle the case where $input === FALSE.

I agree that we shouldn't require a strict comparison of #default_value with #return_value, but should instead take the lead from theme_checkbox() and make it a non-strict comparison instead. However, NULL, 0, 1, TRUE, and FALSE are special. This patch makes that more readable.

This does mean that #return_value of NULL, integer 0, or FALSE can't be used in any meaningful way, but that's desired. These are absolutely terrible options for the #return_value of a checkbox, and even without this patch, they likely cause bugs or WTFs.

x-post with #19, but #20 addresses that.

Seems that the notice is now back again in form_type_checkbox_value() for one special case? (After submitting a form with an accessible, disabled, unchecked checkbox without a #default_value.) EDIT: NOPE, that code is all gone, so no notice, but if no default value then that means an element value of '' instead of 0? EDIT 2: NOPE, first time around NULL is passed and nothing is returned, nothing assigned, so now we pass FALSE which *will* return either $element['#return_value'] or 0, so we don't need a default value. Reaaly need some sleep now.

Then we have these cases where $element['#default_value'] equals TRUE, FALSE or NULL.
Initial build, $input === FALSE
Without this patch form_type_checkbox_value() will return nothing and element values will be based on default values leading to values TRUE, FALSE and ''. Checked or unchecked depends on a comparison of default value and return value.
With patch, form_type_checkbox_value() checks default values and returns 0 for these three, leading to element values of 0, leading to unchecked boxes regardless of return value.

Is the intended behaviour documented somewhere?

@Eric_A: thanks for your thoroughness on this and the related issues. I agree with #8: it's great to have more attention being paid to the arcane issues with checkboxes. The whole checkbox thing has been a FAPI difficulty for years, and I can definitely see some value in documenting it better. Perhaps starting either in http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... or http://api.drupal.org/api/drupal/developer--topics--forms_api_reference...., or as some special "what you need to know about FAPI and checkboxes" handbook page.

Then we have these cases where $element['#default_value'] equals TRUE, FALSE or NULL....With patch, form_type_checkbox_value() checks default values and returns 0 for these three, leading to element values of 0, leading to unchecked boxes regardless of return value. Is the intended behaviour documented somewhere?

I think you mean integer 0, FALSE, or NULL, and I'm quite confident the patch is a fix and HEAD behavior is a bug, though quite an edge case, since no one in their right mind should set #return_value to one of these three. Probably the most likely edge case that's currently a problem in HEAD is if someone sets #return_value to '' (empty string) and #default_value to NULL (or leaves it unspecified), then HEAD considers this checked (clearly not what anyone intended) and with this patch, that's fixed to unchecked, but even this is an edge case, because other than trying to make FAPI break, why would you set #return_value to empty string?

I don't have time now, but I think we need to check this out: #654796: Identify fix bugs with checkbox element.
Perhaps this one, too: #227966: Use default values to #disabled form fields

Whichever of this one or #654796: Identify fix bugs with checkbox element lands first, the other will need a re-roll, but the two issues can progress independently.

I simplified this patch quite a bit. There was stuff in #19 that was new code and inappropriate for this issue. This one, however, I believe is within this issue's scope, and I believe the tests are sufficiently thorough, and cover the condition that caused #11.

Hoping someone RTBCs.

+  // A checkbox with #default_value of TRUE should always be checked and a
+  // checkbox with #default_value of FALSE or integer 0 should always be
+  // unchecked as long as #return_value is legitimate: NULL, empty string, and
+  // FALSE are not legitimate options for #return_value, and the behavior of
+  // such a checkbox is not specified, not to be tested here, and may change as
+  // Drupal code changes.

The empty string is a valid xhtml checkbox value. It may end up in $input. Why is it not legitimate in $element['#return_value']?

+    // A checkbox value, even the default value, must always be either
+    // #return_value or numeric 0, so #default_value can only be used to
+    // determine the default checked state, but must not be returned.
+    $checked = isset($element['#default_value']) && $element['#default_value'] !== 0 && $element['#default_value'] == $element['#return_value'];
+    return $checked ? $element['#return_value'] : 0;

As far as I know HEAD now explicitly checks for integer 0 everywhere (except for a few broken tests that expect the empty string), so as to allow for the string value of "0".
In other words, should it not be "integer 0" instead of "numeric 0"?

In other words, should it not be "integer 0" instead of "numeric 0"?

Due to legacy only, code comments use the phrase "numeric 0", but they intend to mean "integer 0". I agree with you about it being desirable to change these comments to use "integer 0", and that's done so in #426056-30: Server-side enforcement of #disabled is inconsistent.

The empty string is a valid xhtml checkbox value. It may end up in $input. Why is it not legitimate in $element['#return_value']?

You're so right. But HEAD is broken when using #return_value of empty string or string "0". We need this fixed and thorough tests written: #654796: Identify fix bugs with checkbox element.

I'm postponing this issue until that one is fixed, mostly so that we have robust tests, so that we don't have a repeat of #11. It may well be that the process of fixing that other issue will include code refactoring that fixes this one too.

This sounds similar/related to #2911473: Selected yet disabled individual options from checkboxes element don't persist through save

This seems to be fixed in D7 after #654796: Identify fix bugs with checkbox element. I was not able to trigger the Notice/Warning using the steps from the issue summary. Therefore closing this.