New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Time#change
should pass the zone
property through if it's set
#42583
Conversation
82e9f43
to
27b7c82
Compare
@@ -159,6 +159,8 @@ def change(options) | |||
::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, new_offset) | |||
elsif utc? | |||
::Time.utc(new_year, new_month, new_day, new_hour, new_min, new_sec) | |||
elsif zone && zone.is_a?(ActiveSupport::TimeZone) | |||
::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, zone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my first attempt at a fix merged this branch and the next one, but Time.new
doesn't like zones formatted as some kinds of strings, eg. it was crashing on ::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, "CST")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time.zone
isn't necessarily an instance of ActiveSupport::TimeWithZone
- Ruby allows any object there as long as it quacks in the correct manner. Perhaps a zone.respond_to?(:utc_to_local)
would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the docs I was trying to find! I'll tidy it up, thanks.
27b7c82
to
49a81b1
Compare
@ghiculescu Can you add a changelog too plz? 🙇 |
49a81b1
to
4cc47ca
Compare
4cc47ca
to
d132095
Compare
👍 changelog added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that Time
instances with zone objects has a few more bugs lurking in Active Support 😬
Backport #42583: `Time#change` should pass the `zone` property through if it's set
Fixes #42467