Fix #74243: pdo_dblib option to get milliseconds #2859

fandrieu · 2017-10-17T09:18:26Z

This is an attempt to fix bug #74243, by providing an option to add milliseconds to the date format.

Date formating is still based on dbdatecrack + printf, but the millisecond value returned by the driver is appended to the list of arguments.
The option appends ".%3d" to the printf format. (which is now a variable stored with the db handle)

With the solution we can't specify a custom date format, but it at least provides a way to get all the data and doesn't significantly change the date handling code. The format is still compatible with "new DateTime()".

adambaratz · 2017-10-17T18:28:30Z

Why not add an option to let people pass in a format string? A general solution seems like it would be better. There's a ticket from others asking for a different kind of tweak.

This adds a new attribute PDO::DBLIB_ATTR_MILLISECOND that changes the date format to include milliseconds. Date formating is still based on dbdatecrack + printf, but the millisecond value returned by the driver is appended to the list of arguments. The option appends ".%3d" to the printf format, which is now stored with the db handle.

This adds a new attribute PDO::DBLIB_ATTR_MILLISECOND that changes the date format to include milliseconds. The string format now includes milliseconds, if the option is not present the resulting string is truncated to remove them.

Switch to slprintf as we're dealing with fixed length data

Rename test file

This option controls how datetime data is returned. It has multiple purposes: - "raw" = converted by dblib - "" (default) = former behavior = ISO - "millisecond" = same + millisecond - "object" = same + wrapped in DateTime object - anything else = same + DateTime::format(datetime_format)

fandrieu · 2017-10-18T12:55:21Z

I was aiming for the simpler less intrusive answer here, in my opinion that's enough fo fix most problems (as the string returned is compatible with DateTime, other conversions can be handled in php).

But I agree with the ticket you mention, having the underlying driver handle the formatting is also an elegant solution and should be an option.

Anyway, for the sake of conversation, I just pushed a patch on the other and of the spectrum: an overkill solution with a "datetime_format" string option that implements several behaviors:

