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

Clean merge of fixed mysql datetime handling #440

Closed
wants to merge 1 commit into from

Conversation

koenpunt
Copy link
Collaborator

Rebased version of #317

Repository owner locked and limited conversation to collaborators Jul 20, 2014
Repository owner unlocked this conversation Jul 20, 2014
This was referenced Jul 20, 2014
@koenpunt koenpunt force-pushed the andyleap-datetime branch 5 times, most recently from bdf04cc to e3abc3a Compare December 5, 2014 10:50
@koenpunt
Copy link
Collaborator Author

koenpunt commented Dec 5, 2014

@jpfuentes2 @al-the-x @Rican7 @greut @tuupola the changes provided here (and previously in #317) seem obsolete, as all the tests pass, without change of code. Any thoughts?

@Rican7
Copy link
Collaborator

Rican7 commented Dec 7, 2014

There are so many commits from so many authors with over 80 files and over 3400 lines changed here, with a lot of the changes seemingly just formatting.

Reviewing this is nearly impossible.

@koenpunt
Copy link
Collaborator Author

koenpunt commented Dec 7, 2014

Rebased, should be readable now

@koenpunt
Copy link
Collaborator Author

koenpunt commented Dec 7, 2014

I've left out the commits with the patch for now, to show that tests are not failing without them,

@Rican7
Copy link
Collaborator

Rican7 commented Dec 7, 2014

Hmm, yea, weird that the tests still pass.

@koenpunt koenpunt closed this Jan 18, 2015
@koenpunt koenpunt deleted the andyleap-datetime branch January 18, 2015 00:09
@Rican7
Copy link
Collaborator

Rican7 commented Jan 23, 2015

@koenpunt I think I know why these tests were passing. I believe that Travis-CI changed their mysql configuration, so that sql_mode now allows invalid dates.

We should add mysql -e "SET GLOBAL sql_mode = 'NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES'" to the Travis pre-scripts to make sure that we're testing against strict configurations.

@Rican7
Copy link
Collaborator

Rican7 commented Jan 23, 2015

💥 ❗

So, yea, like I thought, the Travis MySQL environment changed recently which hid the bug away.

Turning on a strict sql_mode again shows the error as still existing:
https://travis-ci.org/jpfuentes2/php-activerecord/jobs/48106167#L186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants