-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[RFC] Change default PDO error mode to exceptions #5388
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
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.
I guess we're not 100% consistent on this but the C file should be indented with tabs only.
I can confirm that no changes are needed for pdo_dblib. The tests for that extension already set the error mode to exceptions and expect those results. |
There are some bugs around the exception error mode that perhaps is more important to fix once this becomes the default mode. False is returned and no exception is thrown: #4288 Warning is triggered instead of an exception: https://bugs.php.net/bug.php?id=74401 |
CI failures spurious. |
Can someone please check whether this needs changes for pdo_oci? cc @cjbj |
pdo_oci test runs show no differences with master for the current test failures. Additionally, none of the failures are related to these changes as far as I can tell:
|
@cmb69 @camporter that agrees with what I saw. |
What still needs to be done before this PR can be merged? Is there anything more I need to do? |
@AllenJB As far as I can tell, you should add an UPGRADING entry for this change |
Thanks for the RFC and the PR. Applied as 5075240. |
These have apparently been missed when PR 5388[1] had been merged. [1] <#5388>
RFC: https://wiki.php.net/rfc/pdo_default_errmode
I have implemented the change and updated the tests for pdo, pdo_sqlite and pdo_mysql.
It is quite possible additional PDO extension tests will need updating (specifically many tests never explicitly set the error mode), but I don't currently have working installs and thought I'd post something up for others to look at.
I did additionally update the pdo_mysql tests README with some notes on the permissions required for the MySQL user.
Some "points of interest" in the tests I updated: