-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Remove driver specific support from ODBC #15727
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I'm all for removing legacy cruft, especially early in the "new" master branch (i.e. in a couple of weeks). I can barely comment on the Unix specialties, though.
715ba27
to
de911c2
Compare
Rebased this; I also suspect |
RFC approved, I'll take another look at this. |
PDO_ODBC doesn't do this, and most of these drivers are not in use with PHP, at least like this. Chances are these expose an ODBC driver you can use with a normal driver manager like unixODBC or iODBC. If not, it can be specified as a custom driver, though it does not include any workarounds. There might be some redundant definitions now as a result. IBM Db2 is kept as a special case due to it also being in PDO_ODBC, though I wonder how good of an idea this is. See phpGH-15630
This would only be used on 68k classic Mac OS. Did PHP ever run there?
All supported driver managers can do extended fetches.
So we don't need the define?
There's no justification behind as to why this should be.
de911c2
to
efcfe76
Compare
The default handling for turning into SQL_C_CHAR is fine, and the special case for Adabas is no longer needed.
The driver managers and even Db2 support this. This would also allow simplifying the fetch code to merge fetch_into and fetch_array into a single implementation perhaps.
FWIW, I think this should be good to review. I have some ideas on how to clean up the fetch code if we can assume all the driver managers implement them, but that should be done in a separate PR. I'm also not sure of removing the Db2 special casing, as it seems Zend was using it in the past at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RM approval, technical review not performed
See GH-15630 for motivation and some concerns. Looks like we can get rid of a bunch of ifdefs!
Additional ones I've had since this:
This seems unneeded;SQL_TIMESTAMP
is gated behind Adabas for some reason (inodbc_bindcols
), even though it seems general? May need to look into how it gets converted toSQL_C_CHAR
per ODBC ref.SQL_C_CHAR
binding should be correct based on display size.ahahaha accidentally deleted it in a cleanup, added backHAVE_SQLDATASOURCES
only gets set on Windows, need to test or assume on Unix