Skip to content

Commit

Permalink
Change the behavior of boolean columns to be closer to Ruby's semantics.
Browse files Browse the repository at this point in the history
Before this change we had a small set of "truthy", and all others
are "falsy".

Now, we have a small set of "falsy" values and all others are
"truthy" matching Ruby's semantics.
  • Loading branch information
rafaelfranca committed Jan 4, 2015
1 parent 07d3d40 commit a502703
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 19 deletions.
9 changes: 9 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
* Change the behavior of boolean columns to be closer to Ruby's semantics.

Before this change we had a small set of "truthy", and all others are "falsy".

Now, we have a small set of "falsy" values and all others are "truthy" matching
Ruby's semantics.

*Rafael Mendonça França*

* Deprecate `ActiveRecord::Base.errors_in_transactional_callbacks=`.

*Rafael Mendonça França*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ module ActiveRecord
module ConnectionAdapters
# An abstract definition of a column in a table.
class Column
TRUE_VALUES = [true, 1, '1', 't', 'T', 'true', 'TRUE', 'on', 'ON'].to_set
FALSE_VALUES = [false, 0, '0', 'f', 'F', 'false', 'FALSE', 'off', 'OFF'].to_set

module Format
Expand Down
15 changes: 3 additions & 12 deletions activerecord/lib/active_record/type/boolean.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,10 @@ def type
def cast_value(value)
if value == ''
nil
elsif ConnectionAdapters::Column::TRUE_VALUES.include?(value)
true
else
if !ConnectionAdapters::Column::FALSE_VALUES.include?(value)
ActiveSupport::Deprecation.warn(<<-MSG.squish)
You attempted to assign a value which is not explicitly `true` or `false`
to a boolean column. Currently this value casts to `false`. This will
change to match Ruby's semantics, and will cast to `true` in Rails 5.
If you would like to maintain the current behavior, you should
explicitly handle the values you would like cast to `false`.
MSG
end
elsif ConnectionAdapters::Column::FALSE_VALUES.include?(value)
false
else
true
end
end
end
Expand Down
10 changes: 4 additions & 6 deletions activerecord/test/cases/types_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ def test_type_cast_boolean
assert type.type_cast_from_user('TRUE')
assert type.type_cast_from_user('on')
assert type.type_cast_from_user('ON')
assert type.type_cast_from_user(' ')
assert type.type_cast_from_user("\u3000\r\n")
assert type.type_cast_from_user("\u0000")
assert type.type_cast_from_user('SOMETHING RANDOM')

# explicitly check for false vs nil
assert_equal false, type.type_cast_from_user(false)
Expand All @@ -28,12 +32,6 @@ def test_type_cast_boolean
assert_equal false, type.type_cast_from_user('FALSE')
assert_equal false, type.type_cast_from_user('off')
assert_equal false, type.type_cast_from_user('OFF')
assert_deprecated do
assert_equal false, type.type_cast_from_user(' ')
assert_equal false, type.type_cast_from_user("\u3000\r\n")
assert_equal false, type.type_cast_from_user("\u0000")
assert_equal false, type.type_cast_from_user('SOMETHING RANDOM')
end
end

def test_type_cast_float
Expand Down

5 comments on commit a502703

@connorshea
Copy link
Contributor

@connorshea connorshea commented on a502703 Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca it's a bit late to ask, but would it be acceptable to add a note about the removal of ConnectionAdapters::Column::TRUE_VALUES in the Changelog? I realize no one was intended to depend on that, but apparently we did at GitLab and that caused a bit of headache. Would have been nice to not have to search as hard to find this commit.

@arthurnn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connorshea As far as I can tell, the TRUE_VALUES constant was public. So yeah, we could add a note to the changelog. WDYT @rafaelfranca ?

@connorshea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in #26619, thanks @arthurnn :)

@runephilosof
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it helped :)

@runephilosof
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention that FALSE_VALUE has also been moved to ActiveModel::Type::Boolean::FALSE_VALUES

Please sign in to comment.