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

Better Datetime handling #317

Closed
wants to merge 1 commit into from
Closed

Better Datetime handling #317

wants to merge 1 commit into from

Conversation

andyleap
Copy link

@andyleap andyleap commented May 1, 2013

This is a series of fixes, that includes handling mysql datetimes better and improving datetime round-tripping. Also included is code to allow columns to cast based on whether value is coming from db or from user code, so that users can use the same format for any adapter(wasn't an issue, but may be with some DB's)

@al-the-x
Copy link
Collaborator

The duplicate try-catch blocks in Column.php and Connection.php smell funny to me, particularly since they're catching naked \Exception... Couldn't that logic go live in \ActiveRecord\DateTime as a factory that returned the instance or NULL depending on a more specific Exception...?

@al-the-x
Copy link
Collaborator

This makes sense for 1.2 (next release), since it's a better fix than the current hackery. ;)

@VendanAndrews
Copy link
Contributor

Most likely, I was just starting to get frustrated by the exception handling, as namespaces are a little new to me. I was really just trying to fix the issue that MySQL doesn't handle timezones at all. Realistically, it's still an issue, as if 2 different php scripts hit the same table, with different default TZ, they will get different times, but that's a little outside the scope of php-activerecord

@al-the-x
Copy link
Collaborator

@VendanAndrews Agreed. We can't ensure data integrity across those kind of boundaries when MySQL doesn't help us any. Use PostgreSQL. O_O

@al-the-x
Copy link
Collaborator

@andyleap @koenpunt Can you guys sort out if #311 is superseded by this PR or what?

@andyleap
Copy link
Author

andyleap commented Aug 2, 2013

IMHO, yes, #311 is superseded by this.

@koenpunt
Copy link
Collaborator

Closing in favor of #440

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

Successfully merging this pull request may close these issues.

4 participants