Skip to content

Conversation

kalaspuffar
Copy link

Changes:

  • Extended zend_throw_exception_ex and created a pdo_throw_exception_ex that will populate the errorInfo property of the PDOException class.
  • Added testcase for Bug #64705 errorInfo property of PDOException is null when PDO::__construct() fails.
  • Changed all drivers so they use pdo_throw_exception_ex instead of zend_throw_exception_ex.

Tested with:

  • DSN sqlite - sqlite memory database
  • DSN mysql - mysql database locally
  • DSN pgsql - postgresql database locally
  • DSN odbc - db2 database locally
  • DSN firebird - firebird database locally
  • DSN oci - oracle database locally
  • DSN dblib - mssql database Amazon EC2

@kalaspuffar
Copy link
Author

After reading the comments closely on the comments on the document page I suggest we make a document change to be more explicit and change what code is.

An exception has a code int that is populated from the internal error code from the different PDO drivers. But the SQLState (5 char) should not override this in the PDOException implementation. This makes the class hard to use and does not work with already written tests.

So my suggestion is that we keep the code int and return the errorInfo structure as normal with the SQLState included.

Exception::errorInfo = array(3) {
[0]=>
string(5) "%s"
[1]=>
int(%d)
[2]=>
string(%d) "SQLSTATE[%s]%A"
}

So index 0 is the state, index 1 is the driver code and lastly index 2 is the message.
Same driver code is the exception code. And the message is also the same for the Exception.

Have a value being both a String and Int at the same time will confuse unnecessarily.

@smalyshev smalyshev added the Bug label Oct 17, 2015
@@ -38,6 +38,51 @@

static int pdo_dbh_attribute_set(pdo_dbh_t *dbh, long attr, zval *value TSRMLS_DC);

PDO_API zval *pdo_throw_exception_ex(zend_class_entry *exception_ce, pdo_error_type *pdo_err, long code TSRMLS_DC, const char *format, ...) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

Every time pdo_throw_exception_ex is ever called exception_ce parameter is always php_pdo_get_exception(). Since this is clearly a PDO-specific function, maybe this parameter should be eliminated and this function should call php_pdo_get_exception() instead?

Copy link
Author

Choose a reason for hiding this comment

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

Well, zend_throw_exception_ex was extendable so I wanted to keep that option, but your right this is possibly not required or wanted in a library where the purpose is to unify driver interaction.

Tyrael and others added 19 commits April 15, 2016 10:32
Doing a less intrusive variant of the PHP 7.0 fix for 5.6.
Actually, this only be fixed if php uses mysqlnd
… that will populate the errorInfo property of the PDOException class.
@krakjoe
Copy link
Member

krakjoe commented Jan 6, 2017

Since this targets a security fix only branch, and since a patch against a supported branch would have to be different, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch, in addition please leave whitespace changes out of any subsequent PR, and make those as separate PR's.

@krakjoe krakjoe closed this Jan 6, 2017
@kalaspuffar
Copy link
Author

@krakjoe Thank you for the feedback.

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.

7 participants