Bug #19293
closedThe new Time.new(String) API is nice... but we still need a stricter version of this
Description
The Ruby 3.2 style Time.new(String)
API works very well so far, but since the original Time.new(Integer, Integer, Integer...)
API actually accepts String objects as its arguments, there's one ambiguous case as follows:
Time.new('20230123') #=> 20230123-01-01 00:00:00 +0900
Then the problem that I'm facing is that we cannot tell if Time.new
would parse the given String as ISO8601-ish or just a year, and in order to avoid this ambiguity, we still need to somehow parse the String beforehand in our application side (like we're doing this way in Ruby on Rails https://github.com/rails/rails/blob/c49b8270/activemodel/lib/active_model/type/helpers/time_value.rb#L64-L70), then dispatch to the new Time.new
only when the String is validated to be conforming the ISO format. Otherwise, if we just optimistically pass in given Strings to Time.new
, we'll occasionally get a Time object with an unintended buggy value.
Therefore, it unfortunately seems that my feature request on #16005 still continues... I have to keep proposing that we need either of the following:
-
A trustworthy version of ISO8601 parser method perhaps with another name than
.new
that accepts strict ISO8601-ish String only (but with the T delimiter, I still don't know what the proper name of this format is). -
Change
Time.new(Integer-ish, Integer-ish, Integer-ish...)
not to accept Integer-ish Strings but to accept only Integers. But I can imagine that this direction is very unlikely acceptable, due to the incompatibility.
Files
Updated by nobu (Nobuyoshi Nakada) almost 2 years ago
- File time_benchmark.rb time_benchmark.rb added
Kernel#Integer
may be easier (and probably faster) than the Regexp.
Time.new(string) unless Integer(string, exception: false)
Benchmark.
$ ruby time_benchmark.rb
Warming up --------------------------------------
active_model 33.895k i/100ms
time 78.272k i/100ms
Calculating -------------------------------------
active_model 365.327k (± 0.9%) i/s - 1.830M in 5.010500s
time 943.682k (± 1.0%) i/s - 4.775M in 5.060040s
BTW, fast_string_to_time
seems having a bug on the negative offset calculation.
Updated by byroot (Jean Boussier) almost 2 years ago
A trustworthy version of ISO8601 parser method perhaps with another name than .new that accepts strict ISO8601-ish String only
Can we improve Time.iso8601
?
Alternatively we could introduce a method based on https://www.rfc-editor.org/rfc/rfc3339. RFC 3339 is pretty much the date and time part of ISO8601 but publicly documented.
Updated by Eregon (Benoit Daloze) almost 2 years ago
I think a new method is cleaner. Or maybe reusing Time.iso8601
.
It feels a hack for Time.new two wildly different things based on the number of arguments and as shown here it's ambiguous.
I had the same concerns when Time.new started supporting this, so "told you so" feeling here.
Updated by mame (Yusuke Endoh) almost 2 years ago
Discussed at the dev meeting. @naruse (Yui NARUSE) said "Time.new("2023")
is 1.9.2 feature. We can’t deprecate on 3.2.1. Need to continue the discussion for Ruby 3.3."
BTW Ruby 3.2.0 accidentally allows Time.new("2023-01")
, Time.new("2023-01-01")
, and Time.new(" \n\n 2023-01-01 00:00:00 \n\n ")
. All of these are considered a bug, so will be prohibited (fixed) in Ruby 3.2.1.
Updated by jeremyevans0 (Jeremy Evans) over 1 year ago
- Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN to 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED
mame (Yusuke Endoh) wrote in #note-4:
BTW Ruby 3.2.0 accidentally allows
Time.new("2023-01")
,Time.new("2023-01-01")
, andTime.new(" \n\n 2023-01-01 00:00:00 \n\n ")
. All of these are considered a bug, so will be prohibited (fixed) in Ruby 3.2.1.
Looks like this wasn't fixed in 3.2.1, 3.2.2, or the master branch. I've submitted a pull request to fix it in the master branch, which could be backported to 3.2.3: https://github.com/ruby/ruby/pull/7974
Updated by mame (Yusuke Endoh) over 1 year ago
Discussed at the dev meeting. The conclusion is:
- Fix it in master (towards Ruby 3.3)
- @nagachika (Tomoyuki Chikanaga) will decide whether to backport to 3.2.3 or not.
Updated by jeremyevans (Jeremy Evans) over 1 year ago
- Status changed from Open to Closed
Applied in changeset git|5d4fff845602872eef072e7611558b5f8762efe0.
Tighten Time.new(string) parsing
Disallow:
- Only year-month
- Only year-month-day
- Preceding whitespace
- Trailing whitespace
Fixes [Bug #19293]
Updated by nagachika (Tomoyuki Chikanaga) over 1 year ago
- Backport changed from 3.0: DONTNEED, 3.1: DONTNEED, 3.2: REQUIRED to 3.0: DONTNEED, 3.1: DONTNEED, 3.2: DONE
ruby_3_2 0b3ed6043c9d091d499ca1caed604a983c7e285b merged revision(s) 5d4fff845602872eef072e7611558b5f8762efe0.
Updated by Eregon (Benoit Daloze) 16 days ago
- Related to Feature #18033: Time.new to parse a string added