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

DateTime Precision is lost when adding date to model factories #24214

Closed
jamiehd opened this issue May 14, 2018 · 27 comments
Closed

DateTime Precision is lost when adding date to model factories #24214

jamiehd opened this issue May 14, 2018 · 27 comments

Comments

@jamiehd
Copy link
Contributor

jamiehd commented May 14, 2018

  • Laravel Version: v5.6.17
  • PHP Version: 7.1.17

Description:

When using a model factory with a fixed date the microseconds are lost. This means when asserting in a unit test dates do not match when they should.

Steps To Reproduce:

public function some_test() {
        Carbon::setTestNow(new Carbon());
        $t = factory(Model::class)->make(['date_field' => Carbon::now()->addWeeks(2)]);
        $this->assertEquals($t->date_field, Carbon::now()->addWeeks(2));
}

Failed asserting that two DateTime objects are equal.
--- Expected
+++ Actual
@@ @@
-2018-05-28T21:53:00.000000+0000
+2018-05-28T21:53:00.613965+0000

@laurencei
Copy link
Contributor

laurencei commented May 15, 2018

When using a model factory with a fixed date the microseconds are lost.

This is expected behavior though, isnt it? A database timestamp does not have microseconds?

This means when asserting in a unit test dates do not match when they should.

Your assertEquals() would still fail - because 0.00003 microseconds has past between the two line items, so they would never be identical in that way?

@sisve
Copy link
Contributor

sisve commented May 15, 2018

@laurencei We support specifying fractional seconds in datetime fields since #18847. The SQL standard uses 6 digits for this, per default, while mysql uses 0.

Note that the test calls Carbon::setTestNow(...). This means that further calls to Carbon::now() in the test will return a specific instance, and there will be no microsecond difference in the values.

@laurencei
Copy link
Contributor

@sisve - yep, your right.

The issue seems to be someone due to the Carbon change in v1.22.

v1.21 works correctly - I can make the test pass. Switching to v1.22 causing the situation above.

I found this: briannesbitt/Carbon#1234

I had a quick look and I cant work out why it's doing it though - as both use Carbon under the hood. I'll have a play later on if I get time.

@sisve
Copy link
Contributor

sisve commented May 15, 2018

Carbon 1.22 was severely broken in many ways. Stay away from it. briannesbitt/Carbon#863

@laurencei
Copy link
Contributor

laurencei commented May 15, 2018

@sisve - yes, but it's not a break - it's actually an improvement in percission due to a PHP7 bug.

The issue flows all the way forward to the latest version, and was an intended change.

The strange thing is why it's dropping percission when going through the factory - I dont see an obvious reason why - so I'm going to dig in further.

@staudenmeir
Copy link
Contributor

The microseconds are removed by HasAttributes::setAttribute(), which uses the database connection's default date format. With the exception of SQL Server, this is always Y-m-d H:i:s.

You can specify a custom format in your model:

protected $dateFormat = 'Y-m-d H:i:s.u';

@tillkruss
Copy link
Collaborator

Is this a Carbon or Laravel issue?

@laurencei
Copy link
Contributor

laurencei commented May 15, 2018

@tillkruss - it's an intendend change in Carbon behaviour, and so that's going to make it Laravel's problem on the way its implemented...

I dont see an obvious fix though, because it would have to be a change in default behaviour in Laravel - which we might not want.

I'd leave this open and see if someone has a good solution for it...

@sisve
Copy link
Contributor

sisve commented May 15, 2018

I do not see how this is related to the linked Carbon issue. They are talking about how the some calls to static methods now have a microsecond value; but that is the expected behavior in this issue. Here the problem is that the microseconds disappears.

I can reproduce this using the mysql grammar, so this does not depend on a mssql installation. I got stuck on reproducing this until I realized I had to add the field to the $dates array. This means that HasAttributes::setAttribute will convert the provided value into a string using the current query grammars; and no query grammars support microsecond precision.

// If an attribute is listed as a "date", we'll convert it from a DateTime
// instance into a form proper for storage on the database tables using
// the connection grammar's date format. We will auto set the values.
elseif ($value && $this->isDateAttribute($key)) {
$value = $this->fromDateTime($value);
}

class MyModel extends Model {
    protected $dates = [ 'date_field' ];
}

$value = Carbon::now();
$model = new MyModel();
$model->date_field = $value;
dd(array(
    'before' => $value,
    'after' => $model->date_field
));

