Suggestion for improving value_to_boolean column conversion

Hi fantastic rails core developers.

Speaking of version 2.3.5:

I find it rather error_prone that values such that "some text" is
silently converted to false when stored in a boolean DB field.

Among other things it means that assign "some text" to boolean fields
on a model will not generate any validation messages (it will silently
be converted to false).

I think it is like silently swallowing exception without knowing what
to do about them.

I will suggest to modify the existing implementation of
ActiveRecord::ConnectionAdapters::Column::value_to_boolean

to
ActiveRecord::ConnectionAdapters::Column.class_eval %q{
  def self.value_to_boolean(value)
    case value
    when *TRUE_VALUES.to_a: true
    when *FALSE_VALUES.to_a: false
    else nil
    end
  end
}

First of all it simplifies the implementation (getting rid of initial
if. secondly it will at least result in nil if value is neither
recognised among TRUE_VALUES nor FALSE_VALUES

Jarl

I think it is like silently swallowing exception without knowing what
to do about them.

I will suggest to modify the existing implementation of
ActiveRecord::ConnectionAdapters::Column::value_to_boolean

to
ActiveRecord::ConnectionAdapters::Column.class_eval %q{
def self.value_to_boolean(value)
case value
when *TRUE_VALUES.to_a: true
when *FALSE_VALUES.to_a: false
else nil
end
end
}

First of all it simplifies the implementation (getting rid of initial
if. secondly it will at least result in nil if value is neither
recognised among TRUE_VALUES nor FALSE_VALUES

The initial if is there deliberately to allow people with html forms
to transmit a nil value by sending a blank string. But as for the
coerce to nil suggestion
that would be similarly silent for users. People who didn't set their
columns to NOT NULL would just start having NULL values showing up
where previously they were false. Seems like this would replace one
slightly strange situation with another, and not necessarily improve
things

Michael Koziarski <michael@koziarski.com> writes:

I think it is like silently swallowing exception without knowing what
to do about them.

I will suggest to modify the existing implementation of
ActiveRecord::ConnectionAdapters::Column::value_to_boolean

to
ActiveRecord::ConnectionAdapters::Column.class_eval %q{
def self.value_to_boolean(value)
case value
when *TRUE_VALUES.to_a: true
when *FALSE_VALUES.to_a: false
else nil
end
end
}

First of all it simplifies the implementation (getting rid of initial
if. secondly it will at least result in nil if value is neither
recognised among TRUE_VALUES nor FALSE_VALUES

The initial if is there deliberately to allow people with html forms
to transmit a nil value by sending a blank string. But as for the
coerce to nil suggestion
that would be similarly silent for users. People who didn't set their
columns to NOT NULL would just start having NULL values showing up
where previously they were false. Seems like this would replace one
slightly strange situation with another, and not necessarily improve
things

I agree, I see your point. Optimally I would suggest raising an
ArgumentError opposed to returning nil.. But there seems to be a
return-nil-instead-of-raise-exception practice in the coerce
functions. so I suggested "else nil" to follow that convention.

Currently the implementation of value_to_boolean does not follow the
convention of returning nil, where input is not coerceable. Many other
coerce functions return nil in these situation, see
ActiveRecord::ConnectionAdapters::Column.string_to_date("bullshit") => nil

But I also see that this "convention" is not general for all coerce
functions:
ActiveRecord::ConnectionAdapters::Column.value_to_decimal("asdfasdf") => #<BigDecimal:7fa16c93fe08,'0.0',9(9)>

Actually my frustration was this:
http://groups.google.com/group/rubyonrails-core/browse_thread/thread/69b8a475f60c3f24#

I assume now that I see value_to_decimal returns 0.0 as default value,
I would have the same frustration about

validates_inclusion_of :decimal_field, :in => [0]

I think I will raise a bug on the more high level problem, and let the
core developers decided how it should be implemented.

Jarl