Skip to content

Conversation

@anyday
Copy link

@anyday anyday commented Nov 18, 2014

An unset Timestamp in database would sometimes cause Carbon to fail:
InvalidArgumentException in Carbon.php line 385: Unexpected data found.

Proposed changes fix issue.

An unset Timestamp in database would sometimes cause Carbon to fail:
InvalidArgumentException in Carbon.php line 385: Unexpected data found.

Proposed changes fix issue.
@lagbox
Copy link
Contributor

lagbox commented Nov 18, 2014

interesting

@anyday anyday changed the title [Bugfix] Fixed Carbon Fatal Error [4.2] Fixed Carbon InvalidArgumentException When Using toArray or toJson On Unset Timestamps Nov 19, 2014
@anyday
Copy link
Author

anyday commented Nov 19, 2014

Just to be clear, the issue crops up when a Timestamp field in the database has it's default value of 0000-00-00 00:00:00. (due to an insert such as DB::raw() )

@GrahamCampbell
Copy link
Collaborator

Could you add a test please.

Added nulled mysql timestamp
@anyday
Copy link
Author

anyday commented Dec 3, 2014

Added a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove all trailing whitespace.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@GrahamCampbell
Copy link
Collaborator

1) DatabaseEloquentModelTest::testToArrayIncludesNulledMysqlTimestamps
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'0000-00-00 00:00:00'
+'-0001-11-30 00:00:00'

@GrahamCampbell
Copy link
Collaborator

This test is still failing.

@anyday
Copy link
Author

anyday commented Dec 3, 2014

A bit perplexed, because the change I made to the original code wouldn't set it negative -- so I imagine my test will fail without my change to the core code as well.

I'll need to figure out why that's happening.

I'm fairly new to Laravel so not sure exactly where to look.

Fixed values to be correct according to default mysql timestamps
@anyday
Copy link
Author

anyday commented Dec 3, 2014

That negative you're getting is what I get with the old code.

I did have an error in the test because of copy /paste syndrome, but that has been corrected.

@anyday
Copy link
Author

anyday commented Dec 3, 2014

The test is failing because it isn't testing my changes made in
src/Illuminate/Database/Eloquent/Model.php

@GrahamCampbell
Copy link
Collaborator

Please rebase, and squash, if the tests are still failing, then it's 100% caused by your changes.

@taylorotwell
Copy link
Member

I don't really agree with this change. If it's a date give it a valid date. I don't think it should just be missing from the resulting array.

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