-
Notifications
You must be signed in to change notification settings - Fork 183
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
legacyDateTypeProcessing flag for datetime processing #265
Conversation
- fix TZ changes to datetime type too
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
@DaGaMs Do you ming adding some unit tests on this PR to take care of your end scenario? |
test/date_types.test.js
Outdated
@@ -232,7 +232,7 @@ describe('MySQL DATE, DATETTIME, TIMESTAMP types on server with non local TZ (+0 | |||
}); | |||
|
|||
var prepareModel = function(tz, done) { | |||
db = getSchema({timezone: tz}); | |||
db = getSchema({timezone: tz, legacyDateTypeProcessing:false}); |
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.
Why are we defaulting to false? We should be defaulting to true to preserve old behaviour and forcing users to turn on the flag for the new major/breaking behaviour.
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.
default was 'undefined' or true. these tests were on fixed date types processing. but it is not mater now, because #257 was reverted. :(
Hey @superkhau I wrote a long post yesterday discussing this and previous PR at #149 (comment) I also present 2 possible paths forward that I think are better options than #257 + #265 . Both of the options I present include the legacy support of this PR but with legacy behavior as default. |
This commit contains all previous work after rebase went badly RE: Pull Request requested by @kjdelisle regarding loopbackio#149 following my proposed solution "A" at loopbackio#149 (comment). As mentioned in the post linked above, this change allows the underlying mysqljs/mysql module to handle Date object serialization and removes the forced conversion of Dates to UTC Strings. An opt-out fallback to forced coercion to UTC is included, which was modeled after loopbackio#265 from @darknos . Old, squashed commits: d0ea1d9 Legacy UTC date processing fallback credit: @darknos a59dad7 Remove orphaned string functions e8fdbdc Incorporate @darknos expanded check for zero dates abd4e0a Remove DATE manipulations in from/toColumnValue
Conflicts: lib/mysql.js test/date_types.test.js
This commit contains all previous work after rebase went badly RE: Pull Request requested by @kjdelisle regarding loopbackio#149 following my proposed solution "A" at loopbackio#149 (comment). As mentioned in the post linked above, this change allows the underlying mysqljs/mysql module to handle Date object serialization and removes the forced conversion of Dates to UTC Strings. An opt-out fallback to forced coercion to UTC is included, which was modeled after loopbackio#265 from @darknos . Old, squashed commits: d0ea1d9 Legacy UTC date processing fallback credit: @darknos a59dad7 Remove orphaned string functions e8fdbdc Incorporate @darknos expanded check for zero dates abd4e0a Remove DATE manipulations in from/toColumnValue
Closing this PR since #271 has been landed. Feel free to reopen if needed. |
new connector option
legacyDateTypeProcessing
defaulttrue
. set tofalse
to use new behaviour as discussed in #257 and #149