PDO_DBlib: stringify 'uniqueidentifier' fields by turboezh · Pull Request #2001 · php/php-src

turboezh · 2016-07-15T07:11:15Z

Before PHP 7.0 pdo_dblib returns uniqueidentifier fields as 36-byte hex string, a 'human' representation of GUID, like, '82A88958-672B-4C22-842F-216E2B88E72A'.
Now such fields returns as 16-byte raw binary string.

I propose keep old behavior and return fields in human representation when PDO::ATTR_STRINGIFY_FETCHES == true.

https://bugs.php.net/bug.php?id=72601

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE

…niqidentifier

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE Tests added.

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE Tests fix.

…niqidentifier

weltling · 2016-07-19T05:16:22Z

@adambaratz could you please check this PR?

Thanks.

…niqidentifier

adambaratz · 2016-07-26T22:58:56Z

ext/pdo_dblib/tests/types.phpt

 bool(true)
 bool(true)
+bool(true)
+bool(true)


This fails for me. The column type comes back as SQLBINARY (45), so it's handled differently.

SQLUNIQUE (SYBUNIQUE) is available only since TDS 7.0. =( Maybe I'm wrong, I can't test it with all variants of databases. But when I set tds version 4.6 it really comes as SQLBINARY (45), tds version 6.0+ gives SQLUNIQUE (36).
Must the extension behave identically at all tds versions? I think 4.* versions should be left off as unsupported by this option. If so I will try to fix the test accordingly.

Ah, my test VM had the older version set. Looks fine now. I think ideally we would skip this test for older TDS versions, but I don't think we can detect that from PHP/PDO. Maybe just add a comment saying why this test might fail?

There are PDO attributes that are not implemented in pdo_dblib now: PDO::ATTR_SERVER_INFO, PDO::ATTR_SERVER_VERSION, PDO::ATTR_CLIENT_VERSION. They could be used to reveal some info about connection including TDS protocol version. But I think it is a task for a separate PR. I'll put a comment about possible fail in the test now with a TODO about these attrs.

Sounds great.

adambaratz · 2016-07-26T23:01:10Z

I'd recommend this behavior be opt-in. I made this (breaking) change in the interest of making this extension work the same was as the (deprecated) mssql extension. So, my fault. But we should allow both behaviors to handle both sets of expectations. A boolean driver attribute would do the job.

…niqidentifier

turboezh · 2016-08-16T15:14:14Z

A boolean driver attribute would do the job.

Could you please give me a hint how to name it? DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER?

adambaratz · 2016-08-16T16:18:39Z

That attribute name is perfect.

Added separate PDO::DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER attribute instead of PDO::ATTR_STRINGIFY_FETCHES.

Added `getAttribute` support for PDO::DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER.

turboezh · 2016-08-22T08:03:22Z

Ready for another review.

adambaratz · 2016-08-22T15:53:41Z

Thanks for doing the cleanup. Test coverage is good. Unit tests run clean, with/without valgrind. There are some slight changes that I think are worth making to the attribute handling, making the code and the memory usage more concise. I put these up on a remote branch. If you cherry pick that commit onto your branch, you'll be good to go.

turboezh · 2016-08-23T09:04:25Z

Done.

adambaratz · 2016-08-23T13:31:14Z

@weltling: this is ready to be merged.

weltling · 2016-08-25T17:04:34Z

The exact part from ext/pdo_dblib/tests/stringify_uniqueidentifier.phpt fails with the default setting. I've tried so far to set tds version = 7.0 and the test passed. Not related here, however two other tests fail with tds 7.0 - ext/pdo_dblib/tests/bug_45876.phpt and ext/pdo_dblib/tests/types.phpt. Seems version dependent, so a way to skip tests were indeed useful.

@adambaratz please send a request to internals for becoming a maintainer. Once you have git access, you'll be able to merge PRs yourself.

Thanks for your work, guys!

adambaratz · 2016-08-25T17:08:08Z

@weltling: will do, thanks! I'm currently poking at some of these TDS version inconsistencies... see my comment on #2095.

weltling · 2016-08-25T17:47:29Z

Merged with a000bff.

Thanks!

stayeronglass · 2016-12-08T11:50:54Z

please document this feature

tylers-username · 2017-01-04T20:42:40Z

Agree with @stayeronglass, this should be documented. I have an API that depends on a MSSQL connection and have yet to be able to get it to upgrade to PHP 7 due to this GUID problem.

How do we restore the human readable output?

turboezh · 2017-01-10T10:06:56Z

@stayeronglass, @tylerssn, @adambaratz.
I doubt my english skills will be enough to update official docs and I have no such experience. I will appreciate if anyone would do it.
The summary is:

PDO::DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER (boolean) attribute was added since PHP 7.0.11;
it may be set as other PDO attributes in PDO constructor or with PDO::setAttribute();
when it's FALSE (default), GUIDs returns as 16-byte binary string;
when it's TRUE, GUIDs returns as 36-byte formatted string, like '82A88958-672B-4C22-842F-216E2B88E72A';
TDS protocol version affects this behavior! uniqueidentifier field type supported only in 7.0+ protocol version, so with version <7.0 field value will be always returned as 16-byte binary string regardless of the attribute value.

joelharkes · 2022-03-16T14:06:42Z

has this option been replaced as DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER is no longer available on the PDO class?

cmb69 · 2022-03-16T16:30:11Z

@joelharkes, no, this attribute is still supposed to be supported. Please file a ticket, if you have a PHP with the pdo_dblib extension enabled, and that constant is not defined.

turboezh added 5 commits July 7, 2016 11:17

pdo_dblib: stringify uniqidentifier field

8ef14e8

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE

Merge remote-tracking branch 'php/PHP-7.0' into pdo_dblib_stringify_u…

50501d2

…niqidentifier

pdo_dblib: Stringify uniqidentifier field

5899a98

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE Tests added.

pdo_dblib: Stringify uniqidentifier field

c5c0736

Keep old 5.6 behavior: return Uniqidentifier value as 36-byte hex string (not binary), when PDO::ATTR_STRINGIFY_FETCHES is TRUE Tests fix.

Merge remote-tracking branch 'php/PHP-7.0' into pdo_dblib_stringify_u…

6599c9b

…niqidentifier

turboezh changed the title ~~PDO_DBlib: stringify 'uniqidentifier' fields~~ PDO_DBlib: stringify 'uniqueidentifier' fields Jul 15, 2016

turboezh added 2 commits July 21, 2016 15:21

Merge remote-tracking branch 'php/PHP-7.0' into pdo_dblib_stringify_u…

617c2be

…niqidentifier

Merge branch 'PHP-7.0' into pdo_dblib_stringify_uniqidentifier

0bb888f

weltling mentioned this pull request Jul 25, 2016

Fix #67495: pdo_dblib incorrectly escapes binary and unicode values #2017

Closed

adambaratz reviewed Jul 26, 2016
View reviewed changes

turboezh added 2 commits August 1, 2016 17:28

Merge remote-tracking branch 'php/PHP-7.0' into pdo_dblib_stringify_u…

1f4cec7

…niqidentifier

Merge remote-tracking branch 'php/PHP-7.0' into pdo_dblib_stringify_u…

cf8272f

…niqidentifier

turboezh added 4 commits August 17, 2016 10:23

Merge branch 'PHP-7.0' into pdo_dblib_stringify_uniqidentifier

9a23bc6

pdo_dblib: Stringify uniqueidentifier field

ae0c798

Added separate PDO::DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER attribute instead of PDO::ATTR_STRINGIFY_FETCHES.

Merge branch 'PHP-7.0' into pdo_dblib_stringify_uniqidentifier

c0ebbb0

pdo_dblib: Stringify uniqueidentifier field

a2a2816

Added `getAttribute` support for PDO::DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER.

Simplify storage of stringify_uniqueidentifier attribute

15ce56c

weltling closed this Aug 25, 2016

turboezh deleted the pdo_dblib_stringify_uniqidentifier branch August 26, 2016 08:58

Conversation

turboezh commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling commented Jul 19, 2016

Uh oh!

adambaratz Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

turboezh Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

adambaratz Aug 16, 2016

Choose a reason for hiding this comment

Uh oh!

turboezh Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

adambaratz Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

adambaratz commented Jul 26, 2016

Uh oh!

turboezh commented Aug 16, 2016

Uh oh!

adambaratz commented Aug 16, 2016

Uh oh!

turboezh commented Aug 22, 2016

Uh oh!

adambaratz commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turboezh commented Aug 23, 2016

Uh oh!

adambaratz commented Aug 23, 2016

Uh oh!

weltling commented Aug 25, 2016

Uh oh!

adambaratz commented Aug 25, 2016

Uh oh!

weltling commented Aug 25, 2016

Uh oh!

stayeronglass commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylers-username commented Jan 4, 2017

Uh oh!

turboezh commented Jan 10, 2017

Uh oh!

joelharkes commented Mar 16, 2022

Uh oh!

cmb69 commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

turboezh commented Jul 15, 2016 •

edited

Loading

adambaratz commented Aug 22, 2016 •

edited

Loading

stayeronglass commented Dec 8, 2016 •

edited

Loading