// array:2 [▼
//     "before" => Carbon {#3097 ▼
//         +"date": "2018-05-15 17:15:48.768878"
//         +"timezone_type": 3
//         +"timezone": "UTC"
//     }
//     "after" => Carbon {#3117 ▼
//         +"date": "2018-05-15 17:15:48.000000"
//         +"timezone_type": 3
//         +"timezone": "UTC"
//     }
// ]

@laurencei
Copy link
Contributor

laurencei commented May 15, 2018

I do not see how this is related to the linked Carbon issue. They are talking about how the some calls to static methods now have a microsecond value; but that is the expected behavior in this issue. Here the problem is that the microseconds disappears.

The problem is we have always been dropping the microseconds. But it was never noticed or an issue, because Carbon was also dropping the microseconds.

Now Carbon has changed behaviour and passes them through - highlighting the issue.

It's not a bug, - its a change in behaviour by Carbon - and now it affects Laravel downstream.

@laurencei
Copy link
Contributor

Technically we are no worse off than before. We've always dropped the microseconds - its not a new bug.

The issue is Carbon tests will just "highlight" the issue now.

So it would seem like something worth targeting for 5.7 to change and incorporate microseconds (if at all).

@staudenmeir
Copy link
Contributor

It looks like we can't really support microseconds on MySQL at the moment.

Saving works, but fetching doesn't: There is a four-year-old bug that removes microseconds when using PDO::ATTR_EMULATE_PREPARES => false (which Laravel does by default).

@tillkruss
Copy link
Collaborator

Can this be closed then?

@antonkomarev
Copy link
Contributor

Thats the only reason why I've started to use integers when I need to store datetime with microseconds.

$model->setAttribute('cursor', Carbon::now()->format('Uu'));

@antonkomarev
Copy link
Contributor

It will be great if everybody who experienced this issue will vote in PHP Bug №67122. It doesn't require registration and possibly will add importance for this bug.

@laurencei
Copy link
Contributor

@tillkruss - i think it needs to stay open as it's going to come up as people upgrade their apps and run into this issue...

And leaving it open might give other people a chance to come up with a creative solution...

@antonkomarev
Copy link
Contributor

antonkomarev commented May 16, 2018

@laurencei I've found possible solution of this issue and posted it to the PHP bug tracker, but sadly I don't have an access to propose these changes directly to the PHP sources.

@antonkomarev
Copy link
Contributor

Here is a PR which should fix this issue: php/php-src#3245

@sisve
Copy link
Contributor

sisve commented May 21, 2018

@a-komarev That PR may fix some related problems, but that PR alone will not fix this issue. Laravel still formats date fields using the query grammar's date format, and it doesn't support milli-/microseconds.

@antonkomarev
Copy link
Contributor

There is a lot of progress with PHP bugfix: php/php-src#3257
It could be released in PHP 7.3 alpha versions.

@strongholdmedia
Copy link

strongholdmedia commented Jun 14, 2018

@sisve I am not totally sure of that being true.
I am uncertain about the factory helper stuff, but we definitely use Laravel/Eloquent with timestamps and Carbon and it definitely produced an issue exactly like this one in appearance until we turned PDO::ATTR_EMULATE_PREPARES on.

@sisve
Copy link
Contributor

sisve commented Jun 15, 2018

@strongholdmedia Look at my reproduction just a few posts up. It doesn't do database calls at all, just creates an Eloquent mode, set's a date field and reads it back again. #24214 (comment)

@strongholdmedia
Copy link

Ah, yes, now I remember.
There is also an issue with Carbon, IIRC.
Something along the "format specifier for microseconds does not work like Laravel uses it" line.

@gitBC
Copy link

gitBC commented Aug 30, 2018

Would love to have full support of fractional seconds as well, this would include ANY date/time and timestamp methods. My use case is to fetch by created_at where there are several records a second added. Something along these lines:

public function mostRecentEvent()
    {
        return Event::all()
            ->orderBy('occurred_at', 'desc')
            ->limit(1)->first();
    }

I've tried a few permutations of modifying the $cast and $dates and creating custom mutators and even overriding the Model::asDateTime() functions to no avail

@driesvints
Copy link
Member

Seems like the php/php-src#3257 was merged for the 7.3 release. If anyone can verify if this is fixed for the latest 7.3 RC that'd be great.

I'm going to close this because this seems to be related to PHP rather than Laravel. People can still find this issue in the tracker.

@staudenmeir
Copy link
Contributor

@driesvints Yes, this is working in PHP 7.3.

@strongholdmedia
Copy link

Glad it finally turned out to be the same issue.
Whoever can not wait for the final 7.3 can turn on emulated prepares.
(Just remember to turn it (back) off afterwards.)

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

No branches or pull requests

9 participants