[html-formfu] checkbox strangeness

[Geoffrey D. Bennett]

On Mon, Aug 25, 2008 at 09:35:18AM +0100, Carl Franks wrote:
[...]
> > All this seems a bit excessive for what I would have thought would be
> > a reasonably common case?
> >
> > I've now got:
> >
> > default_args:
> >  elements:
> >    Checkbox:
> >      value: 1
> >      default_empty_value: 1
> >      inflator: Checkbox
> >
> > But don't those seem like reasonable defaults anyway?
> 
> You should always be setting 'value' for checkboxes - so that the
> browser knows what to send if the checkbox is checked.

Hi Carl,

Should this be documented in HTML::FormFu::Element::Checkbox?

You wrote in another thread:

> The reason that's not a default, is that checkboxes aren't always a
> simple boolean [...]

Can you explain this statement?  The only non-boolean checkboxes I
could think of would be greyed-out disabled ones (but then you'd be
ignoring the value, which would still be a boolean), or a checkbox
that represents a collection of items some of which are in a "true"
state and others in a "false" state, and the checkbox is therefore in
a "mixed" state (although, I don't think HTML supports this).

Even if checkboxes aren't always a simple boolean, I'm sure that's not
the common case, and given that almost everything else about
HTML::FormFu "just works" so nicely, I don't yet understand why
checkbox handling needs extra work done by the person writing the
config file to get the common case working correctly.

By working correctly I mean that a minimal config:

  - type: Checkbox
    name: testcheck
    label: Test

results in the checkbox retaining the value submitted if the form is
redisplayed.

(and that the database gets updated with TRUE/FALSE based on the
checkbox checked status when $form->update() is called, but see
later in this email).

What do you think of this:

Index: lib/HTML/FormFu/Element/Checkbox.pm
===================================================================
--- lib/HTML/FormFu/Element/Checkbox.pm (revision 1204)
+++ lib/HTML/FormFu/Element/Checkbox.pm (working copy)
@@ -38,23 +38,20 @@
         ? $self->get_nested_hash_value( $form->input, $self->nested_name )
         : undef;
 
-    if (   $submitted
-        && defined $value
-        && defined $original
-        && $value eq $original )
+    if ($submitted && $value)
     {
         $render->{attributes}{checked} = 'checked';
     }
     elsif ($submitted
         && $self->retain_default
-        && ( !defined $value || $value eq $EMPTY_STR ) )
+        && $default)
     {
         $render->{attributes}{checked} = 'checked';
     }
     elsif ($submitted) {
         delete $render->{attributes}{checked};
     }
-    elsif ( defined $default && defined $original && $default eq $original ) {
+    elsif ($default) {
         $render->{attributes}{checked} = 'checked';
     }
 

This lets $value, $original, $default be any "true" value to get the
checked attribute set.  (and makes retain_default only set checked
when the default is for the checkbox to be checked, like the
Element::_Field docs imply (to me anyway))

> How are you updating the database?

I'm not doing anything special to update the database.  Just:

  if ($form->submitted_and_valid) {
    my $row = $c->model($model_name)->new_result({});
    $form->model->update($row);
  }

or

  if ($form->submitted_and_valid) {
    my $row = $c->model($model_name)->find($id);
    $form->model->update($row);
  }

The relevant bit from the schema file is:

__PACKAGE__->add_columns(
  ...
  "active",
  {
    data_type => "boolean",
    default_value => "true",
    is_nullable => 0,
    size => 1,
  },
);

> HTML-FormFu-Model-DBIC handles checkboxes correctly, without needing
> 'default_empty_value'.

I don't think it does.  When the checkbox is off, the value is undef,
so inside HTML::FormFu::Model::DBIC::_fix_value, $value is undef, and
therefore gets set to $col_info->{default_value}.  My boolean field in
the database is "DEFAULT TRUE".  That doesn't mean I want it set TRUE
when the checkbox is off.

What do you think of something like this?

Index: lib/HTML/FormFu/Model/DBIC.pm
===================================================================
--- lib/HTML/FormFu/Model/DBIC.pm       (revision 1204)
+++ lib/HTML/FormFu/Model/DBIC.pm       (working copy)
@@ -571,8 +571,14 @@
     my $is_nullable = $col_info->{is_nullable} || 0;
     my $data_type   = $col_info->{data_type} || '';
     if ( defined $value ) {
-        if ( (     $is_nullable
-                || $data_type =~ m/^timestamp|date|int|float|numeric/i
+        if (   $data_type eq "boolean"
+            && defined $field
+            && $field->isa('HTML::FormFu::Element::Checkbox') )
+        {
+            $value = $value ? 1 : 0;
+        }
+        elsif ( (     $is_nullable
+                   || $data_type =~ m/^timestamp|date|int|float|numeric/i
             )
 
             # comparing to '' does not work for inflated objects
@@ -587,9 +593,7 @@
         if ( defined $field
             && $field->isa('HTML::FormFu::Element::Checkbox') )
         {
-            if ( !$is_nullable ) {
-                $value = $col_info->{default_value};
-            }
+            $value = 0;
         }
     }
     return $value;

That way, any boolean checkbox fields will get set true/false even
when value and default_empty_value are not set.

Regards,
-- 
Geoffrey D. Bennett, RHCE, RHCX               mailto:g at netcraft.com.au
Senior Systems Engineer                          sip:g at netcraft.com.au
NetCraft Australia Pty Ltd        http://www.netcraft.com.au/geoffrey/