Skip to content

Feature GH-12576: [mysqli] Added transaction check process #12579

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feature GH-12576: [mysqli] Added transaction check process #12579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 deletions ext/mysqli/mysqli_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,11 @@ PHP_FUNCTION(mysqli_commit)
}
MYSQLI_FETCH_RESOURCE_CONN(mysql, mysql_link, MYSQLI_STATUS_VALID);

if (!MYSQLI_IS_IN_TRANSACTION(mysql)) {
php_mysqli_report_error(NULL, 0, "There is no active transaction");
RETURN_FALSE;
}

if (FAIL == mysqlnd_commit(mysql->mysql, flags, name)) {
MYSQLI_REPORT_MYSQL_ERROR(mysql->mysql);
RETURN_FALSE;
Expand Down Expand Up @@ -523,12 +528,12 @@ PHP_FUNCTION(mysqli_execute_query)

if (FAIL == mysql_stmt_prepare(stmt->stmt, query, query_len)) {
MYSQLI_REPORT_STMT_ERROR(stmt->stmt);

close_stmt_and_copy_errors(stmt, mysql);
RETURN_FALSE;
}

/* The bit below, which is copied from mysqli_prepare, is needed for bad index exceptions */
/* The bit below, which is copied from mysqli_prepare, is needed for bad index exceptions */
/* don't initialize stmt->query with NULL, we ecalloc()-ed the memory */
/* Get performance boost if reporting is switched off */
if (query_len && (MyG(report_mode) & MYSQLI_REPORT_INDEX)) {
Expand Down Expand Up @@ -1420,6 +1425,10 @@ PHP_FUNCTION(mysqli_rollback)
}
MYSQLI_FETCH_RESOURCE_CONN(mysql, mysql_link, MYSQLI_STATUS_VALID);

if (!MYSQLI_IS_IN_TRANSACTION(mysql)) {
php_mysqli_report_error(NULL, 0, "There is no active transaction");
RETURN_FALSE;
}

if (FAIL == mysqlnd_rollback(mysql->mysql, flags, name)) {
MYSQLI_REPORT_MYSQL_ERROR(mysql->mysql);
Expand Down
5 changes: 5 additions & 0 deletions ext/mysqli/mysqli_nonapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,11 @@ PHP_FUNCTION(mysqli_begin_transaction)
RETURN_THROWS();
}

if (MYSQLI_IS_IN_TRANSACTION(mysql)) {
php_mysqli_report_error(NULL, 0, "There is already an active transaction");
RETURN_FALSE;
}

if (FAIL == mysqlnd_begin_transaction(mysql->mysql, flags, name)) {
RETURN_FALSE;
}
Expand Down
2 changes: 2 additions & 0 deletions ext/mysqli/php_mysqli_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ extern void php_mysqli_fetch_into_hash_aux(zval *return_value, MYSQL_RES * resul
intern->ptr = NULL; \
}

#define MYSQLI_IS_IN_TRANSACTION(__mysql) \
((mysqli_server_status(__mysql->mysql) & SERVER_STATUS_IN_TRANS) != 0)

ZEND_BEGIN_MODULE_GLOBALS(mysqli)
zend_long num_links;
Expand Down
23 changes: 20 additions & 3 deletions ext/mysqli/tests/mysqli_begin_transaction.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,30 @@ if (!have_innodb($link))

if (mysqli_get_server_version($link) >= 50605) {
/* does it like stupid names? */
if (@!$link->begin_transaction(MYSQLI_TRANS_START_READ_WRITE, "*/trick me?\n\0"))
printf("[020] [%d] %s\n", mysqli_errno($link), mysqli_error($link));
if (@!$link->begin_transaction(MYSQLI_TRANS_START_READ_WRITE, "*/trick me?\n\0")) {
printf("[020] [%d] %s\n", mysqli_errno($link), mysqli_error($link));
} else {
mysqli_rollback($link);
}

/* does it like stupid names? */
if (@!$link->begin_transaction(MYSQLI_TRANS_START_READ_WRITE, "az09"))
if (@!$link->begin_transaction(MYSQLI_TRANS_START_READ_WRITE, "az09")) {
printf("[021] [%d] %s\n", mysqli_errno($link), mysqli_error($link));
} else {
mysqli_rollback($link);
}
}

mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);
try {
mysqli_begin_transaction($link);
mysqli_begin_transaction($link);
} catch (\mysqli_sql_exception $e) {
echo $e->getMessage() . \PHP_EOL;
}

mysqli_rollback($link);
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mysqli_rollback($link);
mysqli_commit($link);

to test if the first mysqli_begin_transaction can still be commit

Copy link
Member Author

Choose a reason for hiding this comment

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

The result is the same no matter which one you use. This is because the only errors that occur are due to the checks added this time.

Personally, I'm fine with either.


