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

Bugfix - Explicit Time Zone assignment regression from #532 #539

Merged
merged 7 commits into from
May 20, 2016

Conversation

Rican7
Copy link
Collaborator

@Rican7 Rican7 commented May 19, 2016

So PR #533 was fantastic in fixing an issue regarding passing in an explicit time-zone during column casting and attribute assigning. Unfortunately, the merge of #532 caused a regression as it overwrote the fix made in #533. 😞

This PR fixes that regression.

The included test makes sure the behavior is correct, and the following snippet demonstrates the issue at hand:

screen shot 2016-05-19 at 12 05 55 pm

@Rican7
Copy link
Collaborator Author

Rican7 commented May 19, 2016

Took a few tries to catch all the places where we were "doing it wrong", but it's good now. Thank god for tests...

/cc: @koenpunt @jpfuentes2

*
* @var string
*/
const DATETIME_TRANSLATE_FORMAT = 'Y-m-d\TH:i:s';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the extra T Here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just the RFC 3339 format without the timezone identifier. I figured it was the most easily (standardized, spec-based) identifiable format. It doesn't really matter, though, as it's just for internal translations (not for storage, etc).

@jpfuentes2
Copy link
Owner

Let's move discussion to upgrading php-unit to #540.

This is why we write tests. Excellent test coverage here, @Rican7 :)

@jpfuentes2 jpfuentes2 merged commit a9349ea into jpfuentes2:master May 20, 2016
@Rican7
Copy link
Collaborator Author

Rican7 commented May 20, 2016

Thanks @jpfuentes2. Tests FTW.

@Rican7 Rican7 deleted the bugfix/timezone-regression branch May 20, 2016 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants