Fix for time type columns with invalid time value #7337

Merged
merged 2 commits into from Sep 5, 2012

Projects

None yet

3 participants

@adzap
Contributor
adzap commented Aug 12, 2012

The string_to_dummy_time method was blindly parsing the dummy time string
with Date._parse which returns a hash for the date part regardless
of whether the time part is an invalid time string.

@danbeaulieu

+1. Is there a work around in the meantime?

@rafaelfranca
Member

What is the current behavior? What is the expected behavior? And why? Please send more information about the issue, it will make easier to review and merge.

@danbeaulieu

@rafaelfranca,
I have an open question on SO about this: http://stackoverflow.com/questions/12263135/time-type-field-getting-bad-default-datetime-if-it-is-invalid

Current behavior is if you have a :time type attribute on a model and you set it to some invalid time, like 'foo', ActiveRecord happily parses it into a valid datetime object.

Expected behavior would be similar to :date and :datetime where if the input is invalid, nil is the result.

Essentially what happens with :time is it comes down to Date._parse("2000-01-01 foo") which will parse into a hash with some values, but since this is specifically a :time type, it doesn't validate the the hash has at least a value for :hour.

Hope this helps.

@rafaelfranca
Member

Make sense. @adzap lets move this forward to merge. Could you add a CHANGELOG entry?

@adzap
Contributor
adzap commented Sep 4, 2012

@rafaelfranca will do.

@danbeaulieu thanks for putting in the explanation.

adzap added some commits Aug 12, 2012
@adzap adzap Fix for time type columns with invalid time
The string_to_dummy_time method was blindly parsing the dummy time string
with Date._parse which returns a hash for the date part regardless
of whether the time part is an invalid time string.
ce7cdb9
@adzap adzap Update changelog with time column type casting fix acf583a
@adzap
Contributor
adzap commented Sep 5, 2012

@rafaelfranca Changelog updated. This change can/should be back ported to v3 branches too.

@rafaelfranca rafaelfranca merged commit 91183ba into rails:master Sep 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment