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

Support for microseconds #246

Merged
merged 1 commit into from Feb 6, 2017

Conversation

4 participants
@hubipe
Contributor

hubipe commented Jan 12, 2017

  • bug fix? no
  • new feature? yes
  • BC break? maybe

Database systems support microseconds in datetime columns, but Dibi always escaped datetime values without second fractions. Therefore even if the datetime column in database is set to accept second fractions, it would always store just full second. I changed formatting rules to include second fractions with date format u (supported since PHP 5.2.2).

If database system is not set to use second fractions, it should beignored, however I'm not sure, if other databases than MySQL, MSSQL and Postgres support time fractions, so I've changed just these drivers.

@dg

This comment has been minimized.

Owner

dg commented Jan 18, 2017

Thanks. Can you fix tests too?

@milo

This comment has been minimized.

Contributor

milo commented Jan 18, 2017

This can be BC break. In PostgreSQL you can define column as timestamp (no limitation) or timestamp(0) (it trims second fragments). After this change, the timestamp column will be filled with microseconds too which wasn't before.

Personally, I'm 👍 for this.

@hubipe

This comment has been minimized.

Contributor

hubipe commented Feb 1, 2017

Sorry, I've been busy. I'll do my best and will try to update tests by this weekend.

@hubipe

This comment has been minimized.

Contributor

hubipe commented Feb 5, 2017

I've updated the tests

@dg

This comment has been minimized.

Owner

dg commented Feb 6, 2017

Thanks

@dg dg merged commit 67bc7cc into dg:master Feb 6, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 69.099%
Details

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 9, 2017

dg added a commit that referenced this pull request Jun 10, 2017

dg added a commit that referenced this pull request Jun 10, 2017

dg added a commit that referenced this pull request Jun 10, 2017

@Ciki

This comment has been minimized.

Contributor

Ciki commented Feb 23, 2018

If database system is not set to use second fractions, it should beignored, however I'm not sure, if other databases than MySQL, MSSQL and Postgres support time fractions, so I've changed just these drivers

@hubipe can you give a source for this statement? I've come across this issue as it's BC BREAK in my opinion .. when the datetime column stores only full seconds, then any comparison using also fractions will fail.. Am I missing sth.?

cc @dg

@dg

This comment has been minimized.

Owner

dg commented Feb 25, 2018

In MySQL 5.7.20 comparsion of datetime column with full seconds and fraction seems to work ok.

@hubipe

This comment has been minimized.

Contributor

hubipe commented Feb 26, 2018

Same in PostgreSQL 10.1 and MSSQL 2008r2.
What comparison are you using? And what database system?

@Ciki

This comment has been minimized.

Contributor

Ciki commented Feb 27, 2018

check MySQL 5.6 version, example here

tested on 5.6.17 and also MariaDB 10.1.26

@hubipe

This comment has been minimized.

Contributor

hubipe commented Feb 27, 2018

I see what are you referring to. In my opinion this is expected behaviour. Compare it with this modified example. When you compare date time column with 0 microseconds it is automatically presumed, that microseconds are 0. Thus comparing with microsecond time with any different microseconds than 0 will result in empty result.

In this way, that PR might be considered as a BC break, if you had depended on this falsy behavior. The question is, if it should stay as it is and you should fix your code not to use microseconds, or if dibi should make a change

@Ciki

This comment has been minimized.

Contributor

Ciki commented Feb 28, 2018

Yes, I agree.

$dt = new DateTime(); // https://secure.php.net/manual/en/datetime.construct.php for php >=7.1 microseconds are filled with actual value. Not with '00000'.
dibi::insert(table, [date=>$dt]); // inserts record without the microseconds
dibi::select(*)->from(table)->where('date=%dt', $dt); // empty result

this example and similar use cases worked prior this PR regardless the php version used. With this PR they work seamlessly with php <=7.1 only.

@hubipe

This comment has been minimized.

Contributor

hubipe commented Feb 28, 2018

What shall we do then? We can either withdraw this PR and think of different solution (maybe new keyword %tm?) to support microseconds, or leave it as it is and mark it as backward incompatibility. @dg?

(Arrghh I hate PHP inconsistency!)

@Ciki

This comment has been minimized.

Contributor

Ciki commented Apr 18, 2018

@dg do you find it correct|expected|safe behaviour regarding BC as well?

@dg

This comment has been minimized.

Owner

dg commented Apr 18, 2018

Correct me if I'm wrong, this affects only cases when you create new DateTime without arguments in PHP >= 7.1, right?

I think the easiest solution would be keep this PR, add notice to older releases and add helper function that strips microseconds from DateTime.

Something like:

$stripped = dibi::stripMicroseconds(new DateTime);  // or stripMicrotime?
@Ciki

This comment has been minimized.

Contributor

Ciki commented Apr 18, 2018

yes, you are correct.

as long as there is a notice about this change in docs I agree with your proposal in all points

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