Skip to content

Conversation

NattyNarwhal
Copy link
Member

As an example using the IBM Db2i ODBC driver:

PDO::ATTR_SERVER_INFO: DB2/400 SQL
PDO::ATTR_SERVER_VERSION: 07.02.0015

Sidebar (probably for @cmb69):

Other attribute types seem to be sporadically implemented across PDO drivers, so I haven't implemented them yet. For example, about half of the drivers implement the autocommit attribute by just returning it from the PDO state, others don't. Makes me curious what should be missing. After this patch, I do know autocommit, connection status, prefetch, and timeout are missing gettings, and some of that probably doesn't map to ODBC anyways. (Setters I haven't even gotten into.)

As an example using the IBM Db2i ODBC driver:

PDO::ATTR_SERVER_INFO: DB2/400 SQL
PDO::ATTR_SERVER_VERSION: 07.02.0015

Sidebar (probably for @cmb69):

Other attribute types seem to be sporadically implemented across
PDO drivers, so I haven't implemented them yet. For example, about
half of the drivers implement the autocommit attribute by just
returning it from the PDO state, others don't. Makes me curious what
should be missing. After this patch, I do know autocommit, connection
status, prefetch, and timeout are missing gettings, and some of that
probably doesn't map to ODBC anyways. (Setters I haven't even gotten
into.)
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for this nice improvement. I've left some comments, and also would suggest to add a test (even if the test just checks that a string is returned).

@ramsey ramsey added the Feature label May 7, 2021
@ramsey
Copy link
Member

ramsey commented May 7, 2021

@NattyNarwhal Thanks for addressing @cmb69's feedback. 👍🏻

@ramsey ramsey merged commit 207666e into php:master May 7, 2021
@cmb69
Copy link
Member

cmb69 commented May 7, 2021

@ramsey, I think this should have an entry in UPGRADING (so it doesn't get overlooked for the migration guide). Could you please add it? An entry in NEWS might be appropriate, too.

@ramsey
Copy link
Member

ramsey commented May 7, 2021

I'll get that added. Thanks!

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.

3 participants