Skip to content

[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

Closed
wants to merge 6 commits into from

Conversation

AllenJB
Copy link
Contributor

@AllenJB AllenJB commented Apr 14, 2020

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:

  • Several tests give double warnings, with the second warning containing less information. I verified this was existing behavior the same test code and a copy of PHP built from master with the same configure options on the same system (ie. do not have my change but are otherwise identical).
    • pdo\tests\bug_44159.phpt
    • pdo_sqlite\tests\pdo_fetch_func_001.phpt
  • pdo_sqlite\tests\pdo_sqlite_transaction.phpt causes a "table is locked" error when attempting to drop the table at the end of the test, but I couldn't see why this occurred.
  • ext/pdo_mysql/tests/pdo_mysql_pconnect.phpt: I updated this test to use silent mode. There's notes in the tests about errors not occurring with libmysql. Can libmysql still be used with PHP or is mysqlnd now the only option? Could the test be rewritten an improved?

Copy link
Member

@iluuu1994 iluuu1994 left a 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.

@kocsismate kocsismate added the RFC label Apr 14, 2020
@adambaratz
Copy link
Contributor

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.

@Llbe
Copy link

Llbe commented Apr 21, 2020

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

@nikic
Copy link
Member

nikic commented Apr 27, 2020

CI failures spurious.

@cmb69
Copy link
Member

cmb69 commented Apr 27, 2020

Can someone please check whether this needs changes for pdo_oci? cc @cjbj

@camporter
Copy link
Contributor

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:

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
via [ext/pdo_oci/tests/common.phpt]
	OCI PDO Common: FR #71885 (Allow escaping question mark placeholders) [ext/pdo_oci/tests/bug_71885.phpt]
via [ext/pdo_oci/tests/common.phpt]
	OCI PDO Common: Bug #73234 (Emulated statements let value dictate parameter type) [ext/pdo_oci/tests/bug_73234.phpt]
PDO_OCI: PDOStatement->getColumnMeta [ext/pdo_oci/tests/pdo_oci_stmt_getcolumnmeta.phpt]
PDO OCI: Inserts 10K with 1 number and 2 LOB columns (stress test) [ext/pdo_oci/tests/pdo_oci_stream_2a.phpt]
=====================================================================

@cjbj
Copy link
Contributor

cjbj commented May 1, 2020

@cmb69 @camporter that agrees with what I saw.

@AllenJB
Copy link
Contributor Author

AllenJB commented May 4, 2020

What still needs to be done before this PR can be merged? Is there anything more I need to do?

@kocsismate
Copy link
Member

@AllenJB As far as I can tell, you should add an UPGRADING entry for this change

@cmb69
Copy link
Member

cmb69 commented May 4, 2020

Thanks for the RFC and the PR. Applied as 5075240.

@cmb69 cmb69 closed this May 4, 2020
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 29, 2020
php-pulls pushed a commit that referenced this pull request Jul 12, 2020
These have apparently been missed when PR 5388[1] had been merged.

[1] <#5388>
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.

10 participants