print "done!";
?>
--CLEAN--
Expand All @@ -102,4 +118,5 @@ if (!have_innodb($link))
--EXPECT--
NULL
mysqli_begin_transaction(): Argument #2 ($flags) must be one of the MYSQLI_TRANS_* constants
There is already an active transaction
done!
9 changes: 9 additions & 0 deletions ext/mysqli/tests/mysqli_commit.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ if (!have_innodb($link))
echo $exception->getMessage() . "\n";
}

$link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket);
mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);
try {
mysqli_commit($link);
} catch (\mysqli_sql_exception $e) {
echo $e->getMessage() . \PHP_EOL;
}

print "done!";
?>
--CLEAN--
Expand All @@ -64,4 +72,5 @@ require_once 'clean_table.inc';
?>
--EXPECT--
mysqli object is already closed
There is no active transaction
done!
6 changes: 6 additions & 0 deletions ext/mysqli/tests/mysqli_commit_oo.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ if (!have_innodb($link))

$mysqli = new my_mysqli($host, $user, $passwd, $db, $port, $socket);

$mysqli->begin_transaction();
if (true !== ($tmp = $mysqli->commit())) {
printf("[002] Expecting boolean/true got %s/%s\n", gettype($tmp), $tmp);
}
Expand Down Expand Up @@ -66,16 +67,21 @@ if (!have_innodb($link))
printf("[011] [%d] %s\n", $mysqli->errno, $mysqli->error);
}

$mysqli->begin_transaction();
if (!$mysqli->commit(0 , "tx_name0123")) {
printf("[012] [%d] %s\n", $mysqli->errno, $mysqli->error);
}

$mysqli->begin_transaction();
var_dump($mysqli->commit(0 , "*/ nonsense"));

$mysqli->begin_transaction();
var_dump($mysqli->commit(0 , "tx_name ulf вендел"));

$mysqli->begin_transaction();
var_dump($mysqli->commit(0 , "tx_name \t\n\r\b"));

$mysqli->begin_transaction();
if (!$mysqli->commit(MYSQLI_TRANS_COR_AND_CHAIN | MYSQLI_TRANS_COR_NO_RELEASE , "tx_name")) {
printf("[016] [%d] %s\n", $mysqli->errno, $mysqli->error);
}
Expand Down
8 changes: 6 additions & 2 deletions ext/mysqli/tests/mysqli_report.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,9 @@ Warning: mysqli_real_query(): (%d/%d): You have an error in your SQL syntax; che

Warning: mysqli_autocommit(): (%s/%d): Commands out of sync; you can't run this command now in %s on line %d

Warning: mysqli_commit(): (%s/%d): Commands out of sync; you can't run this command now in %s on line %d
Warning: mysqli_commit(): (%s/%d): There is no active transaction in %s on line %d

Warning: mysqli_rollback(): (%s/%d): Commands out of sync; you can't run this command now in %s on line %d
Warning: mysqli_rollback(): (%s/%d): There is no active transaction in %s on line %d

Warning: mysqli_stmt_prepare(): (%s/%d): Commands out of sync; you can't run this command now in %s on line %d

Expand All @@ -336,6 +336,10 @@ Warning: mysqli_store_result(): (%s/%d): You have an error in your SQL syntax; c
Warning: mysqli_stmt_attr_set(): (%s/%d): Not implemented in %s on line %d
mysqli_kill(): Argument #2 ($process_id) must be greater than 0

Warning: mysqli_commit(): (%s/%d): There is no active transaction in %s on line %d

Warning: mysqli_rollback(): (%s/%d): There is no active transaction in %s on line %d

Warning: mysqli_stmt_prepare(): (%d/%d): You have an error in your SQL syntax; check the manual that corresponds to your %s server version for the right syntax to use near 'FOO' at line 1 in %s on line %d
[013] Access denied for user '%s'@'%s'%r( \(using password: \w+\)){0,1}%r
[016] Access denied for user '%s'@'%s'%r( \(using password: \w+\)){0,1}%r
Expand Down
9 changes: 9 additions & 0 deletions ext/mysqli/tests/mysqli_rollback.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ if (!have_innodb($link))
echo $exception->getMessage() . "\n";
}

$link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket);
mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT);

should be not needed because of https://wiki.php.net/rfc/mysqli_default_errmode

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I thought at first, but it seems like this is a global setting. If I didn't set it like this here, I ended up in warning mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Warning mode is set at the beginning of this test case, and if I do not change the mode with this line, the warning mode will be inherited even if I create a new mysqli instance.

Copy link
Contributor

@mvorisek mvorisek Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see - https://github.com/php/php-src/blob/832c30f38df/ext/mysqli/tests/connect.inc#L10 - new mysqli_driver is an instance of the monostate pattern - https://www.php.net/manual/en/class.mysqli-driver.php

Copy link
Member Author

Choose a reason for hiding this comment

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

right.

try {
mysqli_rollback($link);
} catch (\mysqli_sql_exception $e) {
echo $e->getMessage() . \PHP_EOL;
}

print "done!\n";
?>
--CLEAN--
Expand All @@ -61,4 +69,5 @@ require_once 'clean_table.inc';
?>
--EXPECT--
mysqli object is already closed
There is no active transaction
done!