Skip to content
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 columns should support time zone aware attributes #15726

Merged
merged 1 commit into from
Jan 15, 2015

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Jun 14, 2014

The types that are affected by time_zone_aware_attributes (which is on by default) have been made configurable, in case this not the desired behavior for existing applications.

Fixes #3145

@rafaelfranca
Copy link
Member

Hmm, maybe this will make the upgrade path more painful. WDYT about keeping the default [:datetime] and generating [:datatime, :time] for new applications?

@sgrif
Copy link
Contributor Author

sgrif commented Jun 19, 2014

I have no strong feelings, I'll make the change.

@matthewd
Copy link
Member

If we feel that [:datetime, :time] should be the default going forward, perhaps we should output a deprecation warning if it's not explicitly configured one way or the other? (While still defaulting to the backward-compatible [:datetime] setting, for now.)

@sgrif
Copy link
Contributor Author

sgrif commented Jun 20, 2014

I have no strong feelings on what the default should be.
On Jun 19, 2014 6:52 PM, "Matthew Draper" notifications@github.com wrote:

If we feel that [:datetime, :time] should be the default going forward,
perhaps we should output a deprecation warning if it's not explicitly
configured one way or the other? (While still defaulting to the
backward-compatible [:datetime] setting, for now.)


Reply to this email directly or view it on GitHub
#15726 (comment).

@rafaelfranca
Copy link
Member

@matthewd's plan is good to me.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 26, 2014

I've changed the default back to [:datetime], and emitted a deprecation warning. I opted to go slightly more conservatively than suggested, since :time columns are not terribly commonly used, and I'd rather not have every user get a deprecation warning. The way I went about implementing is slightly hacky, so I'd appreciate another review.

@@ -1,3 +1,17 @@
* `time` columns can now affected by `time_zone_aware_attributes`. If you have
Copy link
Member

Choose a reason for hiding this comment

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

"can now be affected" here, no ?

The types that are affected by `time_zone_aware_attributes` (which is on
by default) have been made configurable, in case this is a breaking
change for existing applications.
sgrif added a commit that referenced this pull request Jan 15, 2015
Time columns should support time zone aware attributes
@sgrif sgrif merged commit b814d8c into rails:master Jan 15, 2015
@sgrif sgrif deleted the sg-time-zone-aware-times branch January 15, 2015 15:08
time_zone_aware_types.include?(:not_explicitly_configured)
ActiveSupport::Deprecation.warn(<<-MESSAGE)
Time columns will become time zone aware in Rails 5.1. This
sill cause `String`s to be parsed as if they were in `Time.zone`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: sill

claudiob added a commit to claudiob/rails that referenced this pull request Sep 9, 2015
[ci skip]

@sgrif can you review when you have time? Thanks!
claudiob added a commit that referenced this pull request Sep 9, 2015
Fix docs of AR::Timestamp to match #15726
claudiob added a commit to claudiob/rails that referenced this pull request Mar 22, 2016
This PR integrates rails#15726, under which `Time` columns should support time
zone aware attributes.

In order for this to be the **default behavior** on brand new Rails 5.0 apps,
an initializer is required. Without it, even brand new Rails 5.0 apps would be
showing the following deprecation warning, which should be reserved to apps
_ported_ to Rails 5.0 from an older version:

```
Time columns will become time zone aware in Rails 5.1. This
still causes `String`s to be parsed as if they were in `Time.zone`,
and `Time`s to be converted to `Time.zone`.
To keep the old behavior, you must add the following to your initializer:
    config.active_record.time_zone_aware_types = [:datetime]
To silence this deprecation warning, add the following:
    config.active_record.time_zone_aware_types << :time
```

@sgrif Is the above correct?
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time is created in UTC with local time
6 participants