Skip to content

Fix PDOStatement->execute() failed, then execute successfully, errorInfo() information is incorrect #8628

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.

Sign up for GitHub

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 9 commits into from
Closed

Fix PDOStatement->execute() failed, then execute successfully, errorInfo() information is incorrect #8628

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8283f9d
Fix PDOStatement->execute() failed, then execute successfully, errorI…
Yurunsoft May 25, 2022
e1ba1b2
Fix other driver test
Yurunsoft May 25, 2022
4a00255
Fix test
Yurunsoft May 25, 2022
c402f11
Fix test
Yurunsoft May 25, 2022
46d4402
Fix I386 test
Yurunsoft May 25, 2022
b00dabf
Support pgsql, sqlite driver test
Yurunsoft May 26, 2022
d0be22e
Fix
Yurunsoft May 26, 2022
1810d47
remove skip
Yurunsoft Jun 1, 2022
0b7427d
Fix odbc test
Yurunsoft Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ext/pdo/pdo_stmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1617,8 +1617,10 @@ PHP_METHOD(PDOStatement, errorInfo)
array_init(return_value);
add_next_index_string(return_value, stmt->error_code);

if (stmt->dbh->methods->fetch_err) {
stmt->dbh->methods->fetch_err(stmt->dbh, stmt, return_value);
if (strncmp(stmt->error_code, PDO_ERR_NONE, sizeof(PDO_ERR_NONE))) {
if (stmt->dbh->methods->fetch_err) {
stmt->dbh->methods->fetch_err(stmt->dbh, stmt, return_value);
}
}

error_count = zend_hash_num_elements(Z_ARRVAL_P(return_value));
Expand Down
54 changes: 54 additions & 0 deletions ext/pdo/tests/gh8626.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
--TEST--
GH-8626: PDOStatement->execute() failed, then execute successfully, errorInfo() information is incorrect
--SKIPIF--
<?php
if (!extension_loaded('pdo') || !extension_loaded('pdo_mysql')) die('skip');
$dir = getenv('REDIR_TEST_DIR');
if (false == $dir) die('skip no driver');
if (!str_starts_with(getenv('PDOTEST_DSN'), 'mysql')) die('skip non-mysql drivers');
Copy link
Member

Choose a reason for hiding this comment

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

If the test is only meant for PDO_MySQL, there is no point in putting it into ext/pdo/tests and making it a redirect test. Instead, the test should be put into ext/pdo_mysql/tests. However, since there is a behavioral change for all PDO drivers, shouldn't we test it for all drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very familiar with other databases, so I had to use MySQL to test it.
And, I'm not quite sure why the I386 test failed.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem the usage of PHP_INT_MIN to trigger a failure. PHP_INT_MIN is -2147483648 on 32bit architectures, and that wouldn't be problem for the database. Can't we use something else to trigger a failure? I'm thinking about trying to insert a non numeric string into an INTEGER column, or such. That should work for other drivers as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added mysql, pgsql, sqlite tests.
Other databases I am really not familiar with and cannot test.

require_once $dir . 'pdo_test.inc';
PDOTest::skip();
?>
--FILE--
<?php
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';

$db = PDOTest::factory();
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);

$db->exec("SET SESSION sql_mode=CONCAT((select @@sql_mode),',STRICT_TRANS_TABLES')");
@$db->exec('DROP TABLE test');
$db->exec('CREATE TABLE test (x int)');

$stmt = $db->prepare('INSERT INTO test VALUES(?)');

// fail
var_dump($stmt->execute([PHP_INT_MIN]), $stmt->errorCode(), $stmt->errorInfo());

// success
var_dump($stmt->execute([1]), $stmt->errorCode(), $stmt->errorInfo());
?>
===DONE===
--EXPECTF--
bool(false)
string(%d) "%s"
array(3) {
[0]=>
string(%d) "%s"
[1]=>
int(%d)
[2]=>
string(%d) "%s"
}
bool(true)
string(5) "00000"
array(3) {
[0]=>
string(5) "00000"
[1]=>
NULL
[2]=>
NULL
}
===DONE===
2 changes: 1 addition & 1 deletion ext/pdo_pgsql/tests/bug62593.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ $expect = 'No errors found';

foreach ($errors as $error)
{
if (strpos('Invalid text representation', $error[2]) !== false)
if (null !== $error[2] && strpos('Invalid text representation', $error[2]) !== false)
{
$expect = 'Invalid boolean found';
}
Expand Down