"raw" = converted by dblib (Fix #72141)
"" (default) = former behavior = ISO (backward compatible)
"millisecond" = same + milliseconds (Fix #74243)
"object" = same + wrapped in DateTime object (could be done in php)
anything else = same + DateTime::format(datetime_format) (could be done in php)

adambaratz · 2017-10-18T18:57:27Z

Before you get too deep into implementing this, let's talk through the spec and make sure we agree on that.

I don't like the idea of setting an attribute to either format strings or sentinel values. That kind of design is hard to future-proof, which is very important with libraries that someone will want to update eventually.

I'm really wary of letting this extension return DateTime objects. I don't know much about that (pretty pesky) world. I don't want create potentially unreliable code that could take a while to stabilize, would be hard to future-proof.

My suggestion would be to add an attribute that lets you set a format string. When unset, the existing behavior would be used. I don't know what control characters should be used. If we imitate the date function, that would make things awkward for the people who want to use the values from locales.conf. You could contact the people on that ticket if you want to get some more opinions.

fandrieu · 2017-10-19T08:47:21Z

Thanks for taking the time to look into this.

I too don't like the idea of a string attribute and introducting php_date into the mix, dbconvert already provides a way to specify a custom format in locales.conf and that's what #72141 is all about.

Also the current behavior must be kept and I think it has its uses (it ensures a consistent output format, compatible with strtotime & date_create), it just lacks milliseconds as reported in #74243.

So this last commit is a more sensible solution, the attribute is now an integer with 3 possible values, to keep the current behavior and add these 2 new options:

DBLIB_DATETIME_FORMAT_DEFAULT = converted to YYYY-MM-DD hh:mm:ss
DBLIB_DATETIME_FORMAT_MILLISECOND = converted to YYYY-MM-DD hh:mm:ss.sss
DBLIB_DATETIME_FORMAT_CONVERT = converted by dblib (freetds uses locales.conf "date format")

This adds a DBLIB_ATTR_DATETIME_FORMAT attribute to control how datetime data is converted to string. It implements 3 behaviors: - DBLIB_DATETIME_FORMAT_DEFAULT = converted to YYYY-MM-DD hh:mm:ss - DBLIB_DATETIME_FORMAT_MILLISECOND = converted to YYYY-MM-DD hh:mm:ss.sss - DBLIB_DATETIME_FORMAT_CONVERT = converted by dblib

krakjoe · 2017-10-19T10:46:27Z

@adambaratz can we have a re-read of the latest patch and a thumbs up or down for this please ?

adambaratz · 2017-10-19T17:24:40Z

I don't like the idea of having a second format built in (DBLIB_DATETIME_FORMAT_MILLISECOND). I think it would encourage people to submit patches to add presets for any variation they might want. Could we replace that option with a generic one that lets you pass in a format string? It would be even better if we could scrap that and require everyone to funnel format strings through locales.conf, but I could understand that not every person using pdo_dblib would be able to edit that file.

This adds an attribute to convert datetime data using dblib's dbconvert.

fandrieu · 2017-10-23T11:58:42Z

I see your point in having a second harcoded format, it should either be scraped or replaced with a customizable format.
This can't be done using the current printf approach, but we could use strftime instead, providing a php native alternative to locales.conf.
(one thing though: FreeTDS has a strftime hack to enable milliseconds via %z, this should be ported over...)

So here's some more "pick and choose" commits with that in mind, now with two different options:

PDO::DBLIB_ATTR_DATETIME_CONVERT (bool): converted by dblib (like mssql.datetime_convert, for #72141)
PDO::DBLIB_ATTR_DATETIME_FORMAT (string): converted by php using strftime + FreeTDS ms hack (for #74243)

The first option alone should be enough to satisfy most users, but the second is driver independent and doesn't require access to locales.conf.

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using strftime

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using strftime, with FreeTDS %z strftime hack.

adambaratz · 2017-10-24T17:54:11Z

ext/pdo_dblib/dblib_driver.c

 	H->stringify_uniqueidentifier = 0;
 	H->skip_empty_rowsets = 0;
+	H->datetime_convert = 0;
+	H->datetime_format = zend_string_init("", 0, 0);


Default to NULL so you don't do a needless alloc.

OK, datetime_format is now NULL unless an non-zero-length string is passed

adambaratz · 2017-10-24T17:57:23Z

ext/pdo_dblib/dblib_stmt.c


+static int pdo_dblib_strftime(zval *output, zend_string *format, DBDATEREC *dr)
+{
+	/* mashup of php_date.c php_strftime & FreeTDS convert.c tds_strftime */


How would you feel about leaning on ext/date/php_date.c:php_format_date instead? It's designed to be accessible across extensions. The format strings are different, but it'd provide the same functionality (it's what powers the date function).

That's how I first tried to implement this, but there were a few drawbacks:

milliseconds are only supported when using the DateTime object interface (like in this early commit)

it introduces an unneeded back and forth conversion to unix timestamp (need to call php_mktime before php_format_date)

adambaratz · 2017-10-24T17:57:59Z

ext/pdo_dblib/dblib_driver.c

 		H->stringify_uniqueidentifier = pdo_attr_lval(driver_options, PDO_DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER, 0);
 		H->skip_empty_rowsets = pdo_attr_lval(driver_options, PDO_DBLIB_ATTR_SKIP_EMPTY_ROWSETS, 0);
+		H->datetime_convert = pdo_attr_lval(driver_options, PDO_DBLIB_ATTR_DATETIME_CONVERT, 0);
+		H->datetime_format = pdo_attr_strval(driver_options, PDO_DBLIB_ATTR_DATETIME_FORMAT, 0);


Use NULL instead of 0 for the default value.

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using date(), with "u"/"f" format hack to support micro/milli seconds

fandrieu · 2017-10-25T16:12:33Z

So here's a more php-y DATETIME_FORMAT implementation, based on php_format_date :)

I actually preferred this one and have now figured out a decent way of putting it together:

convert to timestamp with a single dbconvert call, using the bigdatetime format
hack milli (and micro) second support in date(), using the "inject in format" approach from FreeTDS

Convert to SYBMSDATETIME2 to get timestamp: - works with FreeTDS < 1 - better precision

fandrieu · 2017-10-26T12:58:34Z

I fixed this last commit:

use DATETIME2 as intermediary format: it works with FreeTDS<1 and offers better precision
support escaping in date() format: I missed that

adambaratz · 2017-10-30T20:33:39Z

I know I suggested going in this direction, but I'm getting uncomfortable with the amount of (partially copied, pasted, and rearranged) code involved here. It feels like more business logic than should be living in a driver layer. I'm willing to be okay with it if this is the only way to make it work, but... I'm wondering how true that is.

The CAST function has some date/time options. If we only add the DBLIB_DATETIME_FORMAT_CONVERT attribute and ask everyone else to CAST as needed (or use other functions), will there be any use cases left out? There are a few options available through CAST for getting the milliseconds out.

fandrieu · 2017-10-31T08:41:50Z

I agree, DATETIME_CONVERT is enough and the code I came up for DATETIME_FORMAT is too complicated and doesn't add much (basically an option for people without access to locales.conf).

Anyway I'm OK with scratching this last part and focusing on DATETIME_CONVERT, I pushed a commit doing that.

One last thing though: I'd like to include SQLDATETIME2 in the switch case (like in this commit), to get a consistent behavior for datetime & datetime2 (as of now datetime2 isn't supported and falls back to the default behavior)

This adds an attribute to convert datetime data using dblib's dbconvert.

php-pulls · 2017-10-31T22:04:11Z

Comment on behalf of adambaratz at php.net:

Committed b72af30.

adambaratz · 2017-10-31T22:06:46Z

This change should be all set. Thanks for all your work on it. I'll take a look at merging the SQLDATETIME2 change separately.

fandrieu · 2017-11-03T17:59:43Z

Thanks for working on this, glad this change made it.

Just a thought: the allocation for the generic dbconvert may not be enough.
tmp_data_len = 32 + (2 * (data_len)) is 48 for datetime and won't accomodate the 63 chars FreeTDS allows for formated dates. (such long date strings may never happen, just wanted to mention it)

adambaratz · 2017-11-06T18:49:56Z

Where did you get the 63 for the length? The closest thing I could find in the FreeTDS source was types.csv, which had very different values.

fandrieu · 2017-11-07T08:23:48Z

I did some tests with a large date format and found that the resulting string is truncated to 63 chars.
You can see it in the code here.

With a large enough buffer, datetime_convert.phpt would output something like:
string(63) "Nov 7 2017 09:19:34:570AM PAD_PAD_PAD_PAD_PAD_PAD_PAD_PAD_PAD_"
(without you could get a Termsig=11)

fandrieu · 2017-11-07T08:51:37Z

PS: this test can prevent the bug, it could for example be implemented here like this.

fandrieu added 4 commits October 18, 2017 11:40

Fix #74243: pdo_dblib option to get milliseconds

3f91202

This adds a new attribute PDO::DBLIB_ATTR_MILLISECOND that changes the date format to include milliseconds. The string format now includes milliseconds, if the option is not present the resulting string is truncated to remove them.

Fix #74243: pdo_dblib option to get milliseconds

987aa75

Switch to slprintf as we're dealing with fixed length data

Fix #74243: pdo_dblib option to get milliseconds

f98f8ec

Rename test file

fandrieu force-pushed the bug-74243 branch from 3940b41 to f98f8ec Compare October 18, 2017 09:53

fandrieu force-pushed the bug-74243 branch 3 times, most recently from bb682e0 to d9c5afc Compare October 19, 2017 08:53

fandrieu force-pushed the bug-74243 branch from d9c5afc to 4618f03 Compare October 19, 2017 08:56

krakjoe added Bug Waiting on Review labels Oct 19, 2017

fandrieu added 2 commits October 23, 2017 11:31

Fix #74243 & #72141: revert to master

8d8df1d

Fix #72141: pdo_dblib datetime_convert attr

9fca0a6

This adds an attribute to convert datetime data using dblib's dbconvert.

fandrieu force-pushed the bug-74243 branch 2 times, most recently from 5a9e34d to 41124ce Compare October 24, 2017 15:20

fandrieu added 2 commits October 24, 2017 17:41

Fix #74243 & #72141: pdo_dblib datetime attrs

c359ecb

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using strftime

Fix #74243 & #72141: pdo_dblib datetime attrs

8532620

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using strftime, with FreeTDS %z strftime hack.

fandrieu force-pushed the bug-74243 branch from 41124ce to 8532620 Compare October 24, 2017 15:41

adambaratz reviewed Oct 24, 2017

View reviewed changes

Fix #74243 & #72141: pdo_dblib datetime attrs

e821587

This adds two attributes to convert datetime data: - DBLIB_DATETIME_FORMAT_CONVERT: converted by dblib - DBLIB_DATETIME_FORMAT_FORMAT: converted by php using date(), with "u"/"f" format hack to support micro/milli seconds

fandrieu added 2 commits October 26, 2017 14:50

Fix #74243 & #72141: datetime2 temp format

efcfeb7

Convert to SYBMSDATETIME2 to get timestamp: - works with FreeTDS < 1 - better precision

Fix #74243 & #72141: support format escaping

e89d35e

Fix #74243 & #72141: fix comment

fbfae44

fandrieu force-pushed the bug-74243 branch 5 times, most recently from 46a772b to 1322cf1 Compare October 27, 2017 07:59

Fix #74243 & #72141: integer computations

210ec04

fandrieu force-pushed the bug-74243 branch from 1322cf1 to 210ec04 Compare October 27, 2017 08:02

Fix #74243 & #72141: rewrite u/f hack

6ec7a7f

fandrieu force-pushed the bug-74243 branch from b9f8dc4 to 6ec7a7f Compare October 27, 2017 09:35

Fix #74243 & #72141: fix u/f rounding overflow

a8917f8

Fix #72141: pdo_dblib datetime_convert attr

8888892

This adds an attribute to convert datetime data using dblib's dbconvert.

fandrieu force-pushed the bug-74243 branch from 0e1da4b to 8888892 Compare October 31, 2017 08:51

php-pulls closed this Oct 31, 2017

php-pulls removed the Waiting on Review label Oct 31, 2017

adambaratz mentioned this pull request Nov 6, 2017

Treat DATETIME2 columns like DATETIME #2907

Closed

adambaratz mentioned this pull request Nov 7, 2017

More robust handling of stringified column data #2909

Closed

Fix #74243: pdo_dblib option to get milliseconds #2859

Fix #74243: pdo_dblib option to get milliseconds #2859

Uh oh!

Conversation

fandrieu commented Oct 17, 2017

Uh oh!

adambaratz commented Oct 17, 2017

Uh oh!

fandrieu commented Oct 18, 2017

Uh oh!

adambaratz commented Oct 18, 2017

Uh oh!

fandrieu commented Oct 19, 2017

Uh oh!

krakjoe commented Oct 19, 2017

Uh oh!

adambaratz commented Oct 19, 2017

Uh oh!

fandrieu commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adambaratz Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

fandrieu Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

adambaratz Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

fandrieu Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

adambaratz Oct 24, 2017

Choose a reason for hiding this comment

Uh oh!

fandrieu commented Oct 25, 2017

Uh oh!

fandrieu commented Oct 26, 2017

Uh oh!

adambaratz commented Oct 30, 2017

Uh oh!

fandrieu commented Oct 31, 2017

Uh oh!

php-pulls commented Oct 31, 2017

Uh oh!

adambaratz commented Oct 31, 2017

Uh oh!

fandrieu commented Nov 3, 2017

Uh oh!

adambaratz commented Nov 6, 2017

Uh oh!

fandrieu commented Nov 7, 2017

Uh oh!

fandrieu commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fandrieu commented Oct 23, 2017 •

edited

